Problem/Motivation

See https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co... for a bit of prior art.

Long story short: DrupalApplication doesn't call DrupalKernel::bootEnvironment() which, among other things, sets up the awkward DRUPAL_TEST_IN_CHILD_SITE constant.

Result: if you run a command in a test, and that command runs some code that interrogates the DRUPAL_TEST_IN_CHILD_SITE constant, it craps out with a fatal error because you've tried to access an undefined constant.

Proposed resolution

Option 1: Split out the part of bootEnvironment() responsible for DRUPAL_TEST_IN_CHILD_SITE to a new method

MR !16037

  • Add a new public (but @internal) DrupalKernel::setupDrupalTestInChildSite() method.
  • Move all the code from DrupalKernel::bootEnvironment() that deals with DRUPAL_TEST_IN_CHILD_SITE into that.
  • Call it from both DrupalKernel::bootEnvironment() and DrupalApplication::bootstrap().

Option 2: Add a flag to bootEnvironment() to not change the error handler

MR !16039

  • Add a new optional $set_error_handler flag (defaults to TRUE) to DrupalKernel::bootEnvironment() .
  • Only set the Drupal error handler if this flag is TRUE.
  • Call DrupalKernel::bootEnvironment() from DrupalApplication::bootstrap() with the flag set to FALSE.
  • Remove @todo comment about not calling bootEnvironment().

Issue fork drupal-3597692

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

dww made their first commit to this issue’s fork.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Issue tags: -undefined +Bug Smash Initiative

Yup, another good find. Thanks for opening this!

dww’s picture

dww
we/he/they
commented
Related issues: +#2690035: Extract DrupalKernel::bootEnvironment into SAPI adapter

Seems like we should start the work of #2690035: Extract DrupalKernel::bootEnvironment into SAPI adapter and move the DRUPAL_TEST_IN_CHILD_SITE block in DrupalKernel::bootEnvironment() to a static helper method, and call it from both bootEnvironment() and DrupalApplication.

dww opened merge request !16037

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review

Something like that?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

This fully solves the problem for Drupal CMS. My feeling: land it now before 11.4 to avoid breaking folks who might run into this, refactor later.

dww opened merge request !16039

kingdutch’s picture

kingdutch
he/him
Primary language English
commented

+1 on RTBC based on 16037

16039 is interesting, but makes it more complex to get rid of the $app_root argument from bootEnvironment based on the extra optional argument.

At least to me it makes eliminating bootEnvironment (or the contents of setupDrupalTestInChildSite) more complex, because making sure the logic is moved around is no longer enough and we now have an additional branch we need to figure out when refactoring.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

Thanks for the review Kingdutch! Agreed on all points. Pushed a commit to apply the suggestions, and fully make $app_root required (update docs). Also pushed a commit to mark DrupalKernel::setupDrupalTestInChildSite() @internal even though it's public. Pipeline is still green.

However, also considered another possible solution here. Is MR!16039 better? No new method (just a new optional flag to DrupalKernel::bootEnvironment()) and now we can call it from DrupalApplication. Removes a @todo and @see.

Updated the summary with both options for proposed resolution.

Back to NR, although either MR could be RTBC at this point.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

Restoring RTBC for MR 16037. Leaving 16039 up for committer discretion.

dww’s picture

dww
we/he/they
commented

For clarity, when I wrote comment #10, comment #9 only said "+1 on RTBC". 😅 So I was only agreeing on "all points" from the review of 16037. After I posted #10, #9 was edited to compare the 2 MRs.

I think 16037 is probably better, and that's the one phenaproxima has already tested and confirmed.

kingdutch’s picture

kingdutch
he/him
Primary language English
commented

Yeah, when I wrote +1 on RTBC I only saw one MR 😜 so edited to clarify 🤝

Edit: typo

godotislate changed the visibility of the branch 3597692-bootenvironment-error-handler-flag to hidden.

godotislate closed merge request !16037

  • godotislate committed 75547409 on main 
    fix: #3597692 [regression] The 'dr' command doesn't set...

  • godotislate committed bd169751 on 11.x 
    fix: #3597692 [regression] The 'dr' command doesn't set...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed changes from MR 16037 and and pushed 7554740 to main, and bd16975 to 11.x. Thanks!

Since we are in RC period, will need to check with other RMs about whether this can be backported to 11.4. Setting to Patch (to be ported) for now.

  • godotislate committed 3a81647d on 11.4.x 
    fix: #3597692 [regression] The 'dr' command doesn't set...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.4.x-dev
Status: Patch (to be ported) » Fixed

Spoke with @longwave on Slack, and this is good for 11.4.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

dww closed merge request !16039

dww’s picture

dww
we/he/they
commented

Thanks! Relieved to see this in all the relevant branches.

Status: Fixed » Closed (fixed)

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