Problem/Motivation

The autoloader added in #2293767: Allow PSR-4 test classes to be used in D7 uses include instead include_once to load files.
This can lead to issues in some special scenario.
My special scenario (no, don't ask why I've so many autoloaders...):
Modules: xautoload, service_container (registry_autoload)
Error: Fatal error: Cannot redeclare class Drupal\service_container_test\ServiceContainer\ServiceProvider\ServiceContainerServiceProvider
Code-Flow whe calling admin/config/development/testing:

  1. simpletest_test_get_all() creates the class name Drupal\service_container\Tests\modules\service_container_test\src\ServiceContainer\ServiceProvider\ServiceContainerServiceProvider based on service_container_test module and then checks it with class_exists(). (There is a class Drupal\service_container_test\ServiceContainer\ServiceProvider\ServiceContainerServiceProvider, but none with the built class name).
  2. This triggers xautoload which does it's magic and really finds the file, but unfortunately the give class name isn't the actual class name, so LoadClassInjectedAPI::guessPath() returns FALSE, which leads to further execution of autoloaders.
  3. Thus _simpletest_autoload_psr0() is invoked too - and also this handler manages to find the file despite the bogus class name provided by simpletest_test_get_all(). However, _simpletest_autoload_psr0() uses include to load the file, this now collides with the fact that the file was already loaded by xautoload.
  4. Hurray, redeclared class.

Proposed resolution

Use include_once instead include.
The whole thing still will be a mess but at least we wont have a fatal error.

Remaining tasks

Reviews needed.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#8 drupal-simpletest-includeonce-2468163-8.patch806 bytesjrreid
drupal-simpletest-use-include_once.patch439 bytesdas-peter
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, drupal-simpletest-use-include_once.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well, at least in Drupal 8 we don't place test modules inside /lib or /src ... as they really don't belong there, its not part of the PSR-0/4 tree of that module.

@fabianx
Do you mind a PR to move the test module into a different location? https://github.com/LionsAd/service_container/tree/7.x-1.x/lib/Drupal/ser...

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Drupal Association commented

Not at all, that was an oversight, it should be in the global ./tests/ directory unless I am mistaken?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Drupal Association commented

To that issue:

I think what is simpler is to check if the class already exists, which is cheaper than include_once and has less side effects:

if (!class_exists($class, FALSE) && file_exists($path)) {
  include $path;
}

It is an edge case, but it solves the problem that the file was loaded already somewhere else without having to go to the performance wise slower include_once.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I think what is simpler is to check if the class already exists, which is cheaper than include_once and has less side effects:

Well, note, we optimize for the simpletest case here, not for a generic classloading during runtime.
Given that I'd prefer the include_once solution, given that its a bit easier to read.

The PR is in https://github.com/LionsAd/service_container/pull/19

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Drupal Association commented

#5: Fair, that is an edge case of and edge case, nvm me :)

Edit: PR is merged, will push to upstream / d.org shortly.

das-peter’s picture

das-peter CreditAttribution: das-peter commented

@Fabianx class_exists() won't work in my scenario since the class name constructed is bogus but xautoload and the other loader manage somehow to find the file. If class_exists() would work xautoload would return TRUE and no other autloader would be executed.

jrreid’s picture

jrreid CreditAttribution: jrreid at University of Calgary commented
FileSize
drupal-simpletest-includeonce-2468163-8.patch806 bytes

Rerolling since the original patch failed. Small update was needed to catch both include statements in the current core for both PSR-0 and PSR-4

jrreid’s picture

jrreid CreditAttribution: jrreid at University of Calgary commented
Status: Needs work » Needs review
donquixote’s picture

donquixote CreditAttribution: donquixote commented

Is this issue still relevant?
I found this while planning a suitable directory structure to put more tests into D7 features module..

It seems this entire problem only occurs a module has files inside /src/ or /lib/ which do not follow the rules of PSR-4 or PSR-0.
Do we really care about this case, or should we rather close this as won't fix?

My special scenario (no, don't ask why I've so many autoloaders...):
Modules: xautoload, service_container (registry_autoload)

fyi:
I created two modules which allow to pick one or the other.. IF all the depending modules would actually change their dependencies :)
https://www.drupal.org/project/psr4ra
https://www.drupal.org/project/psr4xa