Skip to content

[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

Closed
wants to merge 3 commits into from

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented May 5, 2015

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.

@Seldaek Seldaek added Translation FrameworkBundle DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels May 5, 2015
@aitboudad
Copy link
Contributor

@Seldaek see #14399

@Seldaek
Copy link
Member Author

Seldaek commented May 5, 2015

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 :)

@aitboudad
Copy link
Contributor

😄 well I closed my own PR, what do you think about adding priority ?

@weaverryan
Copy link
Member

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()
Copy link
Member

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?

Copy link
Member Author

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.

@Seldaek
Copy link
Member Author

Seldaek commented May 6, 2015

@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 :)

@xabbuh
Copy link
Member

xabbuh commented May 6, 2015

@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())
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, fixing

@weaverryan
Copy link
Member

@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.

@fabpot
Copy link
Member

fabpot commented May 6, 2015

Ready to be merged?

@Seldaek
Copy link
Member Author

Seldaek commented May 6, 2015

To me yes. It doesn't prevent further improvements as were discussed above.

@aitboudad
Copy link
Contributor

👍

@aitboudad aitboudad added the Ready label May 6, 2015
@weaverryan
Copy link
Member

Yep - my comments should not stop merging at all 👍 from me - it looks very simple.

@aitboudad
Copy link
Contributor

Thank you @Seldaek.

aitboudad added a commit that referenced this pull request May 6, 2015
… 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
@aitboudad aitboudad closed this May 6, 2015
@aitboudad
Copy link
Contributor

Doc issue symfony/symfony-docs#5236

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) FrameworkBundle Ready Translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants