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

Composite component does not reflect locale changes #5160

Closed
cristof opened this issue Oct 30, 2022 · 8 comments · Fixed by #5161
Closed

Composite component does not reflect locale changes #5160

cristof opened this issue Oct 30, 2022 · 8 comments · Fixed by #5161

Comments

@cristof
Copy link
Contributor

cristof commented Oct 30, 2022

Bug

According to VDL documentation (2.3, 3.0, and 4.0), "[t]he normal localization rules for ResourceBundle would apply" for composite components localized with <composite_component_name>.properties files that uses #{cc.resourceBundleMap}. As such, when using a composite component named custom_button.xhtml and its associated custom_button.properties and custom_button_fr.properties, the change in locale definition should change the displayed messages.

In mojarra and myfaces (2.3, 2.3-next (myfaces), 3.0 and 4.0), the expected behaviour does not occur (as recognized by BalusC in 2013.) His proposed hack still works for MyFaces, but I couldn't use it in recent versions of mojarra.

To reproduce

Example project localized-components and localized-web are available to show the issue. Please install the first one into your local maven and then compile the second.
You may deploy localized-web into a tomee-plume versions 8.0.13 (profile jakarta8) and 9.0.0-MX (profile jakarta9) and check the behavior. Go to localhost:8080/localized-web and change the language.

Expected behavior

The button label should change reflecting the desired language, but it doesn't.

Solution

I'm opening three pull requests that fixes the issue for branches 2.3.18, 3.0.2 and 4.0

@cristof cristof changed the title Composite component does not reflec locale changes Composite component does not reflect locale changes Oct 30, 2022
@BalusC
Copy link
Contributor

BalusC commented Oct 31, 2022

Quick review: I noticed that the getLocalizedProperties() method was copypasted into both ClasspathResourceHelper and WebappResourceHelper, which both extend from ResourceHelper. Can you please avoid code duplication and move up the method into ResourceHelper as a protected method? See also https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

@cristof
Copy link
Contributor Author

cristof commented Oct 31, 2022

Moved the method as requested in all branches, changing the name to getLocalizedPaths because it will return a list of a single path even if the target path is not a properties resource - so it will be the one and only localized path.

@cristof
Copy link
Contributor Author

cristof commented Oct 31, 2022

Changed localized-web project so it is easier to test the issue and the fix. Added readme with instructions there. As this is my first contribution to mojarra, I'm struggling on how I could add a test into the project.

@BalusC
Copy link
Contributor

BalusC commented Nov 12, 2022

Tests should be added in one of the faces* subfolders here https://github.com/jakartaee/faces/tree/master/tck

E.g.:
If you've fixed it in 2.3 (and inherently also 3.0/4.0), then add to faces23, it'll also be tested against 3.0 and 4.0.
If you've fixed it in 4.0 only, then add to faces40, it won't be tested against 2.3 and 3.0.

In this specific case, add to faces23.

@cristof
Copy link
Contributor Author

cristof commented Nov 13, 2022

Do I have to open another issue there, or I can use this one as reference for pull requests?

@melloware
Copy link
Contributor

@cristof i think you are safe to refernence this issue and also the MyFaces issue you opened in the TCK test. It would be a great addition since then MyFaces and Mojarra would have to pass the test!

@cristof
Copy link
Contributor Author

cristof commented Nov 15, 2022

Created the TCK test and made the pull request, as previously asked.

@melloware
Copy link
Contributor

Nice work!

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 a pull request may close this issue.

3 participants