Problem/Motivation

The Drupal\system\Tests\Module\ClassLoaderTest should test if a class provided by a module could not be loaded if the module is disabled. But that test doesn't work the way it is implemented. The route that is accessed by this web test is not available because the module providing it is not installed. The resulting 404 is erroneously taken as successful check that the class loader was prevented to load a class of a disabled module.

Proposed resolution

Fix the test. Done.

Remaining tasks

none

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#22 2777651_fixed_test.patch2.13 KBmkalkbrenner
#7 2777651-demonstrate_autoloader_issue_with_disabled_modules-interdiff.txt1.05 KBmkalkbrenner
#5 2777651_demonstrate_autoloader_issue_with_disabled_modules.patch2.76 KBmkalkbrenner
#4 2777651_fixed_test.patch2.13 KBmkalkbrenner
#2 2777651_failing_test.patch1010 bytesmkalkbrenner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mkalkbrenner created an issue. See original summary.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
Status: Active » Needs review
FileSize
2777651_failing_test.patch1010 bytes

Here's a the "corrected" failing test.

Status: Needs review » Needs work

The last submitted patch, 2: 2777651_failing_test.patch, failed testing.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
Status: Needs work » Needs review
FileSize
2777651_fixed_test.patch2.13 KB
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
FileSize
2777651_demonstrate_autoloader_issue_with_disabled_modules.patch2.76 KB

Now we have the existing tests fixed in #4.
But the tests states that auto loading of classes provided by a non enabled module should not be possible!

Here's a demonstration that there's no protection like this (that affected us in search_api).

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented

.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
FileSize
2777651-demonstrate_autoloader_issue_with_disabled_modules-interdiff.txt1.05 KB

The interdiff between #4 and #5.

Status: Needs review » Needs work

The last submitted patch, 5: 2777651_demonstrate_autoloader_issue_with_disabled_modules.patch, failed testing.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
Status: Needs work » Needs review

OK, understood the reason why the patch in #5 fails and why classloading has to be implemented as web test.
SomeClass is provided by a test module and drupal_phpunit_get_extension_namespaces() registers all test modules' paths for the PSR4 classloader.

So the patch in #4 is correct and should be committed to have some test coverage that classes provided by disabled modules are not auto loadable.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
Issue summary: View changes
cspitzlay’s picture

cspitzlay
Primary language German
Location 🇩🇪🇪🇺
CreditAttribution: cspitzlay at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

cspitzlay’s picture

cspitzlay
Primary language German
Location 🇩🇪🇪🇺
CreditAttribution: cspitzlay at bio.logis Genetic Information Management GmbH commented
Issue summary: View changes

Updated the issue summary

cspitzlay’s picture

cspitzlay
Primary language German
Location 🇩🇪🇪🇺
CreditAttribution: cspitzlay at bio.logis Genetic Information Management GmbH commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

This is still in progress and there's no passing patch yet, so should not be RTBC. Interesting find in the issue though.

cspitzlay’s picture

cspitzlay
Primary language German
Location 🇩🇪🇪🇺
CreditAttribution: cspitzlay at bio.logis Genetic Information Management GmbH commented

As stated in #9 the patch from #4 contains the correct fix to the broken test and it did pass.

The expectation that the classloading in #5 should not work is wrong as the test is done in the test process itself where classloading works differently because phpunit interferes.

What is missing for this issue to be RTBC?

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
Status: Needs work » Reviewed & tested by the community

This is still in progress and there's no passing patch yet, so should not be RTBC.

The patch in #4 should be committed as reviewed by cspitzlay. It passes and fixes the existing test.

In #9 I answered the question I asked myself in #7. If we consider drupal_phpunit_get_extension_namespaces() to be a bad implementation in general, we should open a follow-up issue. BTW personally I think that it is a bad thing that class loading behaves different in a Kernel Test.
Please excuse the confusion I potentially created with #7.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Title: Drupal\system\Tests\Module\ClassLoaderTest doesn't test class loading » Improve Drupal\system\Tests\Module\ClassLoaderTest class loading tests

@mkalkbrenner so test class autoloading is a bit of a controversial subject but I'm inclined to agree that running tests should not affect the autoloader in anyway. In my mind autoloaded tests are completely wrong-headed because you never autoload a tests at runtime and by being able to autoload tests you are affecting the system under test.

The tests added here are an improvement. I would not say that the current tests are broken given that there should be no difference between a module that has been installed and then uninstalled and one that has never been installed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Also it helps committers a lot when the patch to commit is the final patch on the issue. And it helps testbot too because rtbc re-test only occurs on the final patch too.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Improve Drupal\system\Tests\Module\ClassLoaderTest class loading tests » Add coverage for uninstalled modules in Drupal\system\Tests\Module\ClassLoaderTest class loading tests
Priority: Major » Normal

(Retitiling for clarity and setting the issue priority based on https://www.drupal.org/core/issue-priority#major).

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Needs review

Ah, and as stated above, please upload the RTBC patch as the final patch on the issue. (I got confused also.) Thanks!

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH for OSCE: Organization for Security and Co-operation in Europe commented
FileSize
2777651_fixed_test.patch2.13 KB

I would not say that the current tests are broken given that there should be no difference between a module that has been installed and then uninstalled and one that has never been installed.

They are broken. The resulting 404 is erroneously taken as successful check that the class loader was prevented to load a class of a disabled module.

please upload the RTBC patch as the final patch on the issue.

No problem. Here's the patch from #4 again.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
Status: Needs review » Reviewed & tested by the community

I think it's safe to get this back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed bd6b92d to 8.3.x and 3de07a9 to 8.2.x. Thanks!

  • alexpott committed bd6b92d on 8.3.x 
    Issue #2777651 by mkalkbrenner: Add coverage for uninstalled modules in...

  • alexpott committed 3de07a9 on 8.2.x 
    Issue #2777651 by mkalkbrenner: Add coverage for uninstalled modules in...

Status: Fixed » Closed (fixed)

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