fix: ValidationInterface #6253

kenjis · 2022-07-11T23:35:21Z

Description
Fixes #6224
Alternative #6247
Ref #4356

fix ValidationInterface::run() param
add missing
- ValidationInterface::setRule()
- ValidationInterface::getRules()
- ValidationInterface::getRuleGroup()
- ValidationInterface::setRuleGroup()
- ValidationInterface::loadRuleGroup()
- ValidationInterface::hasError()
- ValidationInterface::listErrors()
- ValidationInterface::showError()
fix Validation::loadRuleGroup()

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

MGatner

I like this much better. Let's still see what happens on the main thread but this is a good example of modifying the interface. I expect this to have way lower impact than changing Model constructor signature.

system/Validation/Validation.php

MGatner · 2022-09-03T13:11:54Z

@kenjis Can you rebase this? I would like to propose that we merge this for 4.3 and see what kind of uproar it causes (if any), and if it seems generally well-received then we proceed with the other interfaces identified in #5909

kenjis · 2022-09-03T13:30:05Z

Rebased.

MGatner

Good by me! Offering a final chance to #develop

MGatner · 2022-09-04T11:15:04Z

@iRedds have you seen this?

iRedds · 2022-09-04T12:01:42Z

@MGatner In fact, I think that interface hell is going on in the framework. In particular, this applies to the HTTP layer. When instead of describing the public API in interfaces, methods are moved to traits. Instead of inheritance, interfaces are enumerated in a class.
The fact that the interfaces are being fixed is good.

But for some reason these methods are missing.

setRule
getRules
getRuleGroup
setRuleGroup

iRedds · 2022-09-04T12:07:57Z

system/BaseModel.php

@@ -198,7 +197,7 @@ abstract class BaseModel
    /**
     * Our validator instance.
     *
-     * @var Validation
+     * @var ValidationInterface
     */
    protected $validation;


It seems to me that it makes sense to specify the property type directly instead of docBlock in BaseModel and Controller.

The existing user's Model will break.
https://3v4l.org/NkQVr

Why override the property?

Good question. I don't know, but we can override.
I should have said, the existing user's Model might break.

MGatner · 2022-09-05T11:17:57Z

interface hell is going on in the framework

Oh very much so! Please see #5909 if you haven't already. Basically after much discussion: we've decided to update the interfaces with the missing methods that are already used internally. This is a big "no no" but so is the current state, and this has the least likely path to ruin people's existing code.

But for some reason these methods are missing.

setRule

getRules

getRuleGroup

setRuleGroup

I'm on mobile so can't really search the framework right now. @kenjis have you determined these aren't necessary for the interface?

kenjis · 2022-09-05T11:32:44Z

I didn't determined, but at least these methods are not needed in the current framework implementation.
Because there is no PHPStan error.

If we change this return type
https://github.com/codeigniter4/CodeIgniter4/pull/6253/files#diff-f75b78ec6c6dc29bb97bc8c978fadab71609e66edd39159ac32b0f7963c34c02R737
we should add these methods in the interface?

MGatner · 2022-09-05T12:00:29Z

Each Service should return an Interface or Abstract Class (when relevant). Remember that Services is our "alternative to a Container. So yes: we should add these methods if they are needed for our service locator to support the interface.

iRedds · 2022-09-05T13:07:40Z

Oh very much so! Please see #5909 if you haven't already.

Maybe it's easier to announce the development of the 5th version and already start writing correctly?
Because it's not just about interfaces.
With the current activity, as well as the number of strange and incomprehensible implementations, the development of the 5th version will take a year or 1.5. Look at Yii3, it has not been released for 3 years now.
And given the timing, it already needs to be done on php8.1

MGatner · 2022-09-05T13:32:24Z

It's a bigger decision than "how to develop this"; OSS is largely software-driven but there are still business concerns. In this case the CodeIgniter Foundation is essentially our "stakeholder" and (though we have a lot of discretion generally) the major release cycle is an area where we have to be attentive.

I did push to start version 5 last April with a goal of a December release (to correspond with the dropping of 7.4 from security support) but the reality was we don't have enough contributors to accomplish everything that needs to happen in order to make a major version "count". I still think v5 is our next likely big release after 4.3 (maybe 4.4?) but ultimately that will be more of an empirical decision based on workload versus contributions.

The good news? I'm committed to a healthy refactoring process which means as soon as we have a v5 branch it will always represent a usable state (non-production), and there's a LOT of low-hanging fruit. The jump from 7.4 minimum to 8.0 minimum unlocks an entirely new toolset and will give us a rich opportunity to tackle our typing-problems in a Grade A manner.

TL;DR we're not there yet but there's a lot of hope for the journey ahead.

kenjis · 2022-09-05T21:52:27Z

Do we need to add the following methods (except constructor)?

$ grep 'public function' system/Validation/ValidationInterface.php  | cut -f 7 -d ' ' | cut -f 1 -d '(' > ValidationInterface.methods
$ grep 'public function' system/Validation/Validation.php | cut -f 7 -d ' ' | cut -f 1 -d '(' > Validation.methods
$ diff -u Validation.methods ValidationInterface.methods 
--- Validation.methods	2022-09-06 06:50:03.000000000 +0900
+++ ValidationInterface.methods	2022-09-06 06:49:59.000000000 +0900
@@ -1,17 +1,8 @@
-__construct
 run
 check
 withRequest
-setRule
 setRules
-getRules
 hasRule
-getRuleGroup
-setRuleGroup
-listErrors
-showError
-loadRuleGroup
-hasError
 getError
 getErrors
 setError

The following methods seem to be undocumented:

getRules
getRuleGroup
loadRuleGroup

iRedds · 2022-09-05T23:18:21Z

If these methods are not documented, they are not in the interface and they are not used by the framework, then maybe they are not needed?

kenjis · 2022-09-05T23:28:09Z

The above result is on 4.3 branch.
On this PR branch:

$ grep 'public function' system/Validation/Validation.php | cut -f 7 -d ' ' | cut -f 1 -d '(' | sort > Validation.methods
$ grep 'public function' system/Validation/ValidationInterface.php  | cut -f 7 -d ' ' | cut -f 1 -d '(' | sort > ValidationInterface.methods
$ diff -u Validation.methods ValidationInterface.methods 
--- Validation.methods	2022-09-06 08:22:47.000000000 +0900
+++ ValidationInterface.methods	2022-09-06 08:22:59.000000000 +0900
@@ -1,9 +1,6 @@
-__construct
 check
 getError
 getErrors
-getRuleGroup
-getRules
 hasError
 hasRule
 listErrors
@@ -11,8 +8,6 @@
 reset
 run
 setError
-setRule
-setRuleGroup
 setRules
 showError
 withRequest

kenjis · 2022-09-05T23:37:29Z

getRuleGroup() has been documented in the sample code:
https://codeigniter4.github.io/CodeIgniter4/libraries/validation.html?highlight=getrules#getting-setting-rule-groups

kenjis · 2022-09-05T23:38:33Z

getRules() is used in tests.

MGatner · 2022-09-06T00:17:12Z

Ugh. So much for picking an easy one to start with 😕 What matters at the end of the day:

If a component defines an interface and is locatable via Services, the service return type should be the interface
If another component relies on the service as a dependency (injected or located) and currently uses methods not in that dependency: they must be added

It is acceptable to have extra public methods on a component class, even if they are documented, so long as we aren't building dependencies around them.

So for the method list provided... my guess is that we will need them all but I don't think there is a way to know without tracing back through the uses. One way to do this (I think this is how kenjis started) is to switch all the component class references to the interface (e.g. Validation to ValidationInterface) and let static analysis tell us the problems.

…nd showError()

I don't see it is useful.

kenjis · 2022-09-06T02:11:38Z

But for some reason these methods are missing.

setRule

getRules

getRuleGroup

setRuleGroup

Added these methods in the interface.

kenjis · 2022-09-06T02:14:03Z

Now ValidationInterface has all public methods in Validation.

$ grep 'public function' system/Validation/Validation.php | cut -f 7 -d ' ' | cut -f 1 -d '(' | sort > Validation.methods
$ grep 'public function' system/Validation/ValidationInterface.php | cut -f 7 -d ' ' | cut -f 1 -d '(' | sort > ValidationInterface.methods
$ diff -u Validation.methods ValidationInterface.methods 
--- Validation.methods	2022-09-06 11:12:11.000000000 +0900
+++ ValidationInterface.methods	2022-09-06 11:12:19.000000000 +0900
@@ -1,4 +1,3 @@
-__construct
 check
 getError
 getErrors

kenjis · 2022-09-06T02:21:38Z

I think the implementation is done.
If this is okay, I will add the documentation.

MGatner

Yes the implementation looks good to me.

system/Validation/Validation.php

kenjis · 2022-09-07T01:42:04Z

Added the docs.

MGatner

Here goes! 😳

kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jul 11, 2022

kenjis changed the base branch from develop to 4.3 July 11, 2022 23:35

kenjis force-pushed the fix-ValidationInterface branch from fa82cb5 to f89bd78 Compare July 11, 2022 23:43

kenjis mentioned this pull request Jul 11, 2022

fix: do not use ValidationInterface #6247

Closed

5 tasks

kenjis marked this pull request as draft July 11, 2022 23:51

kenjis force-pushed the fix-ValidationInterface branch from f89bd78 to 0710013 Compare July 12, 2022 00:00

kenjis added the 4.3 label Jul 12, 2022

kenjis marked this pull request as ready for review July 12, 2022 00:10

kenjis mentioned this pull request Jul 12, 2022

fix: incorrect type declarations #5909

Closed

5 tasks

kenjis added breaking change Pull requests that may break existing functionalities docs needed Pull requests needing documentation write-ups and/or revisions. labels Jul 12, 2022

MGatner reviewed Jul 12, 2022

View reviewed changes

system/Validation/Validation.php Show resolved Hide resolved

kenjis force-pushed the fix-ValidationInterface branch from 0710013 to d7cfb6a Compare September 3, 2022 13:27

MGatner approved these changes Sep 4, 2022

View reviewed changes

iRedds reviewed Sep 4, 2022

View reviewed changes

paulbalandan approved these changes Sep 4, 2022

View reviewed changes

kenjis force-pushed the fix-ValidationInterface branch from d7cfb6a to 8083390 Compare September 5, 2022 23:34

kenjis added 5 commits September 6, 2022 10:47

fix: ValidationInterface::run()

d7b905a

fix: add missing ValidationInterface::loadRuleGroup()

7af6dc7

fix: Validation::loadRuleGroup()

9ea41a5

fix: fix: add missing ValidationInterface::hasError(), listErrors() a…

518ceae

…nd showError()

docs: use ValidationInterface in Services

56c7073

kenjis force-pushed the fix-ValidationInterface branch from 8083390 to 56c7073 Compare September 6, 2022 01:47

kenjis added 5 commits September 6, 2022 10:48

docs: fix incorrect @throws

b76133c

docs: add comma

5b63caa

docs: remove @throws in ValidationInterface

be3860c

I don't see it is useful.

docs: use @phpstan-ignore-next-line instead of phpstan-baseline

51429cd

fix: add missing methods in ValidationInterface

a127d71

kenjis force-pushed the fix-ValidationInterface branch from 243e559 to a127d71 Compare September 6, 2022 02:04

MGatner reviewed Sep 6, 2022

View reviewed changes

system/Validation/Validation.php Show resolved Hide resolved

docs: add changelog and upgrading guide

c332ee6

kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Sep 7, 2022

iRedds approved these changes Sep 7, 2022

View reviewed changes

MGatner approved these changes Sep 7, 2022

View reviewed changes

kenjis merged commit 8785776 into codeigniter4:4.3 Sep 7, 2022

kenjis deleted the fix-ValidationInterface branch September 7, 2022 21:59

kenjis mentioned this pull request Sep 7, 2022

Bug: The Validation::run() method does not match the ValidationInterface. #6224

Closed

fix: ValidationInterface #6253

fix: ValidationInterface #6253

Uh oh!

Conversation

kenjis commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MGatner commented Sep 3, 2022

Uh oh!

kenjis commented Sep 3, 2022

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

MGatner commented Sep 4, 2022

Uh oh!

iRedds commented Sep 4, 2022

Uh oh!

iRedds Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

kenjis Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

iRedds Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

kenjis Sep 4, 2022

Choose a reason for hiding this comment

Uh oh!

MGatner commented Sep 5, 2022

Uh oh!

kenjis commented Sep 5, 2022

Uh oh!

MGatner commented Sep 5, 2022

Uh oh!

iRedds commented Sep 5, 2022

Uh oh!

MGatner commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenjis commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iRedds commented Sep 5, 2022

Uh oh!

kenjis commented Sep 5, 2022

Uh oh!

kenjis commented Sep 5, 2022

Uh oh!

kenjis commented Sep 5, 2022

Uh oh!

MGatner commented Sep 6, 2022

Uh oh!

kenjis commented Sep 6, 2022

Uh oh!

kenjis commented Sep 6, 2022

Uh oh!

kenjis commented Sep 6, 2022

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kenjis commented Sep 7, 2022

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kenjis commented Jul 11, 2022 •

edited

Loading

MGatner commented Sep 5, 2022 •

edited

Loading

kenjis commented Sep 5, 2022 •

edited

Loading