-
-
Notifications
You must be signed in to change notification settings - Fork 399
[Translator] Refactor TranslationsDumper options from __constructor and setters, to dump method
#3244
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
Conversation
a06c07a to
4addfcb
Compare
TranslationsDumper options from __constructor to dump methodTranslationsDumper options from __constructor and setters, to dump method
4addfcb to
fbd61c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the TranslationsDumper class by moving configuration options (dump directory, TypeScript flag, and domain filters) from constructor parameters and setter methods into parameters of the dump() method itself. This simplifies the API by eliminating hidden state and making the configuration explicit at the point of use.
Key Changes
- Removed
$dumpDirand$dumpTypeScriptfromTranslationsDumperconstructor; now passed todump()method - Removed
addIncludedDomain()andaddExcludedDomain()setter methods; replaced with array parameters indump()method - Updated
TranslationsCacheWarmerto store configuration and pass it todump()method - Updated dependency injection configuration to wire arguments into
TranslationsCacheWarmerinstead ofTranslationsDumper
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Translator/src/TranslationsDumper.php |
Refactored constructor and dump method to accept configuration as method parameters instead of constructor/setter state |
src/Translator/src/CacheWarmer/TranslationsCacheWarmer.php |
Added configuration parameters to constructor and passes them to dump method |
src/Translator/src/DependencyInjection/UxTranslatorExtension.php |
Updated to wire configuration arguments into cache warmer instead of dumper service |
src/Translator/config/services.php |
Updated service definitions to reflect new constructor signatures |
src/Translator/tests/TranslationsDumperTest.php |
Updated all tests to use new dump method signature with named parameters |
src/Translator/tests/CacheWarmer/TranslationsCacheWarmerTest.php |
Updated mock expectations to match new dump method signature |
src/Translator/CHANGELOG.md |
Added breaking change entry for this refactoring |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @internal | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was initially missing
fbd61c7 to
787e493
Compare
…r` and setters, to `dump` method
787e493 to
cbed37a
Compare
This PR simplifies how
TranslationsDumperworks by moving all configuration options from the constructor/setters into thedump()method itself.Even if using the
TranslationsDumperdirectly is not a common use case (most users will the default behavior with the cache warmer), this change improves the overall design of the class:Promise, this is the last BC for Translator 🙏🏻