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

[Translator] Add configuration to filter dumped translations by domain #1930

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

maelanleborgne
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Issues Fix #1225
License MIT

Add two config entries included_domains and excluded_domains to filter which translation domains should be dumped.
If included_domains is empty, it is not taken into account.

TODO:

  • Add entry to changelog

Copy link
Collaborator

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and splitting #1927 into two PR, that was a lot easier to review! :)

src/Translator/doc/index.rst Outdated Show resolved Hide resolved
src/Translator/src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
src/Translator/src/TranslationsDumper.php Outdated Show resolved Hide resolved
}
if (0 !== \count($this->includedDomains) && !\in_array($domain, $this->includedDomains, true)) {
continue;
}
foreach ($catalogue->all($domain) as $id => $message) {
$realDomain = $catalogue->has($id, $domain.MessageCatalogueInterface::INTL_DOMAIN_SUFFIX)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe leverage CatalogueMetadataAwareInterface here ?

@maelanleborgne maelanleborgne force-pushed the feature/translator-filter-domains branch from d96b0f7 to d66804a Compare June 21, 2024 08:22
@maelanleborgne maelanleborgne force-pushed the feature/translator-filter-domains branch from 9d6f736 to e41a908 Compare June 24, 2024 07:22
@smnandre
Copy link
Collaborator

So were are we there ? :) Do you need help for something ? Is the doc ready ?

@maelanleborgne
Copy link
Contributor Author

So were are we there ? :) Do you need help for something ? Is the doc ready ?

@smnandre Yes that should be it, the doc as well. I'm just not sure what version I should use in the changelog entry.

@smnandre
Copy link
Collaborator

2.19 no ?

@maelanleborgne maelanleborgne force-pushed the feature/translator-filter-domains branch from 47a7009 to 3135d1e Compare June 26, 2024 11:27
@smnandre
Copy link
Collaborator

The failing test surprises me a bit ... @WebMamba any idea ? I feel we may have exposed more than we wanted

@WebMamba
Copy link
Collaborator

Looks like this is related to the PR: #1940.

Hummm the test looks flaky. I don't think this is related to this PR though

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Looks good, just questioning the config choice

src/Translator/doc/index.rst Outdated Show resolved Hide resolved
@maelanleborgne maelanleborgne force-pushed the feature/translator-filter-domains branch from 18a0ad4 to f36d05d Compare July 9, 2024 13:56
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jul 9, 2024
@kbond kbond force-pushed the feature/translator-filter-domains branch from f36d05d to 2b56e4a Compare July 18, 2024 12:57
@kbond
Copy link
Member

kbond commented Jul 18, 2024

This is great @maelanleborgne, thank you!

@kbond kbond merged commit 33a95e9 into symfony:2.x Jul 18, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Translator] Filter dumped domains
6 participants