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

Bug: BaseBuilder's query is wrong #3659

Closed
InsiteFX opened this issue Sep 19, 2020 · 4 comments
Closed

Bug: BaseBuilder's query is wrong #3659

InsiteFX opened this issue Sep 19, 2020 · 4 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@InsiteFX
Copy link
Contributor

Newest CodeIgniter Developer version 4

BaseBuilder.php - The query method is being passed 3 parameters but the ConnectionInterface.php is only using 2 parameters.

Example:
system/Database/BaseBuilder.php
Line #1860
: $this->db->query($this->compileSelect(), $this->binds, false); <-- 3 parameters are being passed

system/Database/ConnectionInnterface.php

public function query(string $sql, $binds = null); <-- Only 2 signature parameters are defined

This is affecting every query method in BaseBuilder.php

  • OS: Windows 10 Pro x64
  • Web server Apache 2.4
  • PHP version 7.4.10
@InsiteFX InsiteFX added the bug Verified issues on the current code behavior or pull requests that will fix them label Sep 19, 2020
@michalsn
Copy link
Member

Technically speaking it's not a bug since we still respect the contract in the BaseConnection class (BaseConnection, ConnectionInterface). If the method's parameters have a default value then you don't have to specify them in the interface - they become an optional parameters and it gives you a lot of possibilities.

In this case, I'm not really sure if we want to have all the parameters or are they missing by accident (probably?). However, the current implementation doesn't cause any malfunction.

From what I see this "pattern" occurs also in other database related interfaces (PreparedQueryInterface), so I will call @MGatner to discuss this more... should we keep the interface as it is or change it? I'm not sure if this may cause any problems for future drivers so there is a possiblility it was intended.

@InsiteFX
Copy link
Contributor Author

No Problem, phpStorm flags like that. I guess it is just a warning from phpStorm.

@MGatner
Copy link
Member

MGatner commented Sep 25, 2020

Probably PhpStorm hasn't scanned all the classes correctly. From PHP.net on interfaces:

The class implementing the interface must use a method signatures which is compatible with LSP (Liskov Substitution Principle). Not doing so will result in a fatal error.

Basically it is okay for a class to build on an interface definition as long as it is not violating it.

@michalsn I think that some of these parameters were added later and thus not included in the interface. Whether that was by mistake or for backwards-compatibility I can't say, but for now I think we leave the less-restrictive interface. At some point (version 5? haha) I would like to see the whole database namespace get overhauled to be a) more loosely interoperative and b) less specific to SQL... that would likely be the time to redefine our interfaces or, more likely, to differentiate them.

@MGatner MGatner closed this as completed Sep 25, 2020
@michalsn
Copy link
Member

Yes, I'm not keen to make changes to the interfaces at this point either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

No branches or pull requests

3 participants