search

berlindb / core

All of the required core code
MIT License
252 stars 27 forks source link

Use table alias/name when building date query #164

Open jkudish opened 7 months ago

jkudish commented 7 months ago

This PR attempts to fix the issue described in #9699

Use the received table name & table alias arguments in the get_sql function to set those as properties on the Date class. Note that get_sql already gets called with those params as per the line below. However the get_sql function oddly wasn't defining those arguments in its definition.

https://github.com/awesomemotive/easy-digital-downloads/blob/715373d18971c8b12e949bb3fd5d6c40aac6c234/includes/database/engine/class-query.php#L1363

Then, when building the SQL query, use the alias or name (only if available) to prepend to the column name in the query. This prevents ambiguity issues when performing the SQL query

This prevents and fixes the issue described in https://github.com/awesomemotive/easy-digital-downloads/issues/9699

I am open to receiving feedback on the solution proposed and working on an updated solution. I am also not exactly sure how to test this change in the context of BerlinDB - but I have tested the fix in EDD as per my previous PR opened at https://github.com/awesomemotive/easy-digital-downloads/pull/9700

Thanks!

robincornett commented 7 months ago

Noting that I can confirm the bug, and the fix works. However, in looking into it for EDD, I

Noting that the meta query, compare query, and date query classes all use this get_sql method, and it looks like they're in pretty similar patterns, at least in how they are invoked, as @jkudish indicated. The difference between the Berlin date query and compare/meta query classes is that the latter two extend the WordPress Core WP_Meta_Query class, and inherit the get_sql behavior from that. Since the date query usage passes the same parameters, but the class is only extending the BerlinDB Base class, it's not inheriting the parameters that it's trying to use.

So I think the fix is good, but probably needs to include the additional/same parameters as the WP_Meta_Query class, as that seems to be the expected usage/pattern (the primary ID column and the main query object).

jkudish commented 7 months ago

Hi @robincornett thanks for the feedback.

I updated get_sql to match the signature from WP_Meta_Query as you suggested. I also removed references to table_alias since WP_Meta_Query does not use it, and it's not necessary as table_name can be used for the actual DB query. I also adjusted the @since that I had added to match the BerlinDB version and not the EDD version from the original PR ;)