After the update at #715108: Make Merge queries more consistent and robust, I get a fatal error because the SPL autoloader includes the same database .inc file 2x for some reason. That makes it quite hard to recover.

For very little benefit, the autoloader uses require, unlike the rest of Drupal that uses require_once.

Example error:

Fatal error: Cannot redeclare class InsertQuery_mysql in ~/drupal-7/includes/database/mysql/query.inc on line
88 Call Stack: 0.0003 57396 1. {main}() ~/drupal-7/index.php:0 0.0111 505184 2. drupal_bootstrap()
CommentFileSizeAuthor
#1 839006-SPL-require-once-1.patch767 bytespwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Title: SPL autoloader shoudl use require_once to be more robust. » SPL autoloader should use require_once to be more robust.
Status: Active » Needs review
FileSize
839006-SPL-require-once-1.patch767 bytes
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

ouch. i think this patch is good regardless of #715108: Make Merge queries more consistent and robust, but i'd like to dig a bit into this. maybe its nothing, but worth a look. i don't think looking into this should stop this patch getting committed.

marking as RTBC, because its a trivial change.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

The explanation is easy, we are trying to load MergeQuery_mysql:

  • MergeQuery_mysql is supposedly in include/database/mysql/query.inc, the database autoloader knows that and tries to load that file,
  • Sadly this class is not defined there, so the SPL goes on to the next autoloader, which is the registry,
  • The registry recorded that this class was defined in include/database/mysql/query.inc, so it tries to load that, but uses a require instead of a require_once, leading to the error
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

I agree with that change. The require here is of very little benefit.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

#3 oh, you mean MergeQuery_mysql. yes, that makes sense.

chx’s picture

chx CreditAttribution: chx commented

hold! i want to discuss/understand.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

discussed this with chx in #drupal, i've created #839066: document that moving a class requires a registry rebuild in D7 so we document the new registry snafu's when a code update moves a class.

chx’s picture

chx CreditAttribution: chx commented

OK then this is a go.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Looks pretty straight-forward. Committed to HEAD. Thanks!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

@3: I thought it is because we have multiple classes in that file and the file is loaded when one of these is used and when we are trying to load MergeQuery_mysql, the class is still not defined and the registry tries to load the file again. Your explanation sounds valid too ;)

Anyway, for the reference, performance shouldn't be a big problem anymore regarding require/require_once, see http://arin.me/blog/php-require-vs-include-vs-require_once-vs-include_on....

Status: Fixed » Closed (fixed)

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