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

Added Symfony translation component #1466

Merged
merged 10 commits into from
Aug 30, 2023
Merged

Conversation

Vainonen
Copy link
Contributor

@Vainonen Vainonen commented May 31, 2023

Reasons for creating this PR

Link to relevant issue(s), if any

Description of the changes in this PR

Known problems or uncertainties in this PR

  • Symfony Translator service is given only one locale in constructor. Translator object is now created for every language. It seems to be possible to set and download locales afterwards to same Translator, so translation could be done more effectively.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@Vainonen Vainonen added this to the 3.0 milestone May 31, 2023
@Vainonen Vainonen self-assigned this May 31, 2023
@osma
Copy link
Member

osma commented May 31, 2023

CI tests are failing due to minor PHP code style violations. You should correct these, e.g. by running php-cs-fixer. See here for details: https://github.com/NatLibFi/Skosmos/blob/skosmos-3/CONTRIBUTING.md#code-style

@osma
Copy link
Member

osma commented May 31, 2023

Now that the gettext function is no longer used in PHP code, could you drop gettext from the list of PHP extensions installed for running the tests under GitHub Actions? I.e. the gettext part should be dropped from these two lines:

php_extensions: gettext intl xsl pcov

php_extensions: gettext intl xsl pcov

This should ensure that there are no more remaining uses of gettext functions in the PHP code base, at least for the part of the code that is covered by PHPUnit tests.

@sonarcloud
Copy link

sonarcloud bot commented Jun 1, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 18 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Patch coverage: 70.58% and project coverage change: +6.16% 🎉

Comparison is base (f63e5fb) 63.89% compared to head (fff56d0) 70.05%.
Report is 40 commits behind head on skosmos-3.

Additional details and impacted files
@@               Coverage Diff               @@
##             skosmos-3    #1466      +/-   ##
===============================================
+ Coverage        63.89%   70.05%   +6.16%     
- Complexity        1634     1641       +7     
===============================================
  Files               32       32              
  Lines             4290     4301      +11     
===============================================
+ Hits              2741     3013     +272     
+ Misses            1549     1288     -261     
Files Changed Coverage Δ
src/controller/WebController.php 20.38% <30.00%> (+0.51%) ⬆️
src/controller/RestController.php 22.66% <36.36%> (+6.28%) ⬆️
src/model/Vocabulary.php 85.71% <50.00%> (ø)
src/model/ConceptPropertyValue.php 86.58% <66.66%> (+18.29%) ⬆️
src/model/Concept.php 83.49% <83.33%> (+6.65%) ⬆️
src/model/Model.php 81.53% <93.75%> (+1.53%) ⬆️
src/controller/Controller.php 54.62% <100.00%> (-2.75%) ⬇️
src/model/BaseConfig.php 66.66% <100.00%> (-4.77%) ⬇️
src/model/ConceptMappingPropertyValue.php 84.87% <100.00%> (+66.38%) ⬆️
src/model/ConceptProperty.php 96.61% <100.00%> (+6.95%) ⬆️
... and 3 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Vainonen Vainonen requested a review from osma August 30, 2023 07:09
@Vainonen
Copy link
Contributor Author

Environment variables LANGUAGE and LC_ALL are no longer used, but are still set in dockerfile. Setting locale and languages were removed (the lines below) from tests and did not cause tests to fail:

putenv("LANGUAGE=en_GB.utf8");
putenv("LC_ALL=en_GB.utf8");
setlocale(LC_ALL, 'en_GB.utf8');
bindtextdomain('skosmos', 'resource/translations');
bind_textdomain_codeset('skosmos', 'UTF-8');
textdomain('skosmos');

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

Looks good in general. I gave a few comments:

  1. I don't think the Controller should have its own translator, it should use the one in the Model instead
  2. Some tests were marked as skipped in this PR and others have been skipped earlier because gettext translations didn't work. Those skips should now be removed if possible.
  3. A few minor style issues (mainly commented out lines that should be removed instead)

src/controller/Controller.php Outdated Show resolved Hide resolved
src/controller/WebController.php Outdated Show resolved Hide resolved
src/controller/WebController.php Outdated Show resolved Hide resolved
src/controller/WebController.php Outdated Show resolved Hide resolved
src/controller/WebController.php Outdated Show resolved Hide resolved
src/controller/WebController.php Outdated Show resolved Hide resolved
src/controller/WebController.php Outdated Show resolved Hide resolved
tests/ConceptPropertyValueLiteralTest.php Outdated Show resolved Hide resolved
tests/ConceptTest.php Outdated Show resolved Hide resolved
@Vainonen Vainonen marked this pull request as ready for review August 30, 2023 09:13
@sonarcloud
Copy link

sonarcloud bot commented Aug 30, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 20 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

I fixed a fragile test in a commit. Now I think this is good enough for merging!

There are still many strings in Twig templates that need to be translated using the new syntax, but I think those are best done in follow-up PRs.

@Vainonen
Copy link
Contributor Author

Thank you for review @osma

@Vainonen Vainonen merged commit 7cb3769 into skosmos-3 Aug 30, 2023
7 of 10 checks passed
@Vainonen Vainonen deleted the add-translation-component branch August 30, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

2 participants