[SettingsBundle] Add validation when saving parameters to guarantee database integrity #1193
Conversation
|
I think this is a bugfix, rather than an enhancement. |
| $request->getSession()->getFlashBag()->add('success', $message); | ||
| } catch (ValidatorException $exception) { | ||
| $message = $this->getTranslator()->trans($exception->getMessage(), array(), 'flashes'); | ||
| $request->getSession()->getFlashBag()->add('error', $message); |
There was a problem hiding this comment.
Please move this below, and add variable $type = 'error', and $type = 'success'.
|
Will fix. I should read code guidelines better next time :) |
|
Thanks for figuring this out... How about changing the Parameter.value field to text doctrine type? I'm surprised I've used 255 string... It definitely should be a text field... |
|
@pjedrzejewski why not |
|
Object indeed seems to be exactly the type that fits this requirement |
|
Validation would than only need to be applied on the namespace and name fields |
|
Sorry, I meant |
|
I can integrate this in this PR (change get/set in |
|
How can it be that Travis fails on simply removing a line in a comment? |
|
@pjedrzejewski @stloyd I have added the Doctrine type change in this PR. Don't know why Travis fails though. There are no errors in the scenarios and the last commit only removes one comment line. Can anyone shed some light on this? |
|
@pjedrzejewski Without some trivial CS issues, it looks ok for me, what do you think about this? |
[SettingsBundle] Add validation when saving parameters to guarantee database integrity
|
Thanks Ruud! Nice work. 👍 |
Because of #1114, I've looked into the settings management in the SettingsBundle. The error in question results from an incomplete serialized value in the database, which in turn triggers a PHP notice when unserializing. I've added validation to at least prevent corrupt values in the parameters table.
I'm not yet satisfied how this code handles the error, though. Would it be an improvement to add a new ValidatorException which contains the complete ConstraintViolationList that's returned by $validator->validate()? This would also allow the caller of saveSettings (the SettingsController) to display which parameters failed and on which constraints (instead of only the first failing constraint of one parameter.