Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Translation] Allow usage of Provider domains if possible#45171

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@welcoMattic
Copy link
Member

@welcoMatticwelcoMattic commentedJan 25, 2022
edited
Loading

QA
Branch?5.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets
LicenseMIT
Doc PR

This PR adds possibility fortranslation:push command to use the configured domains value for a provider, if there is no--domains option provided.

Before that, the command tries to get the domains from theMessageCatalog of all local translations (includingvalidators andsecurity even if the user doesn't override these files).

Now, we can configuredomains intranslation.yaml:

framework:default_locale:entranslator:default_path:'%kernel.project_dir%/translations'fallbacks:            -enproviders:lokalise:dsn:'%env(LOKALISE_DSN)%'locales:['de', 'en', 'es', 'fr', 'it', 'pl']domains:['messages']

Andbin/console translation:push lokalise will take['messages'] as domains value, instead of['messages', 'validators', 'security'].

@carsonbot
Copy link

Hey!

I think@rvanlaak has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@fabpot
Copy link
Member

@welcoMattic Do you have time to answer Nicolas's question? Is it ready to be merged then?

@ossinkine
Copy link
Contributor

Thank you for MR! I suggest to merge it to 5.4 but not to 6.1 because actually this can be considered as a fix for more obvious behavior. I've also been surprised when discovered the domains are got not from config.

@welcoMatticwelcoMatticforce-pushed thefeature/use-provider-domains branch from6e02668 to2533821CompareApril 6, 2022 18:01
@welcoMattic
Copy link
MemberAuthor

@fabpot I fixed@nicolas-grekas comment, it's ready to merge now, sorry for the delay

@ossinkine we must consider it as a feature, because nothing is properly broken here

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

This looks like a bugfix to me as there is no way to opt-out of the proposed behavior. If it's a feature, it's a behavioral BC break then, which would be no-go ;)

@welcoMattic
Copy link
MemberAuthor

According to discussion with@nicolas-grekas, this PR is a bugfix. It's rebased on 5.4

@welcoMatticwelcoMatticforce-pushed thefeature/use-provider-domains branch from274625f tob752653CompareApril 12, 2022 14:43
@nicolas-grekas
Copy link
Member

Thank you@welcoMattic.

@nicolas-grekasnicolas-grekas merged commite7fcd9a intosymfony:5.4Apr 12, 2022
@welcoMatticwelcoMattic deleted the feature/use-provider-domains branchApril 12, 2022 14:56
@fabpotfabpot mentioned this pull requestApr 27, 2022
@plfort
Copy link
Contributor

It seems the problem still persist.

$domains is set from the $provider->getDomains() after reading local translations for all domains (if we don't pass 'domains' command option).
So when the diff occurs we still have unrelated domains pushed to the provider.

Maybe we can handle this issue with something like this ?

$domains =$domainsInput =$input->getOption('domains');if ($providerinstanceof FilteringProvider) {$providerDomains =$provider->getDomains();if ($providerDomains) {$domains =$domains ?array_intersect($domains,$providerDomains) :$providerDomains;if (!$domains) {$io->error(sprintf('The provider is configured for this domains: "%s", "%s" provided',implode(',',$providerDomains),implode(',',$domainsInput) ));return Command::FAILURE;        }    }}$localTranslations =$this->readLocalTranslations($locales,$domains,$this->transPaths);if (!$domains) {$domains =$this->getDomainsFromTranslatorBag($localTranslations);}

@plfort
Copy link
Contributor

Or maybe we should handle the case in FilteringProvider::write (and FilteringProvider::delete ?), it seems more logical to filter domains here

chalasr added a commit that referenced this pull requestMay 6, 2022
… the provider has domains (Florian-B)This PR was merged into the 5.4 branch.Discussion----------[Translation] Refresh local translations on PushCommand if the provider has domains| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | no| License       | MIT| Doc PR        | noSince#45171 it's possible to define targeted translation domains in configuration.On `translation:push` without `--domains` option the command should read our configuration in order build local translations list.If the domain is only define in the configuration we need to refresh the local translations list to prevent full update of Provider (push all domains without filtering).I think we should add a test but I failed to do it properly.Commits-------8b0f7eb [Translation] Refresh local translations if the provider has domains
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@wouterjwouterjAwaiting requested review from wouterj

@chalasrchalasrAwaiting requested review from chalasr

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

6 participants

@welcoMattic@carsonbot@fabpot@ossinkine@nicolas-grekas@plfort

[8]ページ先頭

©2009-2025 Movatter.jp