Fix up validation of marshaled data when fields is given. #18324
Conversation
|
I cannot reproduce the fails locally, not sure whats going on here. |
Is the scenario here that a patch/save operation has a |
|
It is mainly also the requirePresence rule that fires too harshly, so even if you don't provide them they could invalidate I guess. |
| * @return array<array> Array of failed fields | ||
| */ | ||
| public function validate(array $data, bool $newRecord = true): array | ||
| public function validate(array $data, bool $newRecord = true, array $fields = []): array |
There was a problem hiding this comment.
The public API can't be changed in 5.x
There was a problem hiding this comment.
For now this is just to get the feel if there is a better way to do this, what strategy to use and naming for the feature flag / config.
There was a problem hiding this comment.
We can go 5.next
We can't do breaking changes even in a new minor. Changing the public method would cause an error if it's overridden. We don't give BC guarantee for only protected overridden methods in minors.
There was a problem hiding this comment.
Any other ideas we can do to mitigate in 5.x? Until a solution can be found for 6.x?
One idea on how to solve the issue of #18232
Reasoning:
When fields are given, one usually does that for security reasons, so not to allow other fields to be touched (e.g. role_id by injecting that into the post data).
Now when that field then gets validated along with it, it undermines the idea behind using the fields config, and as such people stop using it, making the application overall less secure.
So it is vital that we only allow patching (and that includes validation) of fields that are in that list if passed along.
Also, if you are not setting those other fields (into the entity), having them in the payload should just ignore them instead of acting on them as well.
That said:
It would break existing code, e.g. fields you want to validated but not stored in entity (e.g. checkbox for "accept terms and conditions").
So to keep things safe, the
strictFieldsbool flag would have to be switched on for this behavior.Alternative names maybe:
validateOnlyFieldsvalidateFieldList