Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
So I am happily hacking away on my memcache based Drupal fork (dont worry not a big fork) and find a JOIN. And ... well, it's not needed.
Comment | File | Size | Author |
---|---|---|---|
#3 | 374568-unneccessary-join-D7.patch | 1.18 KB | Dave Reid |
uselessjoin.patch | 813 bytes | chx | |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedIndeed.
Comment #2
Dave ReidConfirmed this is absolutely unneeded. Good find chx. :) Should be backported to 6.x as well.
Comment #3
Dave ReidRevised patch also gets rid of the deprecated db_fetch_array() while we're in there.
Comment #4
chx CreditAttribution: chx commentedBack to RTBC as the bot is happy.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedI would like to highlight that the new query is not exactly the same as the old one:
Returns {role_permission} rows for roles in :fetch that have a corresponding row in the {role} table.
Returns {role_permission} rows for roles in :fetch, regardless if they have a corresponding row in the {role} table.
In a nutshell, it means that if the database doesn't have a proper relational integrity between {role} and {role_permission} (for example: the admin manually removed a row from {role}, the result can be different).
Lastly, this additional join has zero performance impact. So, do we really want to remove it?
Comment #6
chx CreditAttribution: chx commentedHow would a user end up with a role that's not in the role table? You need to mess your DB badly to change the meaning of the query.
Comment #7
Dave ReidIf the user is messing with the DB, there are *tons* of places where things could get mucked up, so I don't think should be a concern. Plus I'm working on improving the user role and permissions API in #300993: User roles and permissions API.
Comment #8
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #9
Dave ReidMoving back for 6.x.
Comment #10
halka-dupe CreditAttribution: halka-dupe commenteduselessjoin.patch queued for re-testing.
Comment #11
halka-dupe CreditAttribution: halka-dupe commenteduselessjoin.patch queued for re-testing.
Comment #12
halka-dupe CreditAttribution: halka-dupe commenteduselessjoin.patch queued for re-testing.
Comment #13
halka-dupe CreditAttribution: halka-dupe commented#3: 374568-unneccessary-join-D7.patch queued for re-testing.
Comment #15
halka-dupe CreditAttribution: halka-dupe commenteduselessjoin.patch queued for re-testing.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm going to guess this is a won't fix for D6, since D6 commits focus on security fixes at this point.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedChanging issue status to reflect that it was fixed in 7.x.