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

[Form] Add label_translation_parameters, help_translation_parameters and attr_translation_parameters options to base form type #28635

Merged
merged 1 commit into from Feb 13, 2019

Conversation

Projects
None yet
7 participants
@webnet-fr
Copy link
Contributor

webnet-fr commented Sep 28, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes. Travis-ci isn't green because it tests the components separately. Fabbot.io requires license headers in files where they were not present before.
Fixed tickets #27698
License MIT
Doc PR symfony/symfony-docs#10065

Hi, this is an alternative to #27775.

translation_parameters is separated to label_translation_parameters, help_translation_parameters, attr_translation_parameters.

@webnet-fr webnet-fr changed the title Form translation parameters labels helps [Form] Add label_translation_parameters, help_translation_parameters and attr_translation_parameters options to base form type Sep 28, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Sep 29, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Sep 29, 2018

Travis-ci isn't green because it tests the components separately

that's the point of the PR, ensuring components are still compatible with each others versions listed in composer.json files. This should be fixed.

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Sep 29, 2018

At some point i'd really like to see a Translation VO :) this applies to at least choice labels as well.

A single value could simplify things a lot:

'label' => 'text',
'other_label' => new Translation('text', 'domain', ['param' => 'value'], 'nl_NL'),
'other_other_label' => new IdentityTranslation('text'),
@webnet-fr

This comment has been minimized.

Copy link
Contributor Author

webnet-fr commented Oct 1, 2018

@nicolas-grekas, travis CI is actually green when deps option is not specified. In this case I assume the components changed by this PR are tested together and this test is passed successfully.

On the other hand with deps=high composer installs symfony/form (4.2.x-dev) while with deps=low it uses symfony/form (v4.1.0). Correct me if I am wrong but integrity tests of different versions of components (Form, FrameworkBudle, DoctineBridge, TwigBridge) cannot pass and it is normal.

@webnet-fr

This comment has been minimized.

Copy link
Contributor Author

webnet-fr commented Oct 1, 2018

Hi @ro0NL. There is always a way to acheive desired result in Symfony, that's why we love it :)
In your example we need to get current locale which is not convenient IMO:

'other_label' => new Translation('text', 'domain', ['param' => 'value'], $request->getLocale()),
@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Oct 1, 2018

Those would be all nullable (except $text), as its purpose would be to override defaults

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Oct 1, 2018

@webnet-fr Those tests ensure that Symfony 4.1 components will work with dependencies in version 4.2. This is a bit hard for the form theme tests though as they almost do 1:1 comparisons of the generated HTML. We also struggle with that in #27043 for example. So we need to look how we can relax assertions in 4.1 a bit to reduce the pain here.

@webnet-fr

This comment has been minimized.

Copy link
Contributor Author

webnet-fr commented Oct 4, 2018

@xabbuh I moved help_translation_parameters to FormType. Anyway personally I'd prefer #27775.

Yes, I completely agree that form tests are tricky especially when tesing transverse changes.

@xabbuh xabbuh added the Translator label Oct 25, 2018

@xabbuh xabbuh force-pushed the webnet-fr:form_translation_parameters_labels_helps branch from 5775d58 to 0d55112 Oct 27, 2018

@Soullivaneuh

This comment has been minimized.

Copy link
Contributor

Soullivaneuh commented Dec 10, 2018

Well even if I'm sure of my review, I'm less sure @carsonbot should listen to me. 😉

@xabbuh xabbuh force-pushed the webnet-fr:form_translation_parameters_labels_helps branch 6 times, most recently from e193064 to 3446db6 Feb 8, 2019

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Feb 8, 2019

The deps=high job will pass once the PR is merged.

@xabbuh

xabbuh approved these changes Feb 8, 2019

Copy link
Member

xabbuh left a comment

The changes look good to me.

@webnet-fr Can you add an entry to the changelog file of the Form component to mention the new options?

@webnet-fr

This comment has been minimized.

Copy link
Contributor Author

webnet-fr commented Feb 10, 2019

@xabbuh I've updated changelog.

I will correct documentation PR shortly.

@webnet-fr

This comment has been minimized.

Copy link
Contributor Author

webnet-fr commented Feb 12, 2019

Documentation is updted. symfony/symfony-docs#10065

@fabpot

fabpot approved these changes Feb 13, 2019

[Form] Add label_translation_parameters, help_translation_parameters …
…and attr_translation_parameters options to base form type

@fabpot fabpot force-pushed the webnet-fr:form_translation_parameters_labels_helps branch from d01c06c to b3f3c53 Feb 13, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 13, 2019

Thank you @webnet-fr.

@fabpot fabpot merged commit b3f3c53 into symfony:master Feb 13, 2019

0 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

fabpot added a commit that referenced this pull request Feb 13, 2019

feature #28635 [Form] Add label_translation_parameters, help_translat…
…ion_parameters and attr_translation_parameters options to base form type (webnet-fr)

This PR was squashed before being merged into the 4.3-dev branch (closes #28635).

Discussion
----------

[Form] Add label_translation_parameters, help_translation_parameters and attr_translation_parameters options to base form type

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes. Travis-ci isn't green because it tests the components separately. Fabbot.io requires license headers in files where they were not present before.
| Fixed tickets | #27698
| License       | MIT
| Doc PR        | symfony/symfony-docs#10065

Hi, this is an alternative to #27775.

`translation_parameters` is separated to `label_translation_parameters`, `help_translation_parameters`, `attr_translation_parameters`.

Commits
-------

b3f3c53 [Form] Add label_translation_parameters, help_translation_parameters and attr_translation_parameters options to base form type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment