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

Use table alias/name when building date query #164

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jkudish
Copy link

@jkudish jkudish commented Dec 5, 2023

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 awesomemotive/easy-digital-downloads#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 awesomemotive/easy-digital-downloads#9700

Thanks!

use the received table name & table alias arguments in the `get_sql` function to set those as properties on the Date class.

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 and fixes the issue described in awesomemotive/easy-digital-downloads#9699
@robincornett
Copy link

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).

Removed the table alias as that's not something WP_Meta_Query uses
@jkudish
Copy link
Author

jkudish commented Dec 6, 2023

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 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants