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

Backport of i18n lookup table changes to 4.x, fixes misc url_for issues #2604

Merged
merged 4 commits into from
Feb 6, 2023

Conversation

guillerodriguez
Copy link
Contributor

Backport of #2263 (reimplementation of i18n lookup table) to 4.x branch, including a fix for issue #2594, which was introduced by the referenced PR.

Fixes #2280, fixes #2592.

Lookup table implementation backported from master. This solves
a bug where fully localized templates links can't be generated
by url_for and link_to helper methods.
Helper methods url_for and link_to could not get the correct paths
when mount config option was set to "false".

Fixed it by adding an unlocalized path to the lookup table.
New test to make sure "mount_at_root: false" works as intended,
includes previously failing case of fully localized templates
(e.g.: hello.en.html.erb).
@guillerodriguez guillerodriguez changed the title Fix/mount false lookup 4.x Backport of i18n lookup table changes to 4.x, fixes misc url_for issues Dec 20, 2022
@markets
Copy link
Member

markets commented Dec 20, 2022

Thanks for that @guillerodriguez! The current situation of MM (v4 vs v5) is a bit weird and confusing for contributors 🙏🏼

Last month I've discussed with @tdreyno and we came up with this plan: #2569 (comment).

@guillerodriguez
Copy link
Contributor Author

Thanks for that @guillerodriguez! The current situation of MM (v4 vs v5) is a bit weird and confusing for contributors 🙏🏼

Last month I've discussed with @tdreyno and we came up with this plan: #2569 (comment).

If this helps making MM move forward then it is welcome :-)

@tdreyno
Copy link
Member

tdreyno commented Feb 6, 2023

@guillerodriguez I can merge if you're able to resolve those broken tests

@guillerodriguez
Copy link
Contributor Author

@guillerodriguez I can merge if you're able to resolve those broken tests

@tdreyno Not sure these failures are actually related to the changes in the PR -- all jobs failed early, in the ruby/setup-ruby@v1 action. This is before actually attempting to run any tests...

@tdreyno
Copy link
Member

tdreyno commented Feb 6, 2023

Can you re-run the tests, might have been a GH issue. I don't have permissions to restart your actions

@guillerodriguez
Copy link
Contributor Author

Can you re-run the tests, might have been a GH issue. I don't have permissions to restart your actions

I don't think I can do that either (or can't find how), perhaps because it's been more than 30 days since the original run. I will try to update the branch to see if that triggers a re-run.

@guillerodriguez
Copy link
Contributor Author

guillerodriguez commented Feb 6, 2023

Can you re-run the tests, might have been a GH issue. I don't have permissions to restart your actions

I don't think I can do that either (or can't find how), perhaps because it's been more than 30 days since the original run. I will try to update the branch to see if that triggers a re-run.

Done. Now it says the workflow is "awaiting approval from a maintainer": https://github.com/middleman/middleman/actions/runs/4106777042

@guillerodriguez
Copy link
Contributor Author

Can you re-run the tests, might have been a GH issue. I don't have permissions to restart your actions

I don't think I can do that either (or can't find how), perhaps because it's been more than 30 days since the original run. I will try to update the branch to see if that triggers a re-run.

Done. Now it says the workflow is "awaiting approval from a maintainer": https://github.com/middleman/middleman/actions/runs/4106777042

Success :-)

@tdreyno tdreyno merged commit ae5581e into middleman:4.x Feb 6, 2023
@tdreyno
Copy link
Member

tdreyno commented Feb 6, 2023

Thank you 🙏🏻 @guillerodriguez

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.

None yet

4 participants