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 #5545

Merged

Conversation

paulbalandan
Copy link
Member

@paulbalandan paulbalandan commented Jan 5, 2022

Description
Supersedes and closes #3910
Fixes #3774

I tried cherry-picking @element-code 's commit but ended up with merge conflicts instead, so the original authorship was lost.
The breaking change with this is the signature change of setRule.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@paulbalandan paulbalandan added the breaking change Pull requests that may break existing functionalities label Jan 5, 2022
@kenjis kenjis added the enhancement PRs that improve existing functionalities label Jan 6, 2022
@MGatner
Copy link
Member

MGatner commented Jan 6, 2022

This method change should be safe, right? Since all extended classes will still have string $rules which is LSP-compatible.

@paulbalandan
Copy link
Member Author

No, the parameter will no longer be contravariant since extending classes will now be typehinting a specific type (string) from the parent's type (this PR proposing as mixed).

https://3v4l.org/h6Ehe

@MGatner
Copy link
Member

MGatner commented Jan 6, 2022

In that case we will we need additions to the upgrade guide noting the BC. Since we have a ValidationInterface and this method is not included I think this is an acceptable break. Definitely one of the things we need in version 5 is a massive pass at all classes and methods, making everything possible private, final, or @internal so we don't keep running into these issue.

@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Jan 12, 2022
@kenjis
Copy link
Member

kenjis commented Jan 12, 2022

LGTM except documentation.

@paulbalandan paulbalandan force-pushed the fix-validation-placeholders branch 2 times, most recently from 75ca70c to ab8eadb Compare January 14, 2022 13:09
@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Jan 15, 2022
@paulbalandan paulbalandan merged commit 9c0aa1a into codeigniter4:develop Jan 15, 2022
@paulbalandan paulbalandan deleted the fix-validation-placeholders branch January 15, 2022 11:47
@MGatner
Copy link
Member

MGatner commented Jan 15, 2022

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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