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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

Title: Useless JOIN » Useless JOIN - loading role
Status: Needs review » Reviewed & tested by the community

Indeed.

Dave Reid’s picture

Issue tags: +Needs backport to D6

Confirmed this is absolutely unneeded. Good find chx. :) Should be backported to 6.x as well.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.18 KB

Revised patch also gets rid of the deprecated db_fetch_array() while we're in there.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as the bot is happy.

Damien Tournoud’s picture

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?

chx’s picture

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.

Dave Reid’s picture

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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Dave Reid’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Moving back for 6.x.

halka-dupe’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D6

uselessjoin.patch queued for re-testing.

halka-dupe’s picture

uselessjoin.patch queued for re-testing.

halka-dupe’s picture

uselessjoin.patch queued for re-testing.

halka-dupe’s picture

#3: 374568-unneccessary-join-D7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 374568-unneccessary-join-D7.patch, failed testing.

halka-dupe’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6

uselessjoin.patch queued for re-testing.

Anonymous’s picture

Status: Needs review » Closed (won't fix)

I'm going to guess this is a won't fix for D6, since D6 commits focus on security fixes at this point.

Anonymous’s picture

Version: 6.x-dev » 7.x-dev
Status: Closed (won't fix) » Closed (fixed)

Changing issue status to reflect that it was fixed in 7.x.