Posted by chx on February 15, 2009 at 12:35am
7 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | user system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | chx |
| Status: | needs review |
| Issue tags: | needs backport to D6 |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| uselessjoin.patch | 813 bytes | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch uselessjoin.patch. | View details | Re-test |
Comments
#1
Indeed.
#2
Confirmed this is absolutely unneeded. Good find chx. :) Should be backported to 6.x as well.
#3
Revised patch also gets rid of the deprecated db_fetch_array() while we're in there.
#4
Back to RTBC as the bot is happy.
#5
I would like to highlight that the new query is not exactly the same as the old one:
SELECT r.rid, p.permission FROM {role} r INNER JOIN {role_permission} p ON p.rid = r.rid WHERE r.rid IN (:fetch)Returns {role_permission} rows for roles in :fetch that have a corresponding row in the {role} table.
SELECT rid, permission FROM {role_permission} WHERE rid IN (:fetch)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?
#6
How 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.
#7
If 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.
#8
Committed to CVS HEAD. Thanks.
#9
Moving back for 6.x.
#10
uselessjoin.patch queued for re-testing.
#11
uselessjoin.patch queued for re-testing.
#12
uselessjoin.patch queued for re-testing.
#13
#3: 374568-unneccessary-join-D7.patch queued for re-testing.
#14
The last submitted patch, 374568-unneccessary-join-D7.patch, failed testing.
#15
uselessjoin.patch queued for re-testing.