Skip to content
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

Validation: support placeholders for anything #3910

Conversation

element-code
Copy link
Contributor

@element-code element-code commented Nov 19, 2020

This fixes #3774

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a couple of tests.

@paulbalandan paulbalandan added the tests needed Pull requests that need tests label Dec 1, 2020
@MGatner
Copy link
Member

MGatner commented Feb 24, 2021

@element-code I'm afraid I don't understand either the problem or the solution. Could you either provide some tests or elaborate more on what the issue is and how your PR fixes it?

@element-code
Copy link
Contributor Author

element-code commented Feb 24, 2021

@MGatner the issue pops up when applying rules.
I'm not sure which cases are affected, and I don't have time right now to check it back, but the following cases to apply rules are valid and should work -- some of them do not work.
What i definetely fixed is case F, I don't know how the other cases behave, my guess is that A-D are fine and E & F are not working -- maybe I'm wrong and A, C, E are working and B, D, F are not :D .

Cases A, C and E are working, B and F are not working (and fixed) and case D is invalid.
My solution is always splitting them down to arrays to the lowest allowed level and go on from there.
Watch out, a rule can also be an array ['customRule', function(){]] as taken into account here.

A setRule (string)

setRule('foo', 'required|filter[{placeholder}]')

B setRule (array)

setRule('foo', ['required', 'filter[{placeholder}]'])

C setRules (simple, string)

setRules([
  'foo' => 'required|filter[{placeholder}]'
])

D setRules (simple, array) (invalid rule collection format)

setRules([
  'foo' => ['required', 'filter[{placeholder}]']
])

E setRules (complex, string)

setRules([
  'foo' => [
    'rules' => 'required|filter[{placeholder}]'
  ]
])

F setRules (complex, array)

setRules([
  'foo' => [
    'rules' => ['required', 'filter[{placeholder}]']
  ]
])

@MGatner
Copy link
Member

MGatner commented Feb 24, 2021

Thank you, a very good explanation and examples. It strikes me that your examples are pretty much just the test cases we need. Would you be willing to add those to the Validation Tests so we can get this PR reviewed and merged?

@element-code
Copy link
Contributor Author

element-code commented Feb 24, 2021

Jup, i still didnt write any tests and therefore have no experience with tests. Planning this for next week.
It would be nice if you could provide me with one positive and one negative test-case-implementation -- replicating code is much easier than finding out hot tf tests work :D .
In case you plan to do that, then PR branch should be open to public for pushes.

@element-code
Copy link
Contributor Author

@MGatner @paulbalandan any news on this?

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any style issues will be caught when we apply 4.2 to develop.

Please update user_guide_src/source/libraries/validation.rst and then I think this is good!

@element-code
Copy link
Contributor Author

@MGatner done

@MGatner
Copy link
Member

MGatner commented Aug 20, 2021

3 files changed

Still not seeing it! Did you mix up branches perhaps?

@element-code
Copy link
Contributor Author

@MGatner uhm, could you explain what you are expecting?
Im sorry, I just saw the conflict with 4.2 so I merged the updates.

@MGatner
Copy link
Member

MGatner commented Aug 24, 2021

I'm on mobile so I'm having a hard time seeing the whole thing but that file has some references to the old method signature that needs updating. For example:

setRule()
...
It takes the name of the field as
the first parameter, an optional label and a string with a pipe-delimited list of rules
that should be applied

Just scan through the file and make sure anything you changed is updated and any new features are explained properly.

@paulbalandan
Copy link
Member

@MGatner can you check on this one?

@kenjis
Copy link
Member

kenjis commented Sep 9, 2021

It seems the base branch of this PR must be changed to develop.

@paulbalandan paulbalandan changed the base branch from 4.2 to develop September 12, 2021 13:20
@kenjis
Copy link
Member

kenjis commented Oct 31, 2021

@kenjis kenjis added the waiting for info Issues or pull requests that need further clarification from the author label Oct 31, 2021
@kenjis kenjis added stale Pull requests with conflicts enhancement PRs that improve existing functionalities labels Nov 9, 2021
@lonnieezell
Copy link
Member

@kenjis @paulbalandan Do either of you want to champion this one enough to rebase it and fix any conflicts or should we close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities stale Pull requests with conflicts waiting for info Issues or pull requests that need further clarification from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Validation: Placeholder replacement not working for complex validation rules