-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle][DX] Add option to specify additional translation loading paths #14561
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
Oh fail I did not see that.. I'd argue mine wins for testing it works and not just testing it fails though ;) Also my version adds the paths at the best location IMO which is over the symfony translations but under the application ones. Anyway I'll leave open and leave our dear @symfony/deciders decide because I spent enough time on this as it is :) |
😄 well I closed my own PR, what do you think about adding priority ? |
I definitely support this idea. But, even further, (and please don't let this get off-topic - this comment is off-topic enough), I wish resource paths were registered in a way more like how this change allows - i.e. bundles register their own translation paths, instead of having the core look at every bundle and register paths for them. I think that adds to the "magic" of Symfony - a lot is automatic when you use a bundle. I experimented with an approach for templates in https://github.com/symfony/symfony/pull/13928/files#diff-40488c4aeae121d3032bbe13e2f5801eR26 where bundles register template paths, instead in core. Thanks! |
->prototype('scalar')->end() | ||
->defaultValue(array()) | ||
->end() | ||
->end() |
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.
What about merging this node and the fallbacks
node into one single children()
block?
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.
Yeah that's my bad I'll fix.
@aitboudad not sure if we need such a complex system that priority is necessary or not. I mean the need doesn't seem to have come up until now so I doubt it's a real problem. Usually you just want to override stuff from third parties in your app and that's it. @weaverryan sounds like a good improvement for another PR yeah but I know I won't have time to pull it off :) |
@weaverryan @Seldaek Sounds like a use case for Puli. :) /cc @webmozart |
@@ -582,6 +583,10 @@ private function addTranslatorSection(ArrayNodeDefinition $rootNode) | |||
->defaultValue(array('en')) | |||
->end() | |||
->booleanNode('logging')->defaultValue($this->debug)->end() | |||
->arrayNode('paths') | |||
->prototype('scalar')->end() | |||
->defaultValue(array()) |
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.
the defaultValue is useless here. It is already the case for a prototyped node
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.
Right, fixing
@xabbuh I agree (at least in theory - I've talked with Bernhard on Skype about Puli), but it's a very complex problem, especially when you think about integrating it with Symfony and keeping BC as much as possible. Right now, I'm interested in what our ideal API for setting paths to translations, views, etc would look like. When that's more clear, we should see if Puli can fill in for a lot of the work that would need to make this happen. |
Ready to be merged? |
To me yes. It doesn't prevent further improvements as were discussed above. |
👍 |
Yep - my comments should not stop merging at all 👍 from me - it looks very simple. |
Thank you @Seldaek. |
… translation loading paths (Seldaek) This PR was squashed before being merged into the 2.8 branch (closes #14561). Discussion ---------- [FrameworkBundle][DX] Add option to specify additional translation loading paths | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #971 | License | MIT | Doc PR | TODO Thanks to this PR, next time I want to just do: ``` translator: fallback: "%locale%" paths: ['%kernel.root_dir%/../vendor/internal/package/translations'] ``` I will be able to, and won't have to dig in docs, start building a translation loader before I realize it's not what I want, dig in the framework ext, and then have to write a compiler pass to inject my file to the translator resource paths. Commits ------- bba0a25 [FrameworkBundle][DX] Add option to specify additional translation loading paths
Doc issue symfony/symfony-docs#5236 |
Thanks to this PR, next time I want to just do:
I will be able to, and won't have to dig in docs, start building a translation loader before I realize it's not what I want, dig in the framework ext, and then have to write a compiler pass to inject my file to the translator resource paths.