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] Add TranslatorFallbackInterface, fixes #25356#25549

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

Closed
Warxcell wants to merge1 commit intosymfony:masterfromWarxcell:2.7
Closed

Conversation

@Warxcell
Copy link
Contributor

@WarxcellWarxcell commentedDec 19, 2017•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?kind of
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#25356
LicenseMIT
Doc PRno

@Simperfit
Copy link
Contributor

Could you please squash all the commits ? You should link the email you comitted with to your github account.

@Warxcell
Copy link
ContributorAuthor

@Simperfit done.

namespaceSymfony\Component\Translation;

/**
* TranslatorFallbackInterface.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed

interface TranslatorFallbackInterfaceextends TranslatorInterface
{
/**
* Gets the fallback locales.
Copy link
Contributor

Choose a reason for hiding this comment

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

the method name is self explanatory.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I used description as-is from Translator.php

publicfunctiongetFallbackLocales();

/**
* Sets the fallback locales.
Copy link
Contributor

Choose a reason for hiding this comment

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

the method name is self explanatory.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I used description as-is from Translator.php

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this IMHO.

Copy link
ContributorAuthor

@WarxcellWarxcellDec 20, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Everywhere else there is a description. This method is also self-explanatory, but it has description.

/**     * Returns the current locale.     *     * @return string The locale     */publicfunctiongetLocale();

I think it should stay.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the point but the fact is this should be cleaned :#24931

class IdentityTranslatorimplements TranslatorInterface
{
/**
* @var MessageSelector
Copy link
Contributor

Choose a reason for hiding this comment

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

to be removed

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Why? I think it's good to typehint variables in docblock?

Copy link
Contributor

@SimperfitSimperfitDec 20, 2017•
edited
Loading

Choose a reason for hiding this comment

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

It does not add anything because we already know this with the constructor.

private$selector;

/**
* @var string
Copy link
Contributor

Choose a reason for hiding this comment

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

to be removed

@Simperfit
Copy link
Contributor

Since it's a new feature could you please targetmaster ?

Also you are missing some test ;)

publicfunctionsetFallbackLocales(array$locales)
{
if (!$this->translatorinstanceof TranslatorFallbackInterface) {
thrownew \BadMethodCallException('The original translator does not support fallback locales');
Copy link
Contributor

Choose a reason for hiding this comment

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

missing dot at te end

publicfunctionsetFallbackLocales(array$locales)
{
if (!$this->translatorinstanceof TranslatorFallbackInterface) {
thrownew \BadMethodCallException('The original translator does not support fallback locales');
Copy link
Contributor

Choose a reason for hiding this comment

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

missing dot at the end

@Warxcell
Copy link
ContributorAuthor

@Simperfit I'm not sure if it's whole new feature, because the code still looks and acts like before.
The same reason I think tests are enough. I could only assert all fallback tests that Translator is fallback translator ?

@Simperfit
Copy link
Contributor

For the test you need to assert that an exception is thrown when the translator does not implement the new interface.

IMHO it's a new interface, it should be inmaster but I may be wrong.@xabbuh WDYT ?

@Warxcell
Copy link
ContributorAuthor

@Simperfit I added some tests. You can see them.

@nicolas-grekasnicolas-grekas added this to the2.7 milestoneDec 20, 2017
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.

adding a new interface is definitely a new feature to me, so this should be for master, isn't it?

// force catalogue loading
$translator->trans('bar');

$this->assertTrue($translatorinstanceof TranslatorFallbackInterface);

Choose a reason for hiding this comment

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

you should use assertInstanceOf everywhere

@Warxcell
Copy link
ContributorAuthor

Removed redundant descriptions and typehints.

ReplaceassertTrue withassertInstanceOf

@stof
Copy link
Member

this must indeed go in master

@WarxcellWarxcell changed the base branch from2.7 tomasterDecember 20, 2017 10:36
*
* @throws \InvalidArgumentException If a locale contains invalid characters
*/
publicfunctionsetFallbackLocales(array$locales);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this method needs to be in the interface. Configuring the instance does not need to belong in the API.

Copy link
ContributorAuthor

@WarxcellWarxcellDec 20, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Then whyTranslatorInterface has ability tosetLocale ?

I'm just following the same pattern. Indeed I agree that if theTranslation Component does not need to configureTranslator, it should be only inside implementation.

Copy link
Member

Choose a reason for hiding this comment

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

The locale can change throughout the request. But I agree with@stof that this is probably not needed for the fallback locales.

@Warxcell
Copy link
ContributorAuthor

Rebased from master.

@xabbuhxabbuh modified the milestones:2.7,4.1Dec 20, 2017
@Warxcell
Copy link
ContributorAuthor

@xabbuh I made the requested changes. I just don't get what's the problem with appveyor?

@Warxcell
Copy link
ContributorAuthor

Warxcell commentedDec 21, 2017•
edited
Loading

Actually I still believe thatsetFallbackLocales should be in API, as it's used by framework bundle.

https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L888

What will happens if translator is replaced in some project with translator that does not support fallback locales?

@xabbuh
Copy link
Member

The failure on AppVeyor is not related to these changes.

@Warxcell
Copy link
ContributorAuthor

Let's say I also want to build something like "listener". A callback - when locale/fallback locales is/are changed inside translator - something to happen?

@nicolas-grekasnicolas-grekas modified the milestones:4.1,nextApr 20, 2018
@nicolas-grekas
Copy link
Member

I think I'm 👎 here:getFallbackLocales only fits reflection purposes in the code base. We'd better deprecate it actually and find another way for data collectors anddebug:translation. Let's reduce the complexity instead of increasing it.

@Warxcell
Copy link
ContributorAuthor

Warxcell commentedSep 21, 2018•
edited
Loading

@nicolas-grekas actually I'm not using that for debug purposes. I have built Entity Translation Bundle (for translating objects, mainly entities) which can be coupled with Symfony Translator (share same locale and fallback locales) instead of setting twice same thing.

Is there way to acomplish that with data collector?

@nicolas-grekas
Copy link
Member

Setting the same options/locales/fallbacks when configuring your translator seems the best option to me.

@nicolas-grekas
Copy link
Member

Closing in favor of#28579

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

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof left review comments

@xabbuhxabbuhxabbuh requested changes

+1 more reviewer

@SimperfitSimperfitSimperfit left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@Warxcell@Simperfit@stof@xabbuh@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp