Skip to content

Conversation

@paulbalandan
Copy link
Member

Description
These are already defined by ConnectionInterface.

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 refactor Pull requests that refactor code label Oct 19, 2022
@MGatner
Copy link
Member

MGatner commented Oct 19, 2022

It feels a little weird to have implements ConnectionInterface without these methods. Normally I would look at an abstract class to see what I need to fulfill as far as the contract, which won't work in this case. I understand the deduplication, just noting that it feels a bit odd.

Does anyone have examples of this somewhere else (even outside of CodeIgniter)? Where an abstract class implements the interface without the actual methods?

@paulbalandan
Copy link
Member Author

If you are not using an IDE then I believe it is normal to feel the weirdness.

PHP won't complain the absence of the interface's methods in the abstract class because it is abstract (i.e. incomplete). In that case, PHP will assume the concrete classes will complete fulfilling the interface.

@MGatner
Copy link
Member

MGatner commented Oct 19, 2022

It's an interesting consideration that I haven't come across before. I did a little research and didn't find much but this rather mixed-quality, old conversation thread: https://thuvienphapluat.edu.vn/can-abstract-class-implement-interface-in-php

If you two concur on this approach then I'm good with it.

@MGatner
Copy link
Member

MGatner commented Oct 19, 2022

If you are not using an IDE

I use Intelephense but I don't know that it really matters for this case unless your IDE is somehow generating the initial class. When I'm fulfilling an abstraction I literally copy the definitions (either from the interface or the abstract methods from the abstract class) and work on filling them in. I was just noting that in this case one would have to know to go to the interface in this case, not the abstract class.

@paulbalandan
Copy link
Member Author

This is an example from Symfony Console's InputInterface

InputInterface defines a getFirstArgument method:
https://github.com/symfony/symfony/blob/c2ad569b9f66f6ff0080a93598a4c89682da1cb6/src/Symfony/Component/Console/Input/InputInterface.php#L30

but abstract class Input does not define it nor list it as abstract:
https://github.com/symfony/symfony/blob/c2ad569b9f66f6ff0080a93598a4c89682da1cb6/src/Symfony/Component/Console/Input/Input.php#L28

because the child classes like ArgvInput will fulfill the contract:
https://github.com/symfony/symfony/blob/c2ad569b9f66f6ff0080a93598a4c89682da1cb6/src/Symfony/Component/Console/Input/ArgvInput.php#L263

@paulbalandan
Copy link
Member Author

In my case using intelephense on VS Code if I'm writing a concrete class implementing an interface or extending an abstract class, intelephense will show a red notice that my concrete class needs to define this and that method.

@MGatner
Copy link
Member

MGatner commented Oct 19, 2022

Awesome! Thanks for that research. Yes let's proceed with this.

@kenjis kenjis merged commit 98a4666 into codeigniter4:develop Oct 20, 2022
@paulbalandan paulbalandan deleted the remove-unneeded-baseconnection-abstract-methods branch October 20, 2022 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Pull requests that refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants