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

Make sure to add hreflang on localized pages. #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nyholm
Copy link
Contributor

@Nyholm Nyholm commented Aug 31, 2015

This introduces a twig function hreflang that will print <link rel="alternate" hreflang="es" href="http://es.example.com/" /> on all pages that are localized.

I do also remove calls to deprecated functions.

@stof
Copy link

stof commented Aug 31, 2015

replacing deprecated calls should be sent in a separate PR rather than being mixed with a new feature. Replacing deprecated calls probably requires less discussions than a new feature, and mixing both makes it harder to review the new feature

* @param RequestStack $requestStack
* @param array $locales
*/
public function __construct(RequestStack $requestStack, $locales)
Copy link

Choose a reason for hiding this comment

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

RequestStack is not supported on the Symfony 2.3 LTS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, you are correct. Should I get the request from the TwigEnvironment instead?

@acasademont
Copy link
Collaborator

Hi @Nyholm as @stof suggested would be better to only have a PR with the new feature. Can you rebase with master?

@Nyholm
Copy link
Contributor Author

Nyholm commented Dec 3, 2015

Thank you @acasademont for taking time to review this. I have now rebased the code.

What do you think about using the twig environment instead of injecting the RequestStack?

@XWB
Copy link
Contributor

XWB commented Dec 4, 2015

@Nyholm You need to fix the tests as well.

@Nyholm
Copy link
Contributor Author

Nyholm commented Dec 5, 2015

I've updated this PR. It supports SF2.3 and the tests are fixed.
@acasademont, is this ready to be merged?

@XWB
Copy link
Contributor

XWB commented Dec 7, 2015

@Nyholm Have you also checked the i18n_locales routing option? Imagine the following setup:

jms_i18n_routing:
    locales: [nl, en, fr]
not_french:
    path: /not-french
    options: { i18n_locales: [nl, en] }

This should ignore the fr locale and only generate <link rel="alternate" hreflang="nl" href="http://nl.example.com/" /> and ```.

@XWB
Copy link
Contributor

XWB commented Dec 7, 2015

You also need to add twig to composer.json.

@Nyholm
Copy link
Contributor Author

Nyholm commented Dec 7, 2015

Thank you @XWB for your feedback. I had not considered the scenario where you do not use all locales.

I have rewritten the PR according to your comments.

@picks44
Copy link

picks44 commented Jan 17, 2017

Hi @Nyholm @stof @XWB, how can we use this function? I see nothing in the doc... Also, is there a possibility to get all locale used in the config? SO we can use this in a controller to create a language switcher for instance, without having to duplicate all language entries...

@picks44
Copy link

picks44 commented Apr 12, 2017

Up ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants