Problem/Motivation

\Drupal\Core\Command\BootableCommandTrait::boot() uses Request::createFromGlobals() to create a request to boot Drupal with.

This, unfortunately, causes the base URL to be wrong; it will be http://:core/scripts, which is totally invalid and will cause URLs to be mis-generated.

Proposed resolution

Drush sidesteps this by actually constructing a real-enough URL and calling Request::create(). We should do something similar.

Remaining tasks

Test coverage is easy: BootableCommandTrait should always assert that the global base URL at least has a valid form, by asserting that parse_url($GLOBALS['base_url']) !== FALSE.

Issue fork drupal-3553664

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.

phenaproxima opened merge request !13552

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

The assert can be removed no? Like it's not really adding much afaics

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

It is. Adding that assert caused all functional test jobs to fail (see the first commit and its pipeline). It’s a very effective way to ensure the URL is coherent.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

You are literally testing the basically hardcoded string you have added in Request::create('http://' . basename($kernel->getSitePath()) . '/core/scripts/drupal');

For me this feels like:

$a = 2 + 2;
assert($a === 4);
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +Drupal CMS stable release blocker

This is a Drupal CMS stable blocker. Our site templates will provide Canvas pages, and you can bet those pages will reference media items -- which is how I turned up this bug, but I don't doubt it could manifest in other ways too. If those recipes cannot be applied at the command line, we're in trouble.

In a quick Slack conversation with @alexpott, I agreed to remove the assertion. That means there's no explicit test coverage of this, but we both felt that the proper long term fix is for BootableCommandTrait to support a global --uri option, same as Drush does. I'll open a follow-up for that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@phenaproxima - now that is a test. Love it.

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

This looks good now, there was a discussion in Slack about the http vs https that this should not affect anything.

There is likely an issue in canvas that needs resolving for follow up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 11.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 8ac2dac793f to 11.x and b07aab80fb8 to 11.2.x. Thanks!

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.

  • alexpott committed b07aab80 on 11.2.x 
    Issue #3553664 by phenaproxima, alexpott, nicxvan: BootableCommandTrait...

  • alexpott committed 8ac2dac7 on 11.x 
    Issue #3553664 by phenaproxima, alexpott, nicxvan: BootableCommandTrait...

Status: Fixed » Closed (fixed)

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