-
Notifications
You must be signed in to change notification settings - Fork 29
Enable the ContentObject provider to work with translated referrers #63
Conversation
if ($referrer instanceof AutoRoute && $referrer->getContent() === $object) { | ||
|
||
// filter the referrering routes by locale | ||
if ($referrer->getLocale() == $context->getLocale()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this does not support fallback yet, it should do before this PR is done.
for translated urls, i think there are 2 viable strategies:
ideally, we could support both, if not we should clearly explain which one we do, and maybe explain in the doc what the approach would be to do the other approach. |
@@ -91,7 +106,7 @@ public function providePath(RouteStack $routeStack) | |||
|
|||
if (count($routes) > 1) { | |||
throw new \RuntimeException( | |||
'Multiple referring routes (i.e. translations) not supported yet.' | |||
'Found more than one referring auto route, this should not happen.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this not go into a logfile, at a high level? an exception could break a running website, which is probably too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it depends on if this would ever happen, and under what circumstances it would happen. It shouldn't happen I think, so I would prefer to bail rather than risk the website acting in unexpected ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
Just to recap, the problem here is how to, when a content is persisted in a new locale, ensure that the As @dbu says we could persist in all the locales each time, but I think this would be inflexible, I am not sure I understand his second point. For me the optimum would be to explicitly reprocess the content object when the new locale is persisted. Note also that this is only necessary because a content object might have a dynamic "locale" element - e.g. |
Hmm. this is tricky. The easiest way to accomplish this would indeed to persist all the locales each time - this would just be modifying a line of code, at quite a cost to write performance. But really, all of this is necessary simply so that if the Locale provider is used in a related content object, then it will show "/en/english-category/article-francais" instead of "/fr/english-category/article-francais". So I wonder if we shouldn't be just doing some kind of token replacement instead. |
ok. Had a good think about this. I think the best thing to do would simply be to remove the If we take a use case of having a I think this is better. So remove |
i am not 100% sure to understand everything, but if we find a way to simplify things whitout making everybody having to write the same code all the time, it would be a good thing to remove unneeded things. |
Will be fixed by #72 |
This PR is to make the
ContentObjectProvider
multilang aware. So that when aContentObject
provider is used in the chain, and a translated content object is persisted, the route provided by theContentObject
provider will correspond to locale of the persisted content object.Note that this PR currently only introduces basic support, where by
However persisting an article in
es
would require persisting a newAutoRoute
for category with the localees
, which I am not sure how best to do.The default behavior when persisting a content which has a ContentObject provider providing an object not available in the locale of the content should be to take the fallback. This would result in an inaccurate prefix, e.g.
/en/category/ceci-est-une-article-francais
but well, voilà.