Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto route integration #8

Merged
merged 12 commits into from
Apr 14, 2013
Merged

Auto route integration #8

merged 12 commits into from
Apr 14, 2013

Conversation

dantleech
Copy link
Member

This PR integrates the auto-route-bundle with the Blog.

Posts and Blogs are now routed automatically, the configuration is included from the sandbox config,yml using "import".

I have removed the BlogRouteManager, which breaks the Tag routing. Tags are not PHPCR documents and so cannot be handled with the AutoRouteBundle yet. Any idegas?

  1. Maybe the AutoRouteBundle should support appending routes, e.g. The Blog route should always have a child "tag" with the pattern "{tag_name}".
  2. Tags should be persisted as PHPCR documents and handled in that way.

@@ -124,6 +124,6 @@ public function getSubRoutes($routeClass)

public function __toString()
{
return $this->name;
return $this->name ? : '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we started doing return (string) $this->name; in the other places. can you do the same here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for sure, I guess there was a change in the PHPCR stack logic recently? I had to add this code to avoid an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sonata admin is using to string i think. and when you have a new document without a name the tostring would fail. as it must return string and only string. hence also the (string) cast, in case somebody manages to put something not a string into the name field.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dbu
Copy link
Member

dbu commented Mar 23, 2013

great. i think it really was the right decision to separate route generation from this bundle!

i commented a few things, most importantly i think we should make the dependency on autoroute just a suggestion. and document that you either need that or need to extend the admin to have the user define routes or generate them otherwise. the autoroute bundle does not take hints from the user, right? a few systems i have seen where sluggifying the title to propose you the url, but you could still hand-tune that afterwards... i don't want to do that here now, but we should not block the path for it.

regarding tags: i think the most dev-friendly way indeed would be to have some sort of TaxonomyBundle (i think the drupal model is really quite good for that feature). it would allow you to define taxonomies which are flat or tree structured collections of tags. each tag would be a document that references content (blog, static content, media, ...) and that content can use referrers if it wants, or run through the document manager getReferrers method. when mapped on documents, you need not even custom controllers but just have the tags RouteAwareInterface and maybe have a route per taxonomy that has a dynamic parameter for the actual tag. or autoroute the tags as well. definitely out of scope of this pull request, but when creating it you talked about it :-)

lets make this work without routeable tags for now, and then if you want we could bring the discussion about tagging onto the mailinglist.

@dantleech
Copy link
Member Author

Removed hard dep. on sonata. I think this branch is going to become the next version of blog bundle. To be feature complete with master we need the tagging stuff.

@dbu
Copy link
Member

dbu commented Apr 9, 2013

so you mean you don't want to merge it? or you want to merge? i would merge it to have the functionality now. its not in conflict with the tag feature.

@dantleech
Copy link
Member Author

Well, the problem is that in master the old RouteManager currently handles the generation of the Tags route, which is appended to the blog route. There is no counterpart for that here. So tags are not routed in this branch and indeed I have removed them.

In any case, I ran up against a problem this morning when progressing with the tutorial, which might be due to a bug in this branch, so its not ready for merging yet in anycase.

@dantleech
Copy link
Member Author

I think it really needs a standalone functional test too ...

dantleech added a commit that referenced this pull request Apr 14, 2013
Auto route integration. REMOVES TAG FUNCTIONALITY! Use 0.1 if this is a concernt.
@dantleech dantleech merged commit c6efa10 into master Apr 14, 2013
@dantleech dantleech deleted the auto_route branch April 14, 2013 12:06
->scalarNode('blog_admin')->treatNullLike('Symfony\Cmf\Bundle\BlogBundle\Admin\BlogAdmin')->isRequired()->end()
->scalarNode('post_admin')->treatNullLike('Symfony\Cmf\Bundle\BlogBundle\Admin\PostAdmin')->isRequired()->end()
->scalarNode('blog')->treatNullLike('Symfony\Cmf\Bundle\BlogBundle\Document\Blog')->isRequired()->end()
->scalarNode('post')->treatNullLike('Symfony\Cmf\Bundle\PostBundle\Document\Post')->isRequired()->end()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the right thing to not need to specify the class element in the config?

@dbu
Copy link
Member

dbu commented Apr 15, 2013

ah i see you merged. no problem, lets re-check and fix things as you write the documentation for it. the direction seems right, now we can polish the pieces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants