Disable cache warmup when translator is identity translator #1965
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SanderVerkuil
changed the title
smnandre
reviewed
Jul 6, 2024
src/Translator/src/DependencyInjection/TranslatorCompilerPass.php
Outdated
Show resolved
Hide resolved
smnandre
reviewed
Jul 6, 2024
smnandre
reviewed
Jul 6, 2024
src/Translator/src/DependencyInjection/TranslatorCompilerPass.php
Outdated
Show resolved
Hide resolved
smnandre
reviewed
Jul 6, 2024
|
Thank you @SanderVerkuil ! Could you maybe add a line or two in the documentation ? |
|
@maelanleborgne @Kocal if you can just confirm, as you know more than me this package :) ? |
Done! Added a small note next to the cache warmup, feel free to modify it as you see fit, or if it's unclear I'd be happy to hear! |
smnandre
approved these changes
Jul 6, 2024
carsonbot
added
Status: Reviewed
Kocal
approved these changes
Jul 6, 2024
kbond
force-pushed
the
fix/1962/disable-cache-warmup-when-translator-is-identity-translator
branch
from
cef9ca6 to
b75e5d5
Compare
|
This is great, thanks Sander! |
Kocal
added a commit
to Kocal/symfony-ux
that referenced
this pull request
Aug 13, 2024
Kocal
added a commit
to Kocal/symfony-ux
that referenced
this pull request
Aug 13, 2024
| @@ -71,6 +71,10 @@ For a better developer experience, TypeScript types definitions are also generat | |||
| Then, you will be able to import those JavaScript translations in your assets. | |||
| Don't worry about your final bundle size, only the translations you use will be included in your final bundle, thanks to the `tree shaking <https://webpack.js.org/guides/tree-shaking/>`_. | |||
|
|
|||
| .. note:: | |||
|
|
|||
| This package requires the `translator` to be enabled in your Symfony application. If you don't use the `translator` service, the warmup command will not generate any translations. | |||
There was a problem hiding this comment.
This package requires the
translatorto be enabled
Is translator a package, a service of something else? In the next sentence it is written that it's a service, I think it should be explicit in the first sentence too.
There was a problem hiding this comment.
Code should have double quotes, otherwise it is displayed like this:
There was a problem hiding this comment.
Side note: This PR may be reverted soon:
There was a problem hiding this comment.
I've updated this note in #2061
SanderVerkuil
deleted the
fix/1962/disable-cache-warmup-when-translator-is-identity-translator
branch
Kocal
added a commit
to Kocal/symfony-ux
that referenced
this pull request
Aug 14, 2024
Kocal
added a commit
to Kocal/symfony-ux
that referenced
this pull request
Aug 14, 2024
kbond
added a commit
that referenced
this pull request
Aug 14, 2024
…fony applications (Kocal) This PR was merged into the 2.x branch. Discussion ---------- [Translator] Revert #1965, which break cache warmup for Symfony applications | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Issues | Fix #2056 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT <!-- Replace this notice by a description of your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - For new features, provide some code snippets to help understand usage. - Features and deprecations must be submitted against branch main. - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> Hi everyone! It looks like we were too fast on #1965, and our tests base didn't see the issue. With 2.19.0, people started to have issues with the CacheWarmer from UX Translator, which was... simply never called, and so the folder `var/translations` was not generated anymore. In private, we've decided to revert the feature to fix the issue, and to re-open the discussion if necessary. Commits ------- baf9f5e [Translator] Revert #1965, which break cache warmup for Symfony applications
kbond
added a commit
that referenced
this pull request
Aug 14, 2024
This PR was squashed before being merged into the 2.x branch. Discussion ---------- [CI] Test ux.symfony.com with local UX packages | Q | A | ------------- | --- | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT <!-- Replace this notice by a description of your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - For new features, provide some code snippets to help understand usage. - Features and deprecations must be submitted against branch main. - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> Following #1965 and #2060, I was a bit surprised to see ux.symfony.com tests were executed with UX packages coming from `2.x` branch, instead of UX packages from the same PR. I believe doing this could help us for the future and prevent some regressions. I've updated the script `.github/build-packages.php` to automatically replaces **all UX packages requires(-dev)** from all significant `composer.json` files from the repo: <img width="1270" alt="image" src="https://github.com/user-attachments/assets/e04ff73f-7558-46cc-91e2-bf49c26a7f04"> Commits ------- 1386749 [CI] Test ux.symfony.com with local UX packages
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the present
translatordoes not implement theTranslatorBaginterface, which is the case if the framework bundle is enabled and either the form, or the validator are enabled, removes the warmup phase.