Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
add missing double-quotes to extra_fields output message#28840
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
add missing double-quotes to extra_fields output message#28840
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedOct 12, 2018
I think the intention was to have something like |
zanbaldwin commentedOct 12, 2018
Congrats on your first Symfony pull request! 🎉 For the merger team: an alternative could be add the extra quotes before and after the imploded fields, but I agree with the style proposed (it's unlikely that form field names will contain commas, and the extra quotes get escaped in APIs using JSON format making it look ugly). Status: Reviewed |
danielkay commentedOct 15, 2018
@fabpot Unfortunately it isn't doing so... I feel we should take the code one way or the other - would with quotes or without quotes be better? Without the first and last quote feels weird... |
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 already hijack the format by submitting e.g.'foo","bar' => 'val', so not caring about quotes here seems fine.
fabpot commentedOct 16, 2018
I would add the missing quotes instead of removing them to be consistent with the way we display other error messages. |
ade8f68 to2379b40Comparedanielkay commentedOct 17, 2018
added missing quotes as per suggestion from@fabpot |
| }else { | ||
| $this->buildViolation($config->getOption('extra_fields_message')) | ||
| ->setParameter('{{ extra_fields }}',implode('", "',array_keys($form->getExtraData()))) | ||
| ->setParameter('{{ extra_fields }}','"'.implode(',',array_keys($form->getExtraData())).'"') |
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.
here'", "' should remain as glue also right
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.
Ooops, well spotted 👍
3ef528c tod1d2c48Compared1d2c48 to9e74141Comparefabpot commentedOct 17, 2018
Thank you@danielkay. |
…danielkay)This PR was squashed before being merged into the 2.8 branch (closes#28840).Discussion----------add missing double-quotes to extra_fields output message| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | N/A| License | MIT| Doc PR | N/AWhen using the extra_fields_message option on FormTypes to include the extra_fields items in the message, the output included some superfluous (or, perhaps, missing?) quotes. This resulted in some strange output: `This form should not contain extra fields: notes1\", \"notes2\", \"notes3`.This PR removes the quotes and instead implodes the extra_fields array using merely ', ' as the glue, resulting in the output: `This form should not contain extra fields: notes1, notes2, notes3`.Commits-------9e74141 add missing double-quotes to extra_fields output message
When using the extra_fields_message option on FormTypes to include the extra_fields items in the message, the output included some superfluous (or, perhaps, missing?) quotes. This resulted in some strange output:
This form should not contain extra fields: notes1\", \"notes2\", \"notes3.This PR removes the quotes and instead implodes the extra_fields array using merely ', ' as the glue, resulting in the output:
This form should not contain extra fields: notes1, notes2, notes3.