Skip to content
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

Use choice translation domain #419

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artursvonda
Copy link
Contributor

@artursvonda artursvonda commented Dec 5, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes (relevant)
Fixed tickets 415
License Apache2

Description

Considers choice translation domain when extracting translation strings. Also ignores translates that have false as translation domain.

Todos

  • Tests
  • Documentation
  • Changelog

@artursvonda
Copy link
Contributor Author

Wasn't sure if this qualifies as BC break since extractor previously didn't take into account choice_translation_domain which will cause translations to be extracted under different (correct) domain now. Also will correctly skip translations with false translation domain.

@electrixx90
Copy link

When it becomes available this feature? I hope soon...

@gnat42
Copy link
Collaborator

gnat42 commented Apr 20, 2017

the other issue with this change is that the choice_translation_domain doesn't exist as a valid option for sf 2.3 which for whatever reason this bundle still supports even though 2.3 is no longer supported. We would have to drop support for sf2.3 with this.

@artursvonda
Copy link
Contributor Author

I don't think we need to drop the support for 2.3. The translation domain falls back to previous behaviour of using translation_domain if choice domain is not available. That is backwards compatible.

@gnat42
Copy link
Collaborator

gnat42 commented Apr 20, 2017

Well the tests will be trying to pass an option that doesn't exist in 2.3. So if we don't drop 2.3 support we need to then only conditionally run those tests when sf is > 2.3.

@artursvonda
Copy link
Contributor Author

I don't think test forms are actually executed so it shouldn't matter. And if users try to use the option on the forms, Symfony will inform them that the option doesn't exist.

@gnat42
Copy link
Collaborator

gnat42 commented Apr 20, 2017

Can you rebase this on master please?

@artursvonda artursvonda force-pushed the use-choice-translation-domain branch from 7dce4f0 to fdc8f32 Compare April 20, 2017 18:47
@artursvonda
Copy link
Contributor Author

Done :)

@gnat42
Copy link
Collaborator

gnat42 commented Apr 20, 2017

Can you review the test failures and see if you can fix/determine the issue?

@artursvonda
Copy link
Contributor Author

Updated tests and rebased. No idea why that one test is failing though.

@yellow1912
Copy link

This is a very old pull request but it has some important improvements that can be considered. For example, it seems like in the current version the translation_domain and choice_translation_domain when set to false still trigger the extraction. This is in conflict with Symfony's document which clearly states that false mean no translation.

@artursvonda artursvonda force-pushed the use-choice-translation-domain branch from 5ff05e2 to 2aa7af9 Compare April 11, 2021 12:47
@artursvonda
Copy link
Contributor Author

Rebased against latest develop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants