Skip to content

Fix required field asterisk on array of rules for field #5792

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jnoordsij
Copy link
Contributor

@jnoordsij jnoordsij commented Mar 31, 2025

WHY

BEFORE - What was wrong? What was happening before this PR?

When using the crud->setValidation helper with a request class or ruleset, where properties have an array of rules applied to them, the field would only be marked as 'required' if the first element of the ruleset would be required, rather than any of them.

This is caused by the 'outer' loop variable $key being overwritten inside the loop, causing it to be false after the first (mismatched) iteration.

AFTER - What is happening after this PR?

The field is correctly marked as required if there is any required rule in its rule array.

HOW

How did you achieve that, in technical terms?

By using a separate variable name for listing the field, we ensure $key will always be the original fieldname.
I also immediately break the loop if a field is found to be required for a slight performance benefit.

Finally I've also updated all the related tests slightly to ensure they contain a non-required field,
to ensure tests also check for false positives.

Is it a breaking change?

No

How can we test the before & after?

See added test: when running the first commit (new test + no fix) tests fail; then the test is fixed by the commit after

…y of rules

Also add some not required fields to validation as sanity check
This ensures subsequent rule entries are correctly parsed
@jcastroa87
Copy link
Member

Hello @jnoordsij thanks for the PR, i will ask @pxpm to check it, if everything is ok, will be merged soon.

Cheers.

@jcastroa87 jcastroa87 requested a review from pxpm March 31, 2025 20:11
@jcastroa87 jcastroa87 moved this to Needs Testing, Review or Docs in This week Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Testing, Review or Docs
Development

Successfully merging this pull request may close these issues.

3 participants