-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Description
This is a (multiple allowed):
- bug
- enhancement
Motivation:
- Hidden bugs in software, often times from 2.x to 3.x upgrades
- High level needs to know about things it shouldnt need to know
- Methods like findOrCreate() and using $conditions array for finding + creating is way more complex currently
IN
This has been annoying me since day one of 3.x, and every time I upgrade a 2.x app, this annoys me even more.
The main reason: It contains hidden bugs as the resulting SQL query is invalid but silently just returns 0 results.
So no one ever knows if those queries work since no results just means "no hits" usually, no one would suspect the query to be invalid then, of course.
$x = $this->Logs->find()
->where(['comment' => null])
->all();
SQL:
WHERE
`comment` = NULL
=> 0 rows, silently returning nothing
That makes no sense to me. In what use case would one ever want to produce such a query? It does not throw a DB exception but is an invalid query that will never yield any useful result.
The desired behavior is pretty clear - make the DB check for the fields that are NULL (the developer doesnt want/need to know the specifics of the query building IMO).
$x = $this->Logs->find()
->where(['comment IS' => null])
->all();
and the resulting
WHERE
(`comment`) IS NULL
suddenly returns the expected 3 records.
So two things we can do for 3.6:
Solution A
The obvious one: Throw an exception for invalid queries instead of silently doing nothing creates a false sense of correctness here.
This would be somewhat in line with IN / NOT IN behavior for empty rows.
Solution B
Go back to the very intuitive and IMO most correct way of using 2.x behavior:
Once a null is passed, use that for the query with proper IS or IS NOT usage.
Especially for the positive case (IS) I do not see any harm here - especially compared to the current bugs we have due to the silent query fault.
That said I am fine with adding extra methods that would provide this. But I honestly do not think those should be necessary.
Cake ORM should go back to the dev friendly way of converting this to what the DB needs without having to force the developer to add extras everywhere.
I still have hundreds of these silent issues somewhere in upgraded apps and they are currently impossible to detect really, unless you use 100% test coverage
and always at least fixture data of >= 1 returned (never allow empty result collection for reading in tests). This is not feasable.
Let's please fix this now.
NOT IN
To complete Solution B, also the negatation should then work as expected (or throw meaningful exception as A suggests):
find()->where(['comment !=' => null])->all();
The generated query should never be
WHERE
`comment` != NULL
but
WHERE
(`comment`) IS NOT NULL
Are there any issues of doing this properly inside the ORM? Or do we have to go with the exception because certain use cases such as nested 'NOT' conditions might fail here?
Schema NOT NULL
Implications for when the schema doesnt even allow null values are simple:
We could automatically use empty string then right away (NO IS NULL check) for the condition.
This becomes necessary IMO if we start throwing exceptions - for the other case it would be a useful convenience here, as the other one would never result valid results either.
Nullable data integrety
This also relates somewhat to #9678 since it then could based on schema always just require the right empty value for the condition, and with these changes here require the right IS vs ==/NOT IS vs != usage.
But I can see why some people would not want too much magic here going on based on schema and instead probably prefer some runtime exception or more manual thought put into query building.
But with the related issue being fixed this one here will also become more clear hopefully.