8000 [Form] Allow to translate each language into its language in LanguageType by javiereguiluz · Pull Request #32388 · symfony/symfony · GitHub
  • [go: up one dir, main page]
    Skip to content

    [Form] Allow to translate each language into its language in LanguageType #32388

    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.

    Sign up for GitHub

    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

    Jump to bottom
    Merged
    fabpot merged 1 commit into from
    Dec 3, 2019
    Merged

    [Form] Allow to translate each language into its language in LanguageType #32388

    fabpot merged 1 commit into from
    Dec 3, 2019

    Conversation

    @javiereguiluz
    Copy link
    Member
    @javiereguiluz javiereguiluz commented Jul 5, 2019 •
    edited
    Loading
    Q A
    Branch? 4.4
    Bug fix? no
    New feature? yes
    BC breaks? no
    Deprecations? no
    Tests pass? yes
    Fixed tickets #31562
    License MIT
    Doc PR I'll do it if this is approved

    This would allow to set this option:

    ->add('language', LanguageType::class, [
    'choice_self_translation' => true,
])

    To display each language translated into its own language:

    image

    @ro0NL if this proposal is approved, could you please tell me why I must try ... catch with \Exception?

    • First, I need the try..catch because of a special language with the code "root" which triggers exceptions (The resource bundle "symfony-code/src/Symfony/Component/Intl/Resources/data/languages/root.json" does not exist)
    • Second, I must catch \Exception because when I try to catch the exact exception (Symfony\Component\Intl\Exception\ResourceBundleNotFoundException) the exception is not caught. Why?

    @javiereguiluz javiereguiluz added this to the next milestone Jul 5, 2019
    @javiereguiluz javiereguiluz mentioned this pull request Jul 5, 2019
    Closed
    yceruto
    yceruto reviewed Jul 5, 2019
    View reviewed changes
    Copy link
    Member
    @yceruto yceruto left a comment

    Choose a reason for hiding this comment

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

    Nice ! I'd like to make it the default behavior if choice_translation_locale is not provided, WDYT?

    src/Symfony/Component/Form/Extension/Core/Type/LanguageType.php Outdated
    return array_flip($languagesList);
    });
    },
    'choice_self_translation' => false,
    Copy link
    Member
    @yceruto yceruto Jul 5, 2019 •
    edited
    Loading

    Choose a reason for hiding this comment

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

    what about make it default true if choice_translation_locale is null else set it to false? that can be achieved with a lazy default function

    ro0NL
    ro0NL reviewed Jul 5, 2019
    View reviewed changes
    Copy link
    Contributor
    @ro0NL ro0NL left a comment

    Choose a reason for hiding this comment

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

    You obtain root from the language codes due

    symfony/src/Symfony/Component/Intl/Resources/data/languages/meta.json

    Line 460 in 09e3cef

    "root",

    It should be excluded.

    However, strictly there's no co-relation between getNames and getLanguageCodes. The latter can have more codes. Consider getNames the canonicalized list.

    For the locales this is more obvious, see

    symfony/src/Symfony/Component/Intl/Resources/data/locales/meta.json

    Lines 569 to 570 in 09e3cef

    "th_TH",
    "th_TH_TRADITIONAL",

    Those are codes, but not available in the "canonicalized list of names".

    Raises the question, should LocaleType have this same option?

    The option should be called choice_self_translate IMHO, as it's an instruction.

    👍 for the option, no real opinion if it should be default. Though http://www.flagsarenotlanguages.com/blog/best-practice-for-presenting-languages/ considers it a best practice 👍

    @ro0NL
    Copy link
    Contributor
    ro0NL commented Jul 5, 2019

    when I try to catch the exact exception, the exception is not caught.

    🤔 that's confusing :) if you iterate getNames for codes it should be fine by default.

    @ro0NL ro0NL mentioned this pull request Jul 6, 2019
    Merged
    fabpot added a commit that referenced this pull request Jul 8, 2019
    @fabpot
    feature #32402 [Intl] Exclude root language (ro0NL)
    c03415d 
    This PR was squashed before being merged into the 4.4 branch (closes #32402).

Discussion
----------

 [Intl] Exclude root language

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Spotted in #32388

Commits
-------

089cc06  [Intl] Exclude root language
    @fabpot
    Copy link
    Member
    fabpot commented Jul 8, 2019

    The PR related to root has been merged now.

    fabpot
    fabpot approved these changes Dec 3, 2019
    View reviewed changes
    Copy link
    Member
    @fabpot fabpot left a comment

    Choose a reason for hiding this comment

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

    @javiereguiluz Can you rebase on current master so that we can merge?

    @javiereguiluz javiereguiluz changed the base branch from 4.4 to master December 3, 2019 07:57
    @javiereguiluz
    Copy link
    Member Author
    javiereguiluz commented Dec 3, 2019

    Rebased. Thanks.

    @fabpot
    Copy link
    Member
    fabpot commented Dec 3, 2019

    Thank you @javiereguiluz.

    @fabpot fabpot mentioned this pull request Dec 3, 2019
    Closed
    @fabpot fabpot closed this in 9097230 Dec 3, 2019
    @fabpot fabpot merged commit 138200c into symfony:master Dec 3, 2019
    @nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
    @fabpot fabpot mentioned this pull request May 5, 2020
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @yceruto yceruto yceruto left review comments

    @fabpot fabpot fabpot approved these changes

    @dunglas dunglas Awaiting requested review from dunglas

    @lyrixx lyrixx Awaiting requested review from lyrixx

    @sroze sroze Awaiting requested review from sroze

    @xabbuh xabbuh Awaiting requested review from xabbuh

    +1 more reviewer

    @ro0NL ro0NL ro0NL left review comments

    Reviewers whose approvals may not affect merge requirements

    Assignees

    No one assigned

    Labels

    Feature Form Status: Reviewed

    Projects

    None yet

    Milestone

    5.1

    Development

    Successfully merging this pull request may close these issues.

    6 participants

    @javiereguiluz @ro0NL @fabpot @yceruto @nicolas-grekas @carsonb 3216 ot
    0