#2252991: phpUnit tests for ModuleHandler adds DRUPAL_ROOT to the phpUnit bootstrap since it is so heavily used in our code base. As follow up, remove its use from other unit tests.

CommentFileSizeAuthor
#12 test.unit_.drupal_root.12.patch3.42 KBsun
#8 drupal_root-2270323-8.patch46.36 KBdawehner
#5 remove-drupal-root-definitions-2270323-5.patch3.45 KBneclimdul
#3 remove-drupal-root-definitions-2270323-3.patch3.39 KBneclimdul
remove-drupal-root-definitions.patch3.37 KBneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Active » Needs work
Issue tags: +needs-reroll

This no longer completely applies.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

DRUPAL_ROOT should not have been added to the PHPUnit bootstrap.php.

I don't understand why it was added, and even added to a section that clearly states that constants should be removed...?

// Look into removing these later.
define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
define('DRUPAL_ROOT', realpath(__DIR__ . '/../../'));

The globally defined constants are tampering the environment of all tests, which is bad.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs work » Needs review
FileSize
remove-drupal-root-definitions-2270323-3.patch3.39 KB

I agree, you should be concerned. But we already do that because we don't force phpUnit tests to run tests in process isolation, we just do it in the tests files. Technically that's worse because it means we can accidentally rely on run order for tests to work. This just puts it up front and consistent.

alansaviolobo’s picture

alansaviolobo CreditAttribution: alansaviolobo commented
Issue tags: -needs-reroll +Needs reroll
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Issue tags: -Needs reroll
FileSize
remove-drupal-root-definitions-2270323-5.patch3.45 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Do we have a @TODO/issue to remove this globalness, for example in the phpunit bootstrap?

+++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
@@ -376,9 +376,6 @@ protected function systemListReset() {
 if (!defined('DRUPAL_MINIMUM_PHP')) {
   define('DRUPAL_MINIMUM_PHP', '5.3.10');
 }

I guess this could be moved to \Drupal without any problems? Not sure about early installer though, tbh.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Due to recent work on PHPUnit, I now agree with this change/cleanup. I don't see a way around avoiding DRUPAL_ROOT in the phpunit bootstrap.php either.

In fact, meanwhile I believe that an APP_ROOT constant like this should actually be supplied universally, centrally, and standardized by Composer's autoloader (which deals with many similar band-aid material like PHP INI settings anyway already). The constant is not really Drupal-specific; it simply points to the base/root directory of the application. I'll try to create an issue for that.

That said, note that there are some unit tests in HEAD that try to avoid DRUPAL_ROOT and thus use excessive relative directory name traversals à la dirname(dirname(dirname(__DIR__))) or __DIR__ . '/../../../../' to locate test fixtures. Would be great to adjust those accordingly here?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +WSCCI
FileSize
drupal_root-2270323-8.patch46.36 KB

Instead of using DRUPAL_ROOT we should now use $container->getParameter('app.root'), so we no longer need this workaround.

This is really just a start for now.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work

@dawehner: Please note the issue title - this issue is about phpunit tests only. There is no container in unit tests.

Can you create a separate issue for #8? (the proposal is questionable, but not to be discussed here)

The last submitted patch, 8: drupal_root-2270323-8.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Okay.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -WSCCI
FileSize
test.unit_.drupal_root.12.patch3.42 KB

Re-rolled against HEAD. No change, so RTBC.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Remove global DRUPAL_ROOT from phpUnit unit tests » Remove DRUPAL_ROOT constant redefinitions from unit tests

Correcting slightly misleading issue title.

webchick’s picture

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

Committed and pushed to 8.x. Thanks!

  • webchick committed 7a61549 on 8.0.x 
    Issue #2270323 by sun, dawehner, neclimdul: Remove DRUPAL_ROOT constant...

Status: Fixed » Closed (fixed)

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