symfony-cmf / routing-auto

RoutingAuto component
https://cmf.symfony.com
Other
6 stars 11 forks source link

Multiple routes refactoring. #62

Closed dantleech closed 8 years ago

dantleech commented 8 years ago

This PR refactors the building of routes to enable the possiblity for us to have multiple routes per object.

The essential change here is that the UriGenerator no longer has a dependency on the ClassMetadataFactory - this depenency forced the UriGenerator only to act once upon an object.

Now each UriContext is populated with the necessary data from the metadata (uri schema, conflict resolver, token providers) before being passed to the UriGenerator. This means that the UriGenerator can generate many different URIs for the same object.

The next step (another PR) would be the specification of multiple "schemas" per object in the ,metadata configuration. (implemented).

This PR makes the second argument to AdapterInterface::createAutoRoute($uriContext, $document, $tag) meaningless, as the context now has all the information required. This method signature has already been changed in master. This PR does NOT change the interface, but it should be done before the next release.

BC: This PR is BC but deprecates the assignation of uri_schema to a string.

stdClass:
    uri_schema: 
        edit:
            template: /forum/{category}/{post_title}/edit
            defaults:
                _type: edit
        view:
            template: /forum/{category}/{post_title}/view
            defaults:
                _type: view
    token_providers:
        category: [foo]
<?xml version="1.0" ?>
<auto-mapping xmlns="http://cmf.symfony.com/schema/routing_auto">
    <mapping class="stdClass">
        <uri-schema template="/forum/{category}/{post_title}/edit">
            <default key="_type">edit</default>
        </uri-schema>

        <uri-schema template="/forum/{category}/{post_title}/view">
            <default key="_type">view</default>
        </uri-schema>

        <conflict-resolver name="auto_increment">
            <option name="token">category</option>
        </conflict-resolver>

        <token-provider token="category" name="method">
            <option name="method">getCategoryName</option>
        </token-provider>

        <token-provider token="slug" name="property">
            <option name="property">title</option>
            <option name="slugify">true</option>
        </token-provider>
    </mapping>
</auto-mapping>
wouterj commented 8 years ago

This replaces https://github.com/symfony-cmf/RoutingAuto/pull/48?

wouterj commented 8 years ago

We should be carefull here to not advocate the usage of 2 URLs with exactly the same content. Maybe we should enforce that a _type/_controller setting is set for the route (so it'll be handled by a different controller)? (maybe opt-in, so people can still have 2 routes serving the same content, e.g. when using a front-end framework like Angular)

dbu commented 8 years ago

We should be carefull here to not advocate the usage of 2 URLs with /exactly/ the same content. Maybe we should enforce that a |_type|/|_controller| setting is set for the route (so it'll be handled by a different controller)? (maybe opt-in, so people can still have 2 routes serving the same content, e.g. when using a front-end framework like Angular)

the seo bundle also has a canonical url functionality. that could help as well. but i agree that its often not desired to have multiple urls to the same content.

dantleech commented 8 years ago

@WouterJ @dbu in the next PR (or this one, whatever is best) the metadata configuration will allow the specification of defaults, so the user can set the _type for example.

Saying that - what is wrong with having the same content+controller combo for two different routes?

And, in other news: How do we indicate which route should be generated when using things such as {{ path(contentObject) }} ?

dbu commented 8 years ago

the problem i think of (and i think wouter too) is that google will rate pages lower when they show the same content under several urls

dantleech commented 8 years ago

Ah ok, well - I think we should advertise this, but it shouldn't be a built in limitation imo.

dbu commented 8 years ago

i agree. there are probably also use cases where the same controller and content provide a different page.

one case that comes to mind is multilanguage.

dantleech commented 8 years ago

Have added support to the XML and YAML file loaders, introducing the AutoRouteDefinition class.

Next task would be ensuring that merging (extends) works properly - it should be possible to define the "definitions" with a name, enabling them to be overridden explicitly by extending classes.

dantleech commented 8 years ago

Updated, this is ready for final review. /cc @WouterJ @dbu

I think we should simply release master as 2.0 next, making all the other BC breaking changes in the issue list.

dantleech commented 8 years ago

ping @WouterJ @dbu shall we merge this?

dbu commented 8 years ago

i do not follow routing auto that closely. i guess its ok. rather do a 3.0 later this year than never release 2.

dantleech commented 8 years ago

Need to add tests for the legacy support

dantleech commented 8 years ago

Potential bug: When a new directory level is added (e.g. changing schema from /foo/{title} to /foo/title/{title}) the redirect route is not /foo/hello, but {/foo/title/hello}.

This is perhaps already an issue and not related to this. This is a regression with this PR.

dantleech commented 8 years ago

^^ Have tried extensively to reproduce this issue and cannot, I assume then that I was confused in the first place.

wouterj commented 8 years ago

Tests/Resources/Fixtures/.Article.php.swo should be removed from the diff

wouterj commented 8 years ago

I'm not sure I can follow all logic in this PR (I'm not following RoutingAuto that closely the past year), but the description and tests look fine to me.

dantleech commented 8 years ago

Tests/Resources/Fixtures/.Article.php.swo should be removed from the diff

This PR removes that file, it is already in master.

dantleech commented 8 years ago

Squashed.

dantleech commented 8 years ago

@WouterJ @dbu i have removed the deprecated features, as this is now the 2.0 branch Note that the documentation has already been merged by accident .. :)

dantleech commented 8 years ago

@WouterJ if you want to review, feel free. Otherwise I will merge this on Saturday.

wouterj commented 8 years ago

Documentation has to be reverted, as the docs show CMF 1.3. Version 2.0 won't be included in CMF 1.3.

wouterj commented 8 years ago

Feel free to merge btw