- Notifications
You must be signed in to change notification settings - Fork3.4k
Fix up validation of marshaled data when fields is given.#18324
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
base:5.next
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The public API can't be changed in 5.x
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.
We can go 5.next
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Any other ideas we can do to mitigate in 5.x? Until a solution can be found for 6.x?
I didnt get any feedback on this since April.
I dont think this is how it worked so far. So |
I propose context (#18769 ) of being merged (first). A clear migration guide will minimize issue with extensions. It should also be easy to rework by adding the additional things into that existing context then on userland side. |
Uh oh!
There was an error while loading.Please reload this page.
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