-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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 our terms of service and privacy 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
Conversation
I think the intention was to have something like |
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 |
@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.
I would add the missing quotes instead of removing them to be consistent with the way we display other error messages. |
ade8f68
to
2379b40
Compare
added missing quotes as per suggestion from @fabpot |
->setInvalidValue($form->getExtraData()) | ||
->setCode(Form::NO_SUCH_FIELD_ERROR) | ||
->addViolation(); | ||
} 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
to
d1d2c48
Compare
d1d2c48
to
9e74141
Compare
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/A 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`. 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
.