Querying for a null value casts query to 0
Closed this issue · 5 comments
I've run into this with EDD, which I know is not using the latest BerlinDB code, but I looked for what I expected to find in the latest Berlin code and am not finding it so I'm opening an issue.
Basically, in EDD 3.0, I expect to be able to run a query like this:
edd_get_order_items(
array(
'price_id' => null,
)
);And get an array of order item objects which have a NULL price ID. However, Berlin is returning an array of objects with a 0 price ID instead, which is a very different thing in EDD.
Looking at the generated SQL query for counting order items with a NULL price ID, what's actually running is this:
SELECT COUNT(*)
FROM wp_edd_order_items edd_oi
WHERE edd_oi.order_id = 48
AND edd_oi.product_id = 28
AND edd_oi.price_id = 0
AND edd_oi.status IN ('complete', 'partially_refunded')
The SQL query really should be this instead:
SELECT COUNT(*)
FROM wp_edd_order_items edd_oi
WHERE edd_oi.order_id = 48
AND edd_oi.product_id = 28
AND edd_oi.price_id IS NULL
AND edd_oi.status IN ('complete', 'partially_refunded')
I'm proposing getting around this in one scenario by not including the price ID if it's null, but it feels problematic to have to think to do that: awesomemotive/easy-digital-downloads#9182
Does the current release of BerlinDB allow the kind of query we want? I feel like it should, so if it's not already built in, I'm formally requesting it.
Hello @robincornett! 👋 I've done a bit of cursory research, and I believe your findings are likely to be correct.
See:
Lines 1170 to 1190 in da041d6
Best I can tell, there is not explicit support for the condition when allow_null is true and default is null, like there is in Compare via the Meta query.
Totally doable to add that in, if that's the problem!
Yes, I think that's it. In our case, the default price_id value is NULL (if a product does not have variable pricing enabled), or it will be a numeric value. I think so far, most queries that we have will remove the price_id parameter or set it to an empty string if querying a specific product, but for validating a download URL, we do pass both the product and price ID to Berlin for evaluating, and when the price ID is null, it fails, so for now, I'm removing it. But that feels like something I don't want to have to remember to do before setting up the parameters to send to Berlin. Thank you for looking at it!
That does make sense. 🧐
Being the opinionated piece of software that it is 😬 I originally coded Berlin with some rigid constraints – like the one that you are seeing here 😅
When I'm designing an "ideal" (and descriptive) database schema, one of the things I try my best to avoid is the exact situation you are describing (where a different type of database value – null vs. numeric – corresponds to some hidden application meaning – variable pricing: "yes" vs "no").
Ideally, your price_id column value would be default 0 to signify: variable pricing: "no" and any other numeric value would be "yes" and leaving it up to your application logic to further determine what that corresponds to.
Relatedly, in WordPress core, allow null is only used in metadata tables on the meta_key and meta_value columns. You'll notice none of the object tables (comments, options, posts, terms, users) allow null values, and that is explicitly to avoid "mixed types" where some part of the PHP application expects a string or int but unexpectedly ends up with a null.
As PHP "improves" these data types are getting more strict, hence more important to get right, and I think part of that goal is preventing them from changing wherever possible, particularly when fallbacks or defaults perform that magic without any real human interaction on it.
All of that being said, I do still believe that it is worth improving how values are parsed – specifically to better support allow null and default null – as future metadata table support will depend on it. 💫
Ideally, 0 might mean that variable pricing is not enabled, but since computers start counting arrays at 0, older stores (or those using FES) will certainly have variable prices with a 0 ID. We are trying to move away from that, but since it exists, we are working to try to be consistent in making sure that when we use 0, that it's definitely referencing a price ID, and null means that there isn't a price ID. Before 3.0, 0 can mean either, and it takes some extra digging into the download metadata to know which it is.
If there is anything I can do to help on this please let me know! Thank you for looking into it.