Closed (fixed)
Project:
Drupal core
Version:
8.2.x-dev
Component:
database system
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
4 Jun 2013 at 10:08 UTC
Updated:
17 Aug 2016 at 00:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
pancho[edited:] grep reveals that
LEFTRIGHT JOIN is indeed not used in core, unlikeRIGHTLEFT JOIN, but this can still break contrib code for SQLite.Comment #2
dawehnerAre you sure about that? I have some places in mind where at list views uses a left join instead of an inner join.
Comment #3
panchoYou're absolutely right. Must have been tired when writing that - while the analysis was right, the wording wasn't.
Correct is: We're currently not using RIGHT JOINs in Core, but if we did, they would be broken in SQLite.
So we need to make sure, contrib doesn't use RIGHT JOINs or alternatively we need to rewrite them into a LEFT JOIN.
Comment #4
damien tournoud commentedI agree, the database system should just not support right joins. There are really only limited use-case for them anyway, so let's get rid of the support completely.
Comment #5
panchoOK. Easy to remove RIGHT JOIN support in D8.
Now how about D7? Contrib and/or custom code may already use RIGHT JOINs not caring about SQLite compatibility.
To be on the secure side, we'd have to rewrite RIGHT JOINs to LEFT JOINs for D7, but if then we already have written and tested the rewriting code, then we can keep using it in D8 just as well. Or would we rather deprecate RIGHT JOINs anyway from a KISS point-of-view?
Comment #6
damien tournoud commentedRewriting RIGHT JOIN into LEFT JOIN sounds hard, or else SQLite would just do that? Any idea how hard would it really be?
Comment #6.0
damien tournoud commentedfix typo
Comment #7
jhedstromI think removing this would be considered a pretty major API change at this point, even if it fixes a bug with SQLite?
Comment #8
dcrocks commentedI think the intent was to continue to support RIGHT JOIN from contrib by changing the drivers to convert them to LEFT JOIN, so it isn't an api change. There are no RIGHT JOIN's issued in core. And if issues for SQLite and Postgres continue to be pushed back, the harder it gets to fix them. Can the owner of this issue speak up?
Comment #9
jhedstromI think it could still be considered for 8.0 if it isn't actually a major API change as I assumed above.
Comment #11
daffie commentedA
right joinon drupal is aright outer join. So left test if it works on SQLite or not.Comment #12
daffie commentedThe new testRightOuterJoin fails for SQLite with the following error: "SQLSTATE[HY000]: General error: 1 RIGHT and FULL OUTER JOINs are not currently supported".
This is not fixable and needs a management decision what to about it. I see two options:
I am setting the priority to critical because this bug will: "Render a site unusable and have no workaround."
Comment #13
catchThis doesn't render a site unusable with no workaround - you'd need to actually 1. write a query using a right join (which requires a contrib or custom module) 2. have that query run on an site running sqlite 3. have the query run on every page so that it's not possible to uninstall the module.
That's not impossible, but it's very unlikely.
I agree we should deprecate it though, probably with an additional mention that it's broken on sqlite. Can open up a follow-up to try to convert to a left join if that's viable.
Comment #14
daffie commentedPatch for deprecating the SelectInterface::rightJoin() method.
Comment #15
daffie commentedAdded a change record: https://www.drupal.org/node/2765249.
Comment #16
daffie commentedBetter title
Comment #17
daffie commentedComment #18
tstoecklerLooks good to me.
Comment #19
xjmWe generally deprecate code in minor versions, so moving to 8.2.x. (Documenting a deprecation is also RC-eligible.) I tweaked the CR a little bit.
Leaving this issue for a framework manager.
Comment #20
alexpottDeprecating something that one of our supported dbs can't do just makes sense. Committed db9e719 and pushed to 8.2.x. Thanks!
I don't know how the deprecation should be ported to D7 so marking issue as closed.
Comment #22
David_Rothstein commentedI created #2775593: (D7 backport) Deprecate RIGHT JOINs, because SQLite doesn't support them for backporting this to Drupal 7. The "will be removed in Drupal 9.0.0" line is less necessary there perhaps, but otherwise I think the backport would basically be the same. The key point is to indicate that it should not be used for any code that needs to run on SQLite.