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] lazy transpost_max_size_message.#19595
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
aitboudad commentedAug 10, 2016
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #19256 |
| License | MIT |
| Doc PR | ~ |
| } | ||
| /** | ||
| * This method is public because it needs to be callable from a closure in PHP 5.3. |
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 can do without it by using a reference in theuse of the closure, wouldn't that be better?
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.
even better :), thanks
| $form->addError(newFormError( | ||
| $form->getConfig()->getOption('post_max_size_message'), | ||
| (string)$form->getConfig()->getOption('post_max_size_message'), |
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.
could this be a bc break somehow?
aitboudadAug 13, 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.
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.
not sure, but it can be treated as a string like the old ones
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.
does it work if you remove the cast?
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.
just checked and I found that it can lead a bc break (if someone try to serialize this option)
nicolas-grekas commentedAug 23, 2016
@aitboudad any other idea to fix the linked issue that has no bc break inside? |
| return'form'; | ||
| } | ||
| publicfunction__toString() |
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.
-1 for making the TypeExtension itself castable to string as the error message. It looks really weird to me.
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.
I don't like either :), but I was looking for a solution to avoid bc break
aitboudad commentedAug 23, 2016
Replace |
nicolas-grekas commentedAug 24, 2016
@aitboudad let's try :) |
6f52062 to7797a16Compareaitboudad commentedAug 24, 2016
done |
| 'action' =>'', | ||
| 'attr' =>$defaultAttr, | ||
| 'post_max_size_message' =>'The uploaded file was too large. Please try to upload a smaller file.', | ||
| 'upload_max_size_message' =>$uploadMaxSizeMessage, |
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 could add// internal at the end of the line, and/or maybe add a comment somewhere explaining why you define it as a callable.
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.
just added a small description, can you look up ?
HeahDude commentedAug 24, 2016
Wow, great! Thanks for that fix! |
7797a16 to0b597f5Compare| }; | ||
| // Derive "upload_max_size_message" closure from "post_max_size_message" option, | ||
| // to allow override or translate the message only when it's needed and not during the resolve `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.
I think there are typo:overriding andtranslating.
What about something like:
// Wrap "post_max_size_message" in a closure to translate it lazily$uploadMaxSizeMessage ...
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.
good one, thanks!
0b597f5 toc03164eCompareHeahDude commentedSep 6, 2016
This one is very tricky :) 👍 Status: reviewed |
fabpot commentedSep 6, 2016
Thank you@aitboudad. |
This PR was merged into the 2.7 branch.Discussion----------[form] lazy trans `post_max_size_message`.| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19256| License | MIT| Doc PR | ~Commits-------c03164e [form] lazy trans `post_max_size_message`.