Updated: Comment #5

Problem/Motivation

Class names in PHP, unlike variable names, are not case sensitive. See class_exists(). However, the code registry check for class names is case sensitive.

Proposed resolution

Use LIKE for code registry comparison.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Darren Oh’s picture

Title: Code registry fails to load lower case class names » Ignore case in code registry lookups
Assigned: Darren Oh » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
5.12 KB

Added patch and updated title.

Darren Oh’s picture

Issue summary: View changes
TaraRowell’s picture

+1 Following

TaraRowell’s picture

Status: Needs review » Reviewed & tested by the community

Up and running smoothly in current site application.

Darren Oh’s picture

Issue summary: View changes

Remaining tasks have been completed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: drupal-autoload-2170453.patch, failed testing.

Darren Oh’s picture

Status: Needs work » Needs review

1: drupal-autoload-2170453.patch queued for re-testing.

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: drupal-autoload-2170453.patch, failed testing.

Darren Oh’s picture

Status: Needs work » Needs review

1: drupal-autoload-2170453.patch queued for re-testing.

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

The test system keeps failing, but the patch has passed testing twice.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: drupal-autoload-2170453.patch, failed testing.

Darren Oh’s picture

Status: Needs work » Needs review

1: drupal-autoload-2170453.patch queued for re-testing.

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Bad testbot!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

This function looks like it can be called frequently, and I think LOWER can be slow... do we know if there's any performance impact here?

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

The code registry is cached, so the lookup is only run when the cache needs to be updated.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Right, but after a cache clear there will be a lot of queries running against this table, and each adds up on what's already a slow situation.

I'm not sure the table ever gets big enough for this to matter that much, but currently we have an index on the 'name' column, and switching to a LOWER() query effectively is the same as removing the index.

Also, is this only an issue on certain databases or database configurations? On my default MySQL installation this whole issue never occurs because the database deals with the case-insensitivity on its own:

mysql> select name from registry where name = 'drupalQueue';
+-------------+
| name        |
+-------------+
| DrupalQueue |
+-------------+
1 row in set (0.00 sec)

If you try to use the "drupalQueue" class in your code there, it all works fine.

PostgreSQL may be different, but I'm pretty sure other parts of the code use "X LIKE Y" rather than "LOWER(X) = Y" for this kind of thing (and rely on Drupal's PostgreSQL driver to force the LIKE comparison to be case-insensitive, which it should). That's one of the reasons Drupal 7 doesn't have any LOWER() calls at all these days.

Darren Oh’s picture

This is what you seem to be suggesting. I was able to confirm that string comparisons are not case sensitive in a default MySQL configuration.

Status: Needs review » Needs work

The last submitted patch, 18: drupal-autoload-2170453-17.patch, failed testing.

Darren Oh’s picture

The last submitted patch, 18: drupal-autoload-2170453-17.patch, failed testing.

Darren Oh’s picture

1: drupal-autoload-2170453.patch queued for re-testing.

Darren Oh’s picture

The last submitted patch, 18: drupal-autoload-2170453-17.patch, failed testing.

The last submitted patch, 18: drupal-autoload-2170453-17.patch, failed testing.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
2.17 KB

One little tweak, needed a db_like().

Status: Needs review » Needs work

The last submitted patch, 27: drupal-autoload-2170453-27.patch, failed testing.

mikeryan’s picture

Well, I have no idea why those tests succeeded without db_like() and failed with it, all it does is escape wildcard characters (of which there are none in the class/interface names being tested)...

mikeryan’s picture

Status: Needs work » Needs review
FileSize
4.16 KB

Duh - was just looking at the assertion failures and missed that drupal_autoload_test failed to enable - because I missed it in the patch. Let's try that again...

Darren Oh’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This is working on my PostgreSQL install and passes MySQL tests.

mikeryan’s picture

Status: Reviewed & tested by the community » Needs review

Sorry Darren, as the patch author (I only added a small tweak) it's not appropriate for you to rtbc, we need someone else (like @nevergone or @Gold) to review and confirm it.

Gold’s picture

Status: Needs review » Reviewed & tested by the community

Starting with a clean Drupal 7.x-dev the patch drupal-autoload-2170453-30.patch applies fine for me.

Installing with the minimal profile + the Testing module worked fine.

Running the Bootstrap tests everything came back green except one test in the Page cache test and I don't think that's related to the changes made here.

I would say this is RTBC.

Fabianx’s picture

RTBC + 1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: drupal-autoload-2170453-30.patch, failed testing.

Status: Needs work » Needs review
Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community
Fabianx’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/bootstrap.inc
@@ -3151,10 +3151,11 @@ function _registry_check_code($type, $name = NULL) {
-  $file = Database::getConnection('default', 'default')->query("SELECT filename FROM {registry} WHERE name = :name AND type = :type", array(
-      ':name' => $name,
-      ':type' => $type,

We should keep this.

Using db_select is not the right solution as this can be called earlier ...

Darren Oh’s picture

We can’t keep that. That code causes the bug we are fixing.

Darren Oh’s picture

Status: Needs work » Needs review
Darren Oh’s picture

Darren Oh’s picture

FileSize
795 bytes
5.43 KB

The last submitted patch, 41: drupal-autoload-2170453-41.patch, failed testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

That looks better to me :). Thanks!

Really RTBC now.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
+  // Ensure that the string comparison is not case sensitive.
+  $operator = $connection->mapConditionOperator('LIKE');
+  if (!isset($operator)) {
+    $operator = array('operator' => 'LIKE');
+  }
+  $file = $connection->query("SELECT filename FROM {registry} WHERE name $operator[operator] :name AND type $operator[operator] :type", array(

I'd love to get this patch in, but I see no requirement anywhere that the mapConditionOperator() method return an array with an 'operator' key in it... and here is one place that doesn't: http://cgit.drupalcode.org/sqlsrv/tree/sqlsrv/database.inc?id=9a2e1c3482...

See DatabaseCondition::compile() for how all this stuff gets called.

I guess it's possible to change this patch to fix that, but wow... I had no idea how much trouble the LIKE vs ILIKE stuff would be. The fact that db_select() automatically switches LIKE to ILIKE on postgres but db_query() doesn't isn't shocking when you think about it, but really seems like it should be documented loudly somewhere :)

Anyway, is that a possible argument for going back to #30? That would be a much simpler approach. I am not sure offhand what the performance overhead of going from db_query() to db_select() in this part of the code would be; it would need to be investigated a bit further, but to the extent that it only runs on cache misses I suppose it can only be so bad.

Darren Oh’s picture

I’m in favor of #30. I didn’t believe there was any problem with using db_select() for this. Just needed to satisfy those whose objections were preventing this bug from being fixed.

Fabianx’s picture

db_select() is fine, but then the comment stating that this can be called really early and such Database::getConnection() needs to be called directly should be removed as it is misleading then.

I think its old knowledge anyway, as all that db_select does is:

https://api.drupal.org/api/drupal/includes!database!database.inc/functio...

OH

I see the problem:

https://api.drupal.org/api/drupal/includes%21database%21database.inc/fun...

uses

$key = self::$activeKey;

so this won't work.

Proposed resolution is:

Database::getConnection('default', 'default'])->select($table, $alias, $options);

So calling ->select instead of query, but on the default DB connection.

which is kinda strange (as it means you cannot put registry to another DB), but at least known working as it was before.

So I think that would be a good compromise.

Thanks!

mikeryan’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

OK, here we go with the #30 patch plus the explicit connection selection per @Fabianx.

mikeryan’s picture

Passed tests - is it kosher to return to rtbc, or should someone (e.g., Gold) who hasn't contributed to the patch retest first?

Darren Oh’s picture

As the author of the latest version of the patch, you should wait for someone else to review it. It doesn’t have to be someone who did not contribute to the patch. Previous versions of the patch were marked RTBC by several different people.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Here we go, back to RTBC

Darren Oh’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: drupal-autoload-2170453-48.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: drupal-autoload-2170453-48.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

That was a bogus test failure, so back to RTBC for now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: drupal-autoload-2170453-48.patch, failed testing.

Status: Needs work » Needs review
Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: drupal-autoload-2170453-48.patch, failed testing.

Status: Needs work » Needs review
Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: drupal-autoload-2170453-48.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: drupal-autoload-2170453-48.patch, failed testing.

Status: Needs work » Needs review
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: drupal-autoload-2170453-48.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
-  $file = Database::getConnection('default', 'default')->query("SELECT filename FROM {registry} WHERE name = :name AND type = :type", array(
-      ':name' => $name,
-      ':type' => $type,
-    ))
+  $file = Database::getConnection('default', 'default')
+    ->select('registry', 'r', array('target' => 'default'))

Not sure the 'target' => 'default' part is necessary (isn't that the normal behavior?) but if I'm right it's harmless, and if I'm wrong there's probably a good reason for it, so this looks good to go :)

Committed to 7.x - thanks! Thanks for the perseverance on this one.

I fixed a couple small things on commit (added a code comment explaining why LIKE is used, changed the class description to be more specific since there are other registry tests elsewhere, etc):

--- a/includes/bootstrap.inc
+++ b/includes/bootstrap.inc
@@ -3170,6 +3170,7 @@ function _registry_check_code($type, $name = NULL) {
   $file = Database::getConnection('default', 'default')
     ->select('registry', 'r', array('target' => 'default'))
     ->fields('r', array('filename'))
+    // Use LIKE here to make the query case-insensitive.
     ->condition('r.name', db_like($name), 'LIKE')
     ->condition('r.type', $type)
     ->execute()
diff --git a/modules/simpletest/tests/bootstrap.test b/modules/simpletest/tests/bootstrap.test
index 5c5a72a..ece1cd9 100644
--- a/modules/simpletest/tests/bootstrap.test
+++ b/modules/simpletest/tests/bootstrap.test
@@ -289,7 +289,7 @@ class BootstrapVariableTestCase extends DrupalWebTestCase {
 }
 
 /**
- * Test code registry.
+ * Tests the auto-loading behavior of the code registry.
  */
 class BootstrapAutoloadTestCase extends DrupalWebTestCase {
 
@@ -297,7 +297,7 @@ class BootstrapAutoloadTestCase extends DrupalWebTestCase {
     return array(
       'name' => 'Code registry',
       'description' => 'Test that the code registry functions correctly.',
-      'group' => 'Bootstrap'
+      'group' => 'Bootstrap',
     );
   }

  • David_Rothstein committed ea48131 on 7.x
    Issue #2170453 by Darren Oh, mikeryan, Fabianx: Ignore case in code...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Kristen Pol’s picture

Had to revert this patch to get rid of fatal SelectQuery_mysql error per:

#2471783: After upgrade 7.35 -> 7.36 php fatal error - SelectQuery_mysql

which helped others as well.

leochid’s picture

The above code change is creating issue for us. The above patch triggers module_hook_info in DRUPAL_BOOTSTRAP_FULL phase, before all the modules are included and there by missing to include functions like system_hook_info which contains token_info etc...

we are using varnish,memcache,eck and autoslave module. our instance has one master and two slave database, and settings.php has configuration which tells autoslave module, which database to switch if some thing goes wrong.

we have upgraded our drupal version from 7.34 to 7.43, then onwards our search page and token's stoped working at random. When we disable autoslave configuration in settings.php and flush the cache it starts working and after some time the error again surfaces. The error log throws below error when search page is accessed

" PHP Fatal error: Class name must be a valid object or a string in /var/www/mono.itp.com/htdocs/sites/all/modules/contrib/facetapi/plugins/facetapi/adapter.inc on line 863"

when we insert or edit a new content, we got warning like "The Page title is using the following invalid tokens: [node:title], [node:node_sections_comma_seperated], [node:node_tags_comma_seperated], [site:name]."

In status page we have below warning

Tokens Problems detected
The following tokens or token types are missing required name and/or description information:
$info['types']['date']
$info['tokens']['node']['url']
$info['tokens']['file']['size']
$info['tokens']['user']['url']
$info['tokens']['date']['custom']
The following token types are not defined but have tokens:
$info['types']['node']
$info['types']['term']
$info['types']['vocabulary']
$info['types']['file']
$info['types']['user']
$info['types']['current-user']

We tried to replicate the issue and we have found that, the issue occurs if memcache is cleared and site is accessed immedietly after the memcache is cleared. We have looked in to code and we have found that autoslave is adding tag in the select function. when drupal loads all the module in DRUPAL_BOOTSTRAP_FULL phase, and while eck module is loading, it triggers spl_autoload_call('Entity') which inturn triggers drupal_autoload_class('Entity'),_registry_check_code('class', 'Entity'). The code that triggering the function is below

"class ECKEntity extends Entity {

}"

Below is the backtrace

string(1149) "#0 htdocs/includes/module.inc(768): module_hook_info()
#1 /htdocs/includes/module.inc(1079): module_implements('query_alter')
#2 /htdocs/includes/database/select.inc(1238): drupal_alter(Array, Object(SelectQuery))
#3 /htdocs/includes/database/select.inc(1260): SelectQuery->preExecute()
#4 /htdocs/includes/bootstrap.inc(3221): SelectQuery->execute()
#5 /htdocs/includes/bootstrap.inc(3120): _registry_check_code('class', 'Entity')
#6 [internal function]: drupal_autoload_class('Entity')
#7 /htdocs/sites/all/modules/contrib/eck/eck.module(767): spl_autoload_call('Entity')
#8 /htdocs/includes/bootstrap.inc(1120): include_once('/var/www/mono.i...')
#9 /htdocs/includes/module.inc(24): drupal_load('module', 'eck')
#10 /htdocs/includes/common.inc(5234): module_load_all()
#11 /htdocs/includes/bootstrap.inc(2266): _drupal_bootstrap_full()
#12 /htdocs/index.php(20): drupal_bootstrap(7)
#13 {main}"

In the above backtrace, you can find that the execute() function triggers query_alter which, inturn triggers module_hook_info(). What the function does is it searches for system_hook_info function, before the system.module is included, and since it does not find system_hook_info, that functionality like token_info etc.. is lost. if we revert, this problem is not occuring. Please suggest, shall we use #42 patch instead of #30.

David_Rothstein’s picture

@leochid - that shouldn't be happening. SelectQuery::preExecute only calls drupal_alter() when $this->alterTags is set (i.e. when the database query has been tagged with the addTag method described at https://www.drupal.org/node/310077). That's not the case for the query in _registry_check_code(), and when I debugged it now drupal_alter() wasn't called for me.

In any case, this issue was closed a long time ago, so I would suggest creating a new issue if you still have a problem. But ideally before doing that you should check and try to figure out why $this->alterTags is populated for that query on your particular site (and what it's populated with)... it seems like it could be a site-specific problem.