Better support for NoSQL drivers

[najdanovicivan]

Description

• BaseConnection - Query class determined automatically if Driver specific Query class exist
• Removed strong type of string for $sql param in methods since NoSQL mostly use arrays

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

As stated, changing the signature may be breaking for existing implementations.

[kenjis]

Does the var name $sql make sense in the context of NoSQL?

[najdanovicivan]

The better name for the context will be $statement

[najdanovicivan]

As both QueryClass and $sql string type are BC I'm gonna move the debug toolbar stuff in separate PR

[najdanovicivan]

DebugToolbar changes are moved to separate pr #4077

[paulbalandan]

To preserve BC, how about using a delimiter-based solution to the $sql string param? Then use explode. It may not be optimal but that's the least our option. For NoSQL implementations with array SQL, we can use implode first then this one will explode it.

[paulbalandan]

Existing issue: #2969

So it is either preserve BC or push with this one with proper upgrading instructions.

[najdanovicivan]

@paulbalandan I've tried doing it using jsonEncoded array but this all makes it very annoying to write queries as you have to write query as array then encode it each time query is called.

The solution here is much more elegant. The only issue is how to get around the BC.

IMO it's completely fine merging this one in 4.1 and adding a section in docs on how to do the update. I don't have any clue how many custom driver implementation exist but I don't think at this point there will be a lot of them. So changing this early on and giving users a clear instructions about the change so that we can have better code makes it worthy of a BC change than doing a lot of nasty patches in the future.

This will also completely resolve the #2969

[najdanovicivan]

Also Query Class handling can be resolved without doing a BC by making the query class empty string instead of the hardcoded default value.

I can split the Query Class stuff into separate PR and revisit this one once it's merged

[michalsn]

Personally, I'm good with these changes as long as they will be merged to the 4.1 branch with proper instructions for migration and changelog.

I think that the alternative of having some helper function to encode/decode the strings every time would be really bad in a long run.

[najdanovicivan]

Ok. I'll go ahead and split the Query Class stuff outside of this one so it can be merged

[najdanovicivan]

Here's the PR for QueryClass #4089 Handling once this one's merged I'll rebase this one on the development

[MGatner]

Now that the Query class is merged, and before you go through the rebase trouble, I had an idea that does not circumvent the breaking nature but might help.

Can we define a database-specific Statement class? At its simplest, BaseStatement could be a protected property with __toString() to be compatible with the current SQL drivers, but this would free up NoSQL drivers (or PDO, or whatever) to define more complex statement directives to be handled by Query. Does that get too fancy?

[najdanovicivan]

@MGatner This is not a bad idea but it will require updating all the existing drivers. I'll take a look into it to see what can be done.

[paulbalandan]

@najdanovicivan the Stringable class concept of @MGatner is allowed on string typehints. Maybe we can use that instead to avoid the BC. See https://3v4l.org/LnIRE

[MGatner]

Wow for real, I thought that was one PHP 8 but your 3v4l seems to suggest it was previous versions as well. In that case we should be in awesome shape, just need a well-thought Statement class/base/interface/something.

[najdanovicivan]

I'll try doing it with BaseStatement class. But in such case we should replace all the exiting Implementations with it. I don't like mixing objects with string. In such case all methods should have type of BaseStatement which will again be BC. But having class for query statements will be very useful especially for NoSQL databases.

[najdanovicivan]

@paulbalandan, @MGatner As far as things go with Stringable it will be accepted as parameter but it will automatically be casted to string by invoking __toString method which make it of very little use. The only idea that comes to my mind is to make __toString method return the serialize data and then unserialize it where object is needed but that can also be achieved with json or any other string which is the thing I wanted to avoid here. I want an object to be passed to the execute method in the Connection class.

[najdanovicivan]

In any case the type hint adds problems. Additionally if type of the query is changed to be Stringable it will not accept regular strings. And TypeError will be thrown

Argument 1 passed to CodeIgniter\Database\BaseConnection::query() must implement interface Stringable, string given

[MGatner]

Any class with a __toString() method will be accepted for functions that require a string parameter - Stringable is actually more restrictive than string in this regard.

[kenjis]

mixed means anything. It does not help for code reading.

Can you change it to string|array?

[najdanovicivan]

It may be even needed to use object for such so it can be string|array|object but the main problem is type hint in the method definition. Since merging this one will break all the exiting user implementation where any of these methods have override.

[lonnieezell]

This has been 3 months with no activity. Is this still a desired feature @MGatner @samsonasik @paulbalandan ?

[MGatner]

It's kind of a deadlock. CI4 works very poorly with NoSQL databases, but the changes we would need to make to add support are all breaking (though small).

For my NoSQL project (Firestore) I ended up ditching the database layer altogether. My hunch is that because NoSQL implementations don't share a common language that they will be much harder to make "driver-agnostic" and may not be worth integrating into a unified database layer. If that is that case then the focus should be on making Model and Entity classes to maximize support.

But I've also worked with @najdanovicivan on some of these NoSQL implementations and he has done a lot of work and had some success, I respect his opinions on the matter.

[lonnieezell]

I've only used a NoSQL db once, though it was on a pretty large application. My experience then led me to believe the workflow is too different in all but the simplest of cases, to be standardized into a driver-agnostic solution. There are portions that could be included in a NoSQL layer, though I think things would get pretty complex and convoluted if you were to attempt to rearchitect that. Personally, I'm all for simplifying and keeping each in their own lane, so to speak.

[najdanovicivan]

I believe this one can be closed. I found a way to get around this limitation it's not the nice looking solution but at least completely avoids the need to modify all of those files.

What I dis was made a custom statement class which implements Stringable where __toString() basically returns serialize($this). So the object can be passed as string to the execute method where it is unserialized to get the query parameters.

Maybe better option will be to have some sort of BaseStatement class but it will still be a BC so there is no way to make this one without introducing a BC.

[paulbalandan]

Closing this for now. Until we can arrive at a solution that meets all of our expectations and reservations, this PR is obviously at a standstill. Thanks for the PR, anyway, @najdanovicivan !