symfony-cmf / routing-auto-bundle

Adding automatic route generating on top of the symfony cmf routing
https://cmf.symfony.com
14 stars 29 forks source link

content_method with params #136

Open petforsberg opened 9 years ago

petforsberg commented 9 years ago

There appeared certain problem here:

https://github.com/symfony-cmf/RoutingAutoBundle/issues/132

it is due to use of Global Parameters (e.g. node-paths defined in parameters.yml) in Documents / Entities. Since using external parameters within plain PHP objects is considered not a good practice, what about doing sth like this from inside a Route Auto config section:

        forumPath: [content_method, { method: getParentPath, slugify: false, parameters: %some_cms_path% }]

with this new "parameters" feature, it would be possible to provide global %some_cms_path% variable to the getParentPath method, defined as following:

getParentPath($cmsPath)
{
    //...
}
dbu commented 9 years ago

could we leverage expression language for those use cases?

and i would vote to only do that for the 1.1 version. we are about to release a cmf version and routing auto never was stable so its high time. nothing prevents us from doing 1.1 in 1 month if we have a bunch of new features by then...

dantleech commented 9 years ago

Sorry my last answer was so off track I deleted it :)

Just to think of some alternatives, what about a cotnent_path provider?

formPath: [ content_path, { from: %path_to_form_in_content_tree%, slugify: true } ]

So content_path will provide the entire content path, but offset it by the length of the value given by the from parameter (and also throw an Exception if the path does not match).

The slugify option will operate as with the content_method provider except that it will not slugify the forward slashes.

//cc @WouterJ

petforsberg commented 9 years ago

Might be too restrictive (not flexible enough) I'm afraid. The highest flexibility is given by custom methods, as provided by the "content_method" provider. Having no parameters passed to methods might be considered a bit weird?

Besides, I think it's most intuitive way - is there a need to provide new things to learn like another expression languages, while there is so simple possible solution?

dantleech commented 9 years ago

The problem I have with using the content method is that, to me, it seems like a work around for a missing feature.

Flexibility is good, and I am not against adding it - it would help people implement quick workarounds, but I think your use case would be better addressed with a provider - this use-case was covered explicitly in the old routing-auto bundle, so I would like to see a real solution here too.

Another issue is that content_method slugifies the entire return value (including slashes). Disabling it will mean creating potentially unsafe URLs.

wouterj commented 9 years ago

I'm -1 for adding a feature like this. Let me explain:

The main goal of the Auto Route is to create a route based on the data of an entity. To get access to this data, we needed to use the getters of an entity. That's why we introduced the content_method provider, to call a method to get some data from an entity.

The entity is still an object that holds data and nothing more. It should not contain any logic about what's in the route. In fact, the entity shouldn't even know what a route is. If you need to do custom things for creating the route (i.e. you would have put logic in your entity), you need to refactor that to a custom provider.

petforsberg commented 9 years ago

@WouterJ Yes, this is correct that Entity / Document shouln't know about what is "around". The problem has a wider scope however. I just think that the framwork (Symfony CMF with PHPCR in this case, but the problem is not present with standard Symfony with RDBMS / ORM) shouldn't impose such limitation as they turned out here, I guess and there must be found a general solution.

Let me recall the original problem here, which in fact is quite common and general case: How to create relative paths with Auto Route? If it's not possible, then you can't create e.g. a forum with any nodes depth. Do you want such limitation for Symfony CMF (assuming that we're using Route Auto bundle), where nodes depth can't be of any level? If so, I'm afraid the Route Auto cannot be used. I would say that hierarchical storage is for the cases like this, even if I'm new to the Content Repository.

So, perhaps another way is possible: to provide relative instead of absolute paths with the Route Auto?

dantleech commented 9 years ago

There absolutely is a good way to implement this with the route providers without resorting to the content_method. Lets find that way before thinking about anything else. As you say this is a common requirement and it should be in the core.

There used to be what we would now call a "parent" provider, which returned the path of the content documents parent object. I will have a think about this later -- I can't remember now if there is something fundamentally wrong with that...

petforsberg commented 9 years ago

Provider sth like:

parentPath: [ parent_path ]

?

wouterj commented 9 years ago

Yes and then with an option strip_base_path with the path to strip, which can be a container parameter Op 22 okt. 2014 15:31 schreef "petforsberg" notifications@github.com:

Provider sth like:

parentPath: [ parent_path ]

?

— Reply to this email directly or view it on GitHub https://github.com/symfony-cmf/RoutingAutoBundle/issues/136#issuecomment-60084521 .

eggyal commented 9 years ago

For the time being I have hacked around this issue by creating a "parent path" token whose TokenProviderInterface::provideValue() is as follows ($this->dm is the document manager, injected into the class constructor):

public function provideValue(UriContext $uriContext, $options)
{
    $parent = $uriContext->getSubjectObject()->getParentDocument();

    foreach ($this->dm->getReferrers($parent) as $referrer)
            if ($referrer instanceof Route)
                    return $referrer->getPath();
}

This is suboptimal because:

  1. It has to inspect every document that refers to the parent in order to find its possible routes—if there are many references to the parent, this could prove rather wasteful; and
  2. It arbitrarily selects the first route that it finds—where multiple routes exist to the parent document, one may wish to be more discriminating (in actual fact I'm discriminating by locale).

No doubt there are better ways to accomplish this!

dantleech commented 9 years ago

@eggyal regarding the route selection, that is indeed an issue. See https://github.com/symfony-cmf/Routing/issues/130.

In this case you might be able to further optimize by searching only for AutoRoute instances (providing your parent document does not do manual routing) and ensuring that the route is not a redirect route.

eggyal commented 9 years ago

@dantleech: Thanks for that. As it happens, I'm already restricting my search to instances of AutoRoute (because they're the only routes for which I can ascertain locale, courtesy of getAutoRouteTag()).

Given that my documents all implement RouteReferrersReadInterface, I initially tried to use their getRoutes() method instead of DocumentManager::getReferrers()—but it seems that the underlying property is not hydrated upon flushing the initial commit, so my fixtures all ended up with routes in the root space. Do you have any idea why the property isn't getting hydrated upon the initial flush() (it's loaded fine when documents are subsequently fetched)?

dantleech commented 9 years ago

Hmm, I am not 100% sure what the expected behavior for the ODM is there. But in anycase we do something similar in the ODM adapter in this bundle: https://github.com/symfony-cmf/RoutingAutoBundle/blob/master/Adapter/PhpcrOdmAdapter.php#L186. /cc @dbu

dbu commented 9 years ago

i think the reason that the referrers mapping is not refreshed is the cost of that operation. we would need to reset all loaded referrer mappings even of unchanged documents whenever a new document is created.

though if there is a way to mark the referrer collections as "dirty" so they reload when accessed again, that would probably be bearable. and its only needed when reference collections are changed during save.

this would require to dig rather deep into phpcr-odm but as the problem confuses people regularly, it would be great to fix it. @eggyal if you want to look into it, lets open an issue at phpcr-odm and start with a PR with a failing test to confirm the problem is really what we think it is. i can then help with some pointers to figure out where the issue can be fixed.

wouterj commented 9 years ago

So, the solution here is to create a parent token provider, providing the referrer route of the parent document? Does this mean we have to solve it the same way as @petforsberg did?

It would be nice to include this in 1.1

thisispaul commented 7 years ago

Is this issue dead and the feature not being implemented?

dbu commented 7 years ago

nobody is currently working on it. it needs some digging through phpcr-odm but would be an interesting learning, if you want to give it a try...