Skip to content

Conversation

@kenjis
Copy link
Member

@kenjis kenjis commented Jul 10, 2022

Description
Fixes #6224

BaseModel uses the 3rd param of $this->validation->run($data, null, $this->DBGroup).
So it depends on Validation, not ValidationInterface.

Ref: #5909 (comment)

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

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities 4.3 labels Jul 10, 2022
@kenjis kenjis force-pushed the fix-do-not-use-ValidationInterface branch from c6e9260 to 2dd88a6 Compare July 10, 2022 01:56
@kenjis kenjis added refactor Pull requests that refactor code docs needed Pull requests needing documentation write-ups and/or revisions. and removed refactor Pull requests that refactor code labels Jul 10, 2022
@MGatner
Copy link
Member

MGatner commented Jul 10, 2022

Let's see where the big PR takes this conversation. I'm fine with this one following from that - I do think that this is pretty likely to break some projects and libraries since Model constructors are likely to be extended.

@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

If we can change the ValidationInterface, we have an option to change it instead.

@MGatner
Copy link
Member

MGatner commented Jul 11, 2022

You know that's not a bad idea. If I had to guess I would say there are probably more developers extending Model::__construct() than making their own Validation driver (since the driver can be "extended" via rules already).

@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

there are probably more developers extending Model::__construct() than making their own Validation driver

I agree. I guess so.
It seems changing ValidationInterface makes less apps break.

@kenjis kenjis mentioned this pull request Jul 11, 2022
5 tasks
@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

I sent another PR #6253

@kenjis
Copy link
Member Author

kenjis commented Sep 7, 2022

#6253 was merged.

@kenjis kenjis closed this Sep 7, 2022
@kenjis kenjis deleted the fix-do-not-use-ValidationInterface branch September 7, 2022 22:00
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 bug Verified issues on the current code behavior or pull requests that will fix them docs needed Pull requests needing documentation write-ups and/or revisions.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants