IBX-862: Fixed translation action originating from non-main Location #1845
Conversation
There was a problem hiding this comment.
During merge-up, this code https://github.com/ezsystems/ezplatform-admin-ui/blob/2.3/src/bundle/Controller/ContentEditController.php#L45 should be updated too
|
@mikadamczyk might the locationId also be added here https://github.com/ezsystems/ezplatform-admin-ui/blob/2.3/src/bundle/Controller/TranslationController.php#L80 ? |
| @@ -564,7 +564,7 @@ ezplatform.content.preview: | |||
| locationId: ~ | |||
|
|
|||
| ezplatform.content.translate: | |||
| path: /content/{contentId}/translate/{toLanguageCode}/{fromLanguageCode} | |||
| path: /content/{contentId}/{locationId}/translate/{toLanguageCode}/{fromLanguageCode} | |||
There was a problem hiding this comment.
Hmm, I don't remember if we gave BC promise on route parameters. If so it's a BC break, if not, then it's fine
POV ping @ezsystems/php-dev-team
There was a problem hiding this comment.
I am sure we do (or at least did in the past ;) ), as we also did not rename controllers method parameters because of that. Thats why it should have default value and same behavior as current with it.
There was a problem hiding this comment.
@alongosz, @ViniTou in this case we won't be able to fix this one as we need the
locationIdin order for the proper location to be loaded when translating. Right now this is a decision if we want to keep the BC or fix this.
@barw4 Usually I'd encourage you to find on your own a solution which provides both, but since this is CSR, let me just serve it.
There are 2 ways of providing BC for the route itself:
A) New route:
- Create different route
- Use different route where needed
- If the old route is not needed everywhere, deprecate (if no route deprecation is available, simple place a deprecating comment)
B) Default parameter
- Place
{locationId}at the end instead - Provide default value via
defaultsroute attribute
Question: I don't see consumption changes, only passing the parameter. Where is it used?
There was a problem hiding this comment.
There was a problem hiding this comment.
The default parameter option was not possible as we had the other default parameter at the end.
| @@ -564,7 +564,7 @@ ezplatform.content.preview: | |||
| locationId: ~ | |||
|
|
|||
| ezplatform.content.translate: | |||
| path: /content/{contentId}/translate/{toLanguageCode}/{fromLanguageCode} | |||
| path: /content/{contentId}/{locationId}/translate/{toLanguageCode}/{fromLanguageCode} | |||
There was a problem hiding this comment.
@alongosz, @ViniTou in this case we won't be able to fix this one as we need the
locationIdin order for the proper location to be loaded when translating. Right now this is a decision if we want to keep the BC or fix this.
@barw4 Usually I'd encourage you to find on your own a solution which provides both, but since this is CSR, let me just serve it.
There are 2 ways of providing BC for the route itself:
A) New route:
- Create different route
- Use different route where needed
- If the old route is not needed everywhere, deprecate (if no route deprecation is available, simple place a deprecating comment)
B) Default parameter
- Place
{locationId}at the end instead - Provide default value via
defaultsroute attribute
Question: I don't see consumption changes, only passing the parameter. Where is it used?
alongosz
changed the title
| path: /content/{contentId}/location/{locationId}/translate/{toLanguageCode}/{fromLanguageCode} | ||
| methods: ['GET', 'POST'] | ||
| defaults: | ||
| _controller: 'EzPlatformAdminUiBundle:ContentEdit:translate' |
There was a problem hiding this comment.
Isn't this a legacy way of referencing controllers?
Using FQCN is recommended afaik.
|
@barw4 Could you please apply #1845 (comment) suggestion before merge ? |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Could you merge it up @barw4 ? |
The
locationIdparameter has been added to translate action in order for redirect after the translation to work properly.Checklist:
$ composer fix-cs)