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

[FrameworkBundle][Translation] Add support for Translator paths, Twig paths and Translator aware services paths in commands #29121

Merged
merged 1 commit into from Feb 13, 2019

Conversation

Projects
None yet
6 participants
@yceruto
Copy link
Contributor

yceruto commented Nov 7, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29085, #29633, #17739
License MIT
Doc PR TODO

Add custom (also common) Twig and Translation paths to the translation commands:

  • Custom directories configured in twig.paths.
  • Custom directories configured in translator.paths
  • The Resources/translations/ directory of Validation component (if installed).
  • The Resources/translations/ directory of Form component (if installed).
  • The Resources/translations/ directory of Security Core component (if installed).

@yceruto yceruto force-pushed the yceruto:deprecating_bundle_argument branch 4 times, most recently from 6077362 to 533d4d4 Nov 7, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor Author

yceruto commented Nov 7, 2018

Update: Extracting translation messages from services (e.g. controllers and other PHP files) that have the translator injected into them by using the following service injection methods: (properties, bindings, construct arguments and method calls).

BUT they're other methods that I can't to reach (e.g. the service locator related to: services subscriber and controller service arguments) I need help to accomplish it, maybe @nicolas-grekas ?

@yceruto yceruto force-pushed the yceruto:deprecating_bundle_argument branch from ceb615f to 46fc19b Nov 8, 2018

fabpot added a commit that referenced this pull request Nov 8, 2018

bug #29128 [FrameworkBundle] Cleaning translation commands and fix a …
…bug for --all option (yceruto)

This PR was squashed before being merged into the 4.2-dev branch (closes #29128).

Discussion
----------

[FrameworkBundle] Cleaning translation commands and fix a bug for --all option

| Q             | A
| ------------- | ---
| Branch?       | master (4.2)
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29121 (comment)
| License       | MIT
| Doc PR        | -

Commits
-------

dfc7dcc Also fix a bug for --all option
3cbefd8 Cleaning translation commands

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 8, 2018

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

we should maybe split the translator propagation in another pass.
I'd suggest to look at the HotPathPass also: we could looks at services that has the translation injected transitively. And that would give you a hint about following locators btw (HotPathPass doesn't follow them but here we would - see instanceof ArgumentInterface + its getValue method).

@yceruto yceruto force-pushed the yceruto:deprecating_bundle_argument branch 2 times, most recently from 8004646 to 9fe9c2d Nov 16, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor Author

yceruto commented Nov 16, 2018

Status: Needs work

@yceruto yceruto force-pushed the yceruto:deprecating_bundle_argument branch 9 times, most recently from 5d56766 to 555a562 Nov 16, 2018

@yceruto yceruto force-pushed the yceruto:deprecating_bundle_argument branch 4 times, most recently from 6a12280 to e57b804 Jan 24, 2019

@yceruto

This comment has been minimized.

Copy link
Contributor Author

yceruto commented Jan 24, 2019

Yay! funcional tests passed!

@nicolas-grekas this is ready on my side, how does it look for you?

@yceruto yceruto force-pushed the yceruto:deprecating_bundle_argument branch 2 times, most recently from 7b2afe6 to b37ed12 Jan 25, 2019

@yceruto

This comment has been minimized.

Copy link
Contributor Author

yceruto commented Jan 31, 2019

Friendly ping.

@yceruto

This comment has been minimized.

Copy link
Contributor Author

yceruto commented Feb 6, 2019

Maybe I should split this PR into two for an easier review? I mean, move the last point that contains a little more complexity:

  • All PHP files from services "translator aware" via constructor argument, public property, method calls, service subscriber, controller argument)
@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Feb 8, 2019

@yceruto I think that could be helpful

@yceruto yceruto force-pushed the yceruto:deprecating_bundle_argument branch from b37ed12 to 0383072 Feb 8, 2019

@yceruto

This comment has been minimized.

Copy link
Contributor Author

yceruto commented Feb 8, 2019

@xabbuh working on it, thanks for your reply.

Status: Needs work

@yceruto yceruto force-pushed the yceruto:deprecating_bundle_argument branch 2 times, most recently from 598b3d4 to 31d7a09 Feb 8, 2019

@yceruto

This comment has been minimized.

Copy link
Contributor Author

yceruto commented Feb 8, 2019

Update: I've divided the complexity of this PR as I mentioned earlier, see the updated description.

Status: Needs review

@yceruto

This comment has been minimized.

Copy link
Contributor Author

yceruto commented Feb 8, 2019

Done #30120.

@fabpot

fabpot approved these changes Feb 13, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 13, 2019

Thank you @yceruto.

@fabpot fabpot merged commit 31d7a09 into symfony:master Feb 13, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 13, 2019

feature #29121 [FrameworkBundle][Translation] Add support for Transla…
…tor paths, Twig paths and Translator aware services paths in commands (yceruto)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[FrameworkBundle][Translation] Add support for Translator paths, Twig paths and Translator aware services paths in commands

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29085, #29633, #17739
| License       | MIT
| Doc PR        | TODO

Add custom (also common) Twig and Translation paths to the translation commands:
 * Custom directories configured in `twig.paths`.
 * Custom directories configured in `translator.paths`
 * The `Resources/translations/` directory of `Validation` component (if installed).
 * The `Resources/translations/` directory of `Form` component (if installed).
 * The `Resources/translations/` directory of Security Core component (if installed).

Commits
-------

31d7a09 Add support for translator paths and twig paths in translation commands

@yceruto yceruto deleted the yceruto:deprecating_bundle_argument branch Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment