codeigniter4
fix: incorrect type declarations
Description I propose to fix incorrect type declarations as a bug fix in 4.x.
Related #4356
Checklist:
- [x] Securely signed commits
- [ ] Component(s) with PHPDoc blocks, only if necessary or adds value
- [ ] Unit testing, with >80% coverage
- [ ] User guide updated
- [x] Conforms to style guide
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?
@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.
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.
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.
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!
I think changing signature should be major, but... if it really needed, should be a next minor 4.3
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.
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. https://github.com/codeigniter4/CodeIgniter4/issues/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.
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.
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.
I created this PR, because it seems we have a rule that we change our Interfaces only in major version up.
I sent a PR to fix ValidationInterface with the alternate approach of expanding interfaces to cover the actual method dependencies. See #6253
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.
The rest of this is waiting to see how the ValidationInterface changes are received in 4.3
@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.