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

Fix Duplicated Siteaccesses in Generated URL. #2371

Closed
wants to merge 1 commit into from

Conversation

zerustech
Copy link

@zerustech zerustech commented Jul 1, 2018

Assume two siteaccesses, eng and chi, have been configured to support
eng-US and chi-CN locales respectively. If an article at
/chi/news/article_1 only has chi-CN translation available, calling
path(ez_route(null, {language: 'eng-US'})) in the template would
output something like /eng/chi/view/content/..., which is incorrect
because the /chi part should not be there.

This issue occurs when UrlAliasGenerator has failed to generate the
URL alias for the specific article in the target language, as the
translation in the target language does not exist, so as the fallback
generator, the DefaultRouter will be used to generate the internal URL
for the article. However, by default, the DefaultRouter always
prepends the siteaccess, which is chi in this case, to the genreated
URL, and finally the UrlAliasGenerator will also prepend the target
siteaccess 'eng' to the URL.

To fix this issue, a new parameter $ignoreSiteAccess has been
introducted in argument $parameters of DefaultRouter::generate(), which indicates if the DefaultRouter should ignore the siteaccess from the start of the URL it generates.

Question Answer
JIRA issue EZP-29364
Bug yes
New feature yes
Target version 6.x/7.x
BC breaks no
Tests pass yes
Doc needed no

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@ezrobot

This comment has been minimized.

@zerustech zerustech changed the title Duplicated Siteaccesses in Generated URL. Fix Duplicated Siteaccesses in Generated URL. Jul 1, 2018
@ezrobot

This comment has been minimized.

1 similar comment
@ezrobot

This comment has been minimized.

@ezrobot

This comment has been minimized.

@ezrobot

This comment has been minimized.

1 similar comment
@ezrobot

This comment has been minimized.

@ezrobot

This comment has been minimized.

@andrerom
Copy link
Contributor

andrerom commented Jul 1, 2018

Hi @zerustech,

nice to see you again :)
We don't actually maintain 6.1/6.6 anymore, we maintain the LTS branches: 6.7 (1.7LTS) & 6.13 (1.13LTS), as well as latests Fast Track release which is now 7.2 (2.2) with security patching still on 7.1 (2.1) for another 2-3 months.

Maybe we should archive/delete the older branches to make that more clear.

Anyway, it might solve the php coding style errors you are getting here if you rebase or reopen on 6.7 for instance. There you will be able to run composer fix-cs if you have installed composer packages.

Best,
André

@zerustech zerustech changed the base branch from 6.1 to 6.13 July 1, 2018 22:49
@zerustech
Copy link
Author

zerustech commented Jul 1, 2018

Hi @andrerom,

Great to hear from you again :) Thanks for your feedback, I have fixed the cs issue and re-based this PR to 6.7 and I will re-base it to 6.13 and 7.x branches once it has been approved. In the meantime, I will close the other PR.

Kind regards,

Michael

@zerustech zerustech changed the base branch from 6.13 to 6.7 July 1, 2018 22:58
Copy link
Member

@bdunogier bdunogier left a comment

Choose a reason for hiding this comment

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

Hello Michael. Thanks a lot for the report and bugfix.

Have you considered using a key in $parameters for this ? It would avoid adding arguments to a method that implements a Symfony interface. We already use them to pass the siteaccess on in the same area.

A question: at which point does the UrlGenerator add the prefix again in the sequence ? I'd like to use something other than "ignore_siteaccess". It tells the router what to do. Ideally, it would provide it with an information, and the router would react by skipping the siteaccess prefix (ask, don't tell).

It works as it is, but trying to answer this point may help keep the architecture clear for the future.

Assume two siteaccesses, `eng` and `chi`, have been configured to support
`eng-US` and `chi-CN` locales respectively. If an article at
`/chi/news/article_1` only has `chi-CN` translation available, calling
`path(ez_route(null, {language: 'eng-US'}))` in the template would
output something like `/eng/chi/view/content/...`, which is incorrect
because the `/chi` part should not be there.

This issue occurs when `UrlAliasGenerator` has failed to generate the
URL alias for the specific article in the target language, as the
translation in the target language does not exist, so as the fallback
generator, the `DefaultRouter` will be used to generate the internal URL
for the article. However, by default, the `DefaultRouter` always
prepends the siteaccess, which is `chi` in this case, to the genreated
URL, and finally the `UrlAliasGenerator` will also prepend the target
siteaccess 'eng' to the URL.

To fix this issue, a new parameter 'ignoreSiteAccess' has been
introducted in `$parameters` of `DefaultRouter::generate()`, which
indicates if the `DefaultRouter` should ignore the stieaccess from the
start of the URL it generates.

| Question           | Answer
| ------------------ | ------------------
| **JIRA issue**     | [EZP-29364](https://jira.ez.no/browse/EZP-29364)
| **Bug**            | yes
| **New feature**    | yes
| **Target version** | `6.x`/`7.x`
| **BC breaks**      | no
| **Tests pass**     | yes
| **Doc needed**     | no

**TODO**:
- [ ] Implement feature / fix a bug.
- [ ] Implement tests.
- [ ] Fix new code according to Coding Standards (`$ composer fix-cs`).
- [x] Ask for Code Review.
@zerustech
Copy link
Author

Hi @bdunogier, Thanks for your feedback. Actually, it was my first choice as well, but I found the 'ignoreSiteAccess' parameter was exposed in the query string of the URL generated, so I implemented it with the extra argument. However, I just realized that it is safe to unset it from $parameters before the URL is generated :) So now the code has been changed as per your request.

@zerustech
Copy link
Author

Hi @bdunogier
Forgot to answer your second question. As for the cause of this issue, please see my comments below:

  1. I created a website with two site-accesses eng and chi, which have been configured to support
    eng-US and chi-CN locales respectively.

  2. I used the following code in twig to build the language bar:

{# language is the language code of the target site-access #}
{% set path = path(ez_route(null, {language: language})) %}
...
  1. There is an article /chi/news/article_1 that only has chi-CN translation available, and when I access it at /chi/news/article_1, the path generated for eng becomes something like /eng/chi/view/content/....

  2. This issue is caused by the UrlAliasGenerator failing to generate the URL alias for this article in the target language, 'eng-US' in this case, because the translation is not available at all; the DefaultRouter is then used as the fallback generator to generate internal URL for this article; but DefaultRouter always preprends the site-access, which is chi in this case, to the URL it generates, so the URL becomes /chi/view/content/... first; The generated URL is passed back to UrlAliasGenerator, which will then prepend the target site-access eng to the URL, so it finally becomes /eng/chi/view/content/...

As for the name of the parameter, please let me know when you have decided so that I can update the PR again.

Kind regards,

Michael

@adamwojs
Copy link
Member

Closing PR as obsolete. eZ Platform 2.5 has reached EOM, so please reopen PR in ezsystems/ezplatform-kernel (bug fixes) or ibexa/core (features/improvements) if issue is still valid for v3.3 / v4.x and you are willing to work on it.

@adamwojs adamwojs closed this Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants