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

fix: incorrect type declarations #5909

Closed
wants to merge 12 commits into from

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Apr 18, 2022

Description
I propose to fix incorrect type declarations as a bug fix in 4.x.

Related #4356

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 refactor Pull requests that refactor code breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them labels Apr 18, 2022
system/CodeIgniter.php Outdated Show resolved Hide resolved
@@ -545,7 +544,7 @@ public static function routes(bool $getShared = true)
*
* @return Router
*/
public static function router(?RouteCollectionInterface $routes = null, ?Request $request = null, bool $getShared = true)
public static function router(?RouteCollection $routes = null, ?Request $request = null, bool $getShared = true)
Copy link
Member

Choose a reason for hiding this comment

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

This makes it more difficult for people to pass their own classes that implement the RouteCollectionInterface, which they currently can, so this would be BC.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current Router does not depend on RouteCollectionInterface, it actually depends on RouteCollection.

If a user creates their own RouteCollection which implements RouteCollectionInterface,
it is not compatible with the Router, because some methods are missing in the RouteCollectionInterface.

Copy link
Member

Choose a reason for hiding this comment

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

@lonnieezell can we pick this discussion back up?

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe change the Router class?

@@ -897,7 +897,7 @@ public function prepare(Closure $func, array $options = [])

$this->pretend(false);

if ($sql instanceof QueryInterface) {
if ($sql instanceof Query) {
Copy link
Member

Choose a reason for hiding this comment

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

Again - BC break since they can currently pass custom implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

QueryInterface does not have getOriginalQuery().
So it can't be used.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Would it be less breaking to add getOriginalQuery to the interface? I realize they can just extend the original Query class to do custom implementations but that defeats the whole purpose of having an interface to begin with.

Without looking at the history of things I want to say that method came after the initial release, but even if it didn't I think the bug would be that it wasn't included.

@kenjis kenjis force-pushed the fix-incorrect-types branch from b3879b1 to 6054891 Compare April 20, 2022 06:28
@MGatner
Copy link
Member

MGatner commented Apr 22, 2022

It's a mess, which we've known. I'm still torn on what to do about it. We use methods that don't exist in our interfaces; we have classes that don't use our own interfaces. The current state is not healthy, but would require breaking changes in either direction to fix.

Are we "breaking" things that wouldn't work anyways? Maybe? Hard to know. This is why we were leaving this for version 5.

Maybe an easier approach would be to split these apart into smaller PRs, by category or class components? Then we could deliberate on them individually?

@kenjis
Copy link
Member Author

kenjis commented Apr 22, 2022

@MGatner
This PR drops the dependency on interfaces, but methods can be added to interfaces.

However, it may not be healthy to add more methods to the interface. Fat interfaces are a sign of bad design.

Are you of the opinion that mechanically deciding one way or the other is not a good idea? That may indeed be true.

@MGatner
Copy link
Member

MGatner commented Apr 23, 2022

This is mostly version 5 talk but I think if interfaces need more methods then they probably should be (or should have been) abstract classes. An abstract class gives us as developers the flexibility to add and adjust class needs while still supporting any extensions from other developers.

Interfaces are a great way to keep the framework extensible, especially with anything that is offered as a Service (and this easily replaceable) but as you said these should be kept "bare minimum" which is necessarily restrictive on framework use of them.

@MGatner
Copy link
Member

MGatner commented Apr 23, 2022

For this PR at hand, here's my opinion: split it out by independent components and then identifying the missing interface method/methods of each component.

If those methods appear to be highly specialized/advanced/niche (i.e. unlikely to be re-implemented) then I would consider that interface "safe for advanced replacement", that is, anyone who is supplying their own interfaced class is either extending our class or competent enough to handle a breaking change due to the replacement.

If the missing methods appear to be core to the use of the class, or likely for extension/reimplementation then we should leave the interface, because someone supplying their own interfaces class has likely already gone through the trouble of figuring out the "method _____ doesn't exist" error and adjusted.

@kenjis kenjis marked this pull request as draft May 10, 2022 04:48
@MGatner
Copy link
Member

MGatner commented Jul 9, 2022

Looking back on this after three months of framework cleanup: I am in favor of these changes. The current state is disgraceful and I think leaving it for version 5 (which we still don't have planned) is perpetuating this sad state. Rip the bandaid off, do this the right way!

@kenjis kenjis mentioned this pull request Jul 10, 2022
5 tasks
@kenjis kenjis force-pushed the fix-incorrect-types branch from 6054891 to 168b66c Compare July 10, 2022 02:10
@kenjis kenjis changed the base branch from develop to 4.3 July 10, 2022 02:11
@kenjis kenjis added 4.3 and removed refactor Pull requests that refactor code labels Jul 10, 2022
@kenjis kenjis changed the title Fix incorrect type declarations fix: incorrect type declarations Jul 10, 2022
@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Jul 10, 2022
@kenjis kenjis marked this pull request as ready for review July 10, 2022 02:19
@MGatner
Copy link
Member

MGatner commented Jul 10, 2022

Why 4.3? Just because there are so many potentially-breaking changes?

@samsonasik
Copy link
Member

I think changing signature should be major, but... if it really needed, should be a next minor 4.3

@MGatner
Copy link
Member

MGatner commented Jul 10, 2022

I think changing signature should be major, but... if it really needed, should be a next minor 4.3

For sure it should be major, but version 5 is a long way off and this is currently a mess. We define interfaces and then don't even stick to our own interfaces 😅 I think the 27 PHPStan errors that we can stop ignoring is very telling.

But yes, that makes sense to push to 4.3 due to the impact.

@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

We defined interfaces, but our classes does not depend on the interfaces correctly.
That is if a dev creates a class that implements the interface, there is no guarantee that the class will work with CI4.
Because CI4 classes use methods or params that the interface does not have. E.g. #6224

As a result, no one could trust the interfaces, and VS Code always shows errors!

This is not a normal bug fix. So it should not go to 4.2.x.

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

I started leaving comments but they all became the same thing - what is the benefit to developers of these changes? It seems that every single one of them reduces the ability of devs to extend and customize the framework. As far as I can tell none of them really has a benefit....

@@ -897,7 +897,7 @@ public function prepare(Closure $func, array $options = [])

$this->pretend(false);

if ($sql instanceof QueryInterface) {
if ($sql instanceof Query) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be less breaking to add getOriginalQuery to the interface? I realize they can just extend the original Query class to do custom implementations but that defeats the whole purpose of having an interface to begin with.

Without looking at the history of things I want to say that method came after the initial release, but even if it didn't I think the bug would be that it wasn't included.

@@ -432,7 +432,10 @@ protected function handleRequest(?RouteCollectionInterface $routes, Cache $cache
return $returnResponse ? $possibleResponse : $possibleResponse->pretend($this->useSafeOutput)->send();
}

if ($possibleResponse instanceof Request) {
if (
$possibleResponse instanceof IncomingRequest
Copy link
Member

Choose a reason for hiding this comment

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

While I think it unlikely, what if someone had created their own Request class? LIke perhaps a more PSR-friendly version. What is the benefit of this change other than limiting those options for devs?

@lonnieezell
Copy link
Member

lonnieezell commented Jul 11, 2022

I know I typically stay out of these, and the arguments have already been made, but I agree that if we do it we should make it at least 4.3. Personally, I'm more for adding the methods to the interfaces, if they're actually needed by all implementing classes, and keeping the flexibility and extendability in the framework. That's kind of the whole point behind Services, which become a bit irrelevant if if you can't actually replace framework classes with your own implementations. I know you can extend the class and still get around it but that feels messier to me.

The other option is take a nuke to it all and get rid of interfaces that don't currently have more than one implementation because if we're hard-coding dependencies, they don't matter anymore. But that seems like a worse solution.

@MGatner
Copy link
Member

MGatner commented Jul 11, 2022

I'm fine with this alternate approach of expanding interfaces to cover the actual method dependencies. This prevents a breaking change for extensions of the class depending on the interface. Also if someone has actually provided their own class fulfilling the interface they have likely already added these methods due to "method not exists" errors from our disjoint.

@kenjis
Copy link
Member Author

kenjis commented Jul 11, 2022

I created this PR, because it seems we have a rule that we change our Interfaces only in major version up.

@kenjis
Copy link
Member Author

kenjis commented Jul 12, 2022

I sent a PR to fix ValidationInterface with the alternate approach of expanding interfaces to cover the actual method dependencies. See #6253

@MGatner
Copy link
Member

MGatner commented Jul 12, 2022

we have a rule that we change our Interfaces only in major version

I don't think this rule is any more important than general breaking changes. In these instances where signatures use interfaces but rely on extra methods I suspect that altering the interfaces will have less impact since most developer classes that fulfill the interfaces will be extensions of our classes already. Even if they aren't a developer who tried using their own implementation would quickly find that they had to add the extra methods already.

@MGatner MGatner mentioned this pull request Sep 3, 2022
5 tasks
@MGatner
Copy link
Member

MGatner commented Sep 16, 2022

The rest of this is waiting to see how the ValidationInterface changes are received in 4.3

@codeigniter4 codeigniter4 deleted a comment from martinemily3 Oct 30, 2022
@MGatner
Copy link
Member

MGatner commented Oct 30, 2022

@kenjis Is there any part of this that should still be considered for 4.3?

@kenjis kenjis marked this pull request as draft October 30, 2022 12:23
@kenjis
Copy link
Member Author

kenjis commented Oct 30, 2022

@MGatner Ignore this PR; there are no further fixes I wish to include in 4.3.
In fact, I believe quite a bit has already been fixed in 4.3.

@kenjis kenjis mentioned this pull request Nov 4, 2022
@kenjis kenjis closed this Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 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.

4 participants