Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FORM] fix post_max_size_message translation#18543
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
DavidBadura commentedApr 14, 2016
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #15479 |
| License | MIT |
| Doc PR | - |
| publicfunction__construct(ServerParams$params =null) | ||
| publicfunction__construct(ServerParams$params =null,TranslatorInterface$translator =null,$translationDomain =null) | ||
| { | ||
| $this->serverParams =$params ?:newServerParams(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should create a translator ifnull is passed imo, since this can be used with Form as standalone component.
Alright forget it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a kind of feature, shouldn't be documented in the changelog that you can pass a translator to request handlers ?
Maybe this should go on master ?
nicolas-grekas commentedApr 15, 2016
It's a new feature to me also, should go on master |
Nek- commentedApr 15, 2016
@nicolas-grekas what feature does it add ? :-) IMO translation is not feature. Error translation is a feature of Symfony. Also there is no problem merging it in 2.7, so why not ? |
DavidBadura commentedApr 15, 2016
doesn't the bug exist because of#11924? Btw. the translation already exists:https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Resources/translations/validators.de.xlf#L10 but apparently it doesn't get used. i don't care whether it gets translated in 2.7 or master but it should be done at one point. |
HeahDude commentedApr 15, 2016
So as a bug fix it should go on 2.3, right ? I agree with you and would vote to do this as a bug fix, but I doubt it gets accepted. However, if it goes in master maybe there is a better way to handle "extra" translations (not handled by the violation builder) more globally (maybe use an abstract request handler to hold some common logic), the csrf type extension also gets the translator only to pass it to its listener (even if that allows a lazy translation of the message, I wonder what consumes more memory: passing the translator in many instances or just a translated message?). |
| $this->requestHandler =$this->getRequestHandler($translator); | ||
| $options =array('post_max_size_message' =>'old max {{ max }}!'); | ||
| $form =$this->factory->createNamed('name','text',null,$options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
text -->Symfony\Component\Form\Extension\Core\Type\TextType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this works only in >=2.8
aitboudad commentedApr 16, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Instead of the current implementation what do you think about adding a new form type extension that translate |
HeahDude commentedApr 17, 2016
@aitboudad 👍 Awesome! Remains the question: 2.3 or master? |
xabbuh commentedApr 21, 2016
To me this is a bugfix (IIRC this used to work before#11924). |
DavidBadura commentedApr 21, 2016
so, what are the next steps? is it a bug (merge in 2.3/2.7) or a feature (merge in master)? should I change it as proposed by@aitboudad? |
aitboudad commentedApr 21, 2016
well I would consider it as a bugfix |
fabpot commentedApr 28, 2016
This is a bugfix. |
fabpot commentedJun 15, 2016
2.7 branch is the good one as this is now the oldest and still maintained branch. |
DavidBadura commentedJun 15, 2016
#19061 <- an alternative implementation as proposed by@aitboudad |
fabpot commentedJun 22, 2016
closing in favor of#19061 |
…id Badura)This PR was merged into the 2.7 branch.Discussion----------[FORM] fix post_max_size_message translation (alt. 2)| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#15479,#18543| License | MIT| Doc PR | -Commits-------9d8a5e5 fix post_max_size_message translation