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.
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.
Comment | File | Size | Author |
---|---|---|---|
#48 | drupal-autoload-2170453-48.patch | 4.21 KB | mikeryan |
Comments
Comment #1
Darren OhAdded patch and updated title.
Comment #2
Darren OhComment #3
TaraRowell CreditAttribution: TaraRowell commented+1 Following
Comment #4
TaraRowell CreditAttribution: TaraRowell commentedUp and running smoothly in current site application.
Comment #5
Darren OhRemaining tasks have been completed.
Comment #7
Darren Oh1: drupal-autoload-2170453.patch queued for re-testing.
Comment #8
Darren OhComment #10
Darren Oh1: drupal-autoload-2170453.patch queued for re-testing.
Comment #11
Darren OhThe test system keeps failing, but the patch has passed testing twice.
Comment #13
Darren Oh1: drupal-autoload-2170453.patch queued for re-testing.
Comment #14
Darren OhBad testbot!
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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?
Comment #16
Darren OhThe code registry is cached, so the lookup is only run when the cache needs to be updated.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedRight, 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:
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.
Comment #18
Darren OhThis is what you seem to be suggesting. I was able to confirm that string comparisons are not case sensitive in a default MySQL configuration.
Comment #20
Darren Oh18: drupal-autoload-2170453-17.patch queued for re-testing.
Comment #22
Darren Oh1: drupal-autoload-2170453.patch queued for re-testing.
Comment #23
Darren Oh18: drupal-autoload-2170453-17.patch queued for re-testing.
Comment #27
mikeryanOne little tweak, needed a db_like().
Comment #29
mikeryanWell, 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)...
Comment #30
mikeryanDuh - 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...
Comment #31
Darren OhThis is working on my PostgreSQL install and passes MySQL tests.
Comment #32
mikeryanSorry 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.
Comment #33
GoldStarting 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.
Comment #34
Fabianx CreditAttribution: Fabianx commentedRTBC + 1
Comment #37
Darren OhComment #38
Fabianx CreditAttribution: Fabianx commentedWe should keep this.
Using db_select is not the right solution as this can be called earlier ...
Comment #39
Darren OhWe can’t keep that. That code causes the bug we are fixing.
Comment #40
Darren OhComment #41
Darren OhComment #42
Darren OhComment #44
Fabianx CreditAttribution: Fabianx commentedThat looks better to me :). Thanks!
Really RTBC now.
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedI'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.
Comment #46
Darren OhI’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.
Comment #47
Fabianx CreditAttribution: Fabianx commenteddb_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:
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!
Comment #48
mikeryanOK, here we go with the #30 patch plus the explicit connection selection per @Fabianx.
Comment #49
mikeryanPassed tests - is it kosher to return to rtbc, or should someone (e.g., Gold) who hasn't contributed to the patch retest first?
Comment #50
Darren OhAs 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.
Comment #51
Fabianx CreditAttribution: Fabianx commentedHere we go, back to RTBC
Comment #52
Darren OhComment #55
David_Rothstein CreditAttribution: David_Rothstein commentedComment #58
David_Rothstein CreditAttribution: David_Rothstein commentedThat was a bogus test failure, so back to RTBC for now.
Comment #61
Darren OhComment #64
Darren OhComment #67
David_Rothstein CreditAttribution: David_Rothstein commentedComment #70
Fabianx CreditAttribution: Fabianx commentedComment #72
Fabianx CreditAttribution: Fabianx commentedComment #74
David_Rothstein CreditAttribution: David_Rothstein commentedNot 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):
Comment #77
Kristen PolHad 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.
Comment #78
leochid CreditAttribution: leochid commentedThe 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.
Comment #79
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented@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.