In #228594: UMN Usability: split access rules into an optional module a small performance improvement was introduced. Instead of having to look up the user's IP address in the database for every request, the site owner can specify the list of IP addresses in settings.php - possibly just an empty array.

This is a simple change that seems like a reasonable candidate for inclusion into D6.

CommentFileSizeAuthor
#6 blocked_ips-3.patch1.1 KBc960657
#4 blocked_ips-2.patch1.74 KBc960657
blocked_ips-1-D6.patch2.27 KBc960657

Comments

catch’s picture

catch
he/him
Primary language English
commented

Subscribe. It'd be worth adding this as a merge request to Pressflow as well.

c960657’s picture

c960657 commented

Note that the query does not make good use of MySQL's query cache, because the client's IP address is different for each client. So even though this is just one query per request, it is a query that is often not cached in the database.

c960657’s picture

c960657 commented
Issue tags: +Performance

Tagging.

c960657’s picture

c960657 commented
StatusFileSize
new blocked_ips-2.patch1.74 KB

Rerolled with Git.

rjbrown99’s picture

rjbrown99 commented

Question about #4. It seems to check for a variable blocked_ips, and then does the access check against that array. The variables are all called blocked_ips, but the original function checks for three different $type possibilities: host, mail, or user.

It would seem that you could still potentially accomplish that with an array, but the variable names are a bit misleading. Perhaps renaming to blocked_access or something similar, with examples for each different $type.

c960657’s picture

c960657 commented
StatusFileSize
new blocked_ips-3.patch1.1 KB

The variables are all called blocked_ips, but the original function checks for three different $type possibilities: host, mail, or user.

You are right. The intention was only to support the "host" option with the blocked_ips array (like in D7), because _drupal_bootstrap() calls drupal_is_denied('host', ip_address()) every request.

Here is an updated patch that checks that $type == "host".

rjbrown99’s picture

rjbrown99 commented

Thanks, question though: you COULD pop host, mail, or user into that same array couldn't you? For example, in my case I'm not using this at all so I'd rather just pass in an empty array for all of it, or manually maintain my array in the conf file if needed.

c960657’s picture

c960657 commented

Yes, that would be possible, though the performance improvement for the other two would be smaller, because the function is not invoked with those $type in every request. I think adding support for those two also would be a bit more than just a backport of the D7 feature.

rjbrown99’s picture

rjbrown99 commented

Aaah, I now understand why you are only doing 'host' and not the others. RTFC - bootstrap.inc (from Pressflow below, probably the same or similar):

    case DRUPAL_BOOTSTRAP_ACCESS:
      // Deny access to hosts which were banned - t() is not yet available.
      if (drupal_is_denied('host', ip_address())) {
        header('HTTP/1.1 403 Forbidden');
        print 'Sorry, '. check_plain(ip_address()) .' has been banned.';
        exit();
      }
      break;

The drupal_is_denied call is only made with 'host', so the enclosed patch only really needs to account for that to obtain the speed improvement.

Just posting back in case someone else finds this thread.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.