Problem/Motivation

#2386247: install.php should pass the class loader down into install_begin_request() introduced a backwards-compatibility layer to install_drupal() to temporarily allow calling it without a class loader.

This was necessary because the testbot uses Drush to install Drupal.

Proposed resolution

As soon as that issue is committed, a corresponding fix to Drush is committed and Drush is updated on all test runners this backwards-compatibility layer should be removed.

Remaining tasks

User interface changes

API changes

Only local images are allowed.

Only local images are allowed.

CommentFileSizeAuthor
#25 2389243_25.patch877 bytesmile23
#23 2389243-23.patch848 bytesdawehner
#9 2389243_6.patch1.46 KBmile23
#6 interdiff.txt639 bytesmile23
#6 2389243_6.patch1.46 KBmile23
#4 2389243_4.patch852 bytesmile23
#3 2389243-3.patch856 byteststoeckler

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Parent issue: » #2386247: install.php should pass the class loader down into install_begin_request()
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Created https://github.com/drush-ops/drush/pull/1051

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Postponed » Needs review
StatusFileSize
new 2389243-3.patch856 bytes

I think this should work by now.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2389243_4.patch852 bytes

Reroll.

Encountered while trying to make this work: #2380389: Use a single vendor directory in the root

Status: Needs review » Needs work

The last submitted patch, 4: 2389243_4.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2389243_6.patch1.46 KB
new interdiff.txt639 bytes

Missed one.

This leads to this rather interesting error in two tests when run in the UI:

Drupal\Tests\simpletest\Functional\BrowserTestBaseTest::testGoTo Drupal\Core\Installer\Exception\AlreadyInstalledException:
To start over, you must empty your existing database and copy default.settings.php over settings.php.
To upgrade an existing installation, proceed to the update script.
View your existing site.
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@Mile23 just to confirm: this patch in #6 breaks the simpletests in the UI?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Yes. That error for both Drupal\Tests\simpletest\Functional\BrowserTestBaseTest::testGoTo and Drupal\Tests\simpletest\Functional\BrowserTestBaseTest::testForm.

I couldn't track it down in the time I have to work on Drupal today.

Since we have two (yes, TWO) different ways of setting up the fixture Drupal for functional testing, I'm not surprised.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
StatusFileSize
new 2389243_6.patch1.46 KB

Reupload of #6 to see what happens on the new testbot.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: +rc target triage

Tagging for triage because it would be nice to get rid of this cruft during RC.

joelpittet’s picture

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

This seems to do the trick with the new Drupal CI

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.0.x-dev » 9.x-dev
Status: Reviewed & tested by the community » Postponed
Issue tags: -rc target triage

I'm not comfortable with this BC break during RC (and it is a BC break, despite that the documentation does not explicitly support the older usage).

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

So improvements to tests are now off-limits for 8.0 release?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

The second hunk can be moved to another issue. The first hunk is not in test code, but the installer, and is slated for 9.x

dawehner’s picture

dawehner
Primary language German
commented

I'm not comfortable with this BC break during RC (and it is a BC break, despite that the documentation does not explicitly support the older usage).

I disagree to be honest. The installer is not an official runtime API, so for example applying this patch will not break ANY existing site.

catch’s picture

catch
he/him
Primary language English
commented
Version: 9.x-dev » 8.1.x-dev

Yes I think we could treat the installer as @internal potentially. Moving back to a minor version for discussion at least.

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

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

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.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Postponed » Active

Reopening

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2389243-23.patch848 bytes

Reroll.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2389243_25.patch877 bytes

Another re-roll. The fact that the patch didn't apply is further evidence that we don't need the BC code. (It didn't apply because we changed the function arguments to support a native command-line installer.)

joelpittet’s picture

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

Assuming this is @internal and it looks like Drush hasn't been using this for a while in the testing infra.

The last submitted patch, 23: 2389243-23.patch, failed testing. View results

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

DrupalCI testing infra doesn't use Drush (for D8 anyway).

catch’s picture

catch
he/him
Primary language English
commented
Title: Remove BC-layer in install_drupal() » Remove unnecessary BC-layer in install_drupal()

  • catch committed f9a3fa0 on 8.7.x 
    Issue #2389243 by Mile23, tstoeckler, dawehner: Remove unnecessary BC-...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Looked back over this. Drush was updated for the change in 2014, so I think this is fine to commit to a minor release. While it was introduced as a 'bc layer' this was a shim for a couple of months that never got updated rather than anything we expected code other than 2014-era drush to call.

Committed f9a3fa0 and pushed to 8.7.x. Thanks!

Status: Fixed » Closed (fixed)

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