Problem/Motivation

Automatic Updates isn't ready to handle composer sites, but has 3rd party dependencies of its own.

Proposed resolution

Remove all dependencies on 3rd party packages, except for drupal/php-signify, which will be packaged internally. This should work, since ocramius/package-versions and webflo/drupal-finder were added to simplify support for Composer.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

Issue summary: View changes
hestenet’s picture

Remove all dependencies on 3rd party packages, except for drupal/php-signify, which will be packaged internally. This should work, since ocramius/package-versions and webflo/drupal-finder were added to simplify support for Composer.

I think removing these dependencies makes sense for now, since it's very likely that the only way we'll be able to robustly handle composer support will be under the A/B model of updates in Phase 2.

Removing these elements for now gives us a clean way to handle all the test cases that *are* supported in the current phase, and I think the waters are a bit less muddy.

heddn’s picture

Status: Active » Needs review
FileSize
1.4 MB

This does the leap backwards to not use composer. :sadpanda:

webchick’s picture

Priority: Normal » Major

This was flagged on the cross-initiative call, sounds like it's at least "major."

heddn’s picture

heddn’s picture

Priority: Normal » Major
heddn’s picture

FileSize
41.2 KB

Here's a review only patch, now that we're green.

heddn’s picture

Self review explaining some of the decisions.

  1. +++ b/automatic_updates.info.yml
    @@ -5,5 +5,5 @@ core: 8.x
    +  - drupal:system (>= 8.7)
    

    We had a hidden dependency on 8.7 due to extension.list.module, etc that are only added in 8.7. Plus once we release a stable tag, 8.6 will have zero new releases/support at that time. So this shouldn't be an issue. We can remove the 8.7 dependency 8.6 is officially sunset.

  2. +++ b/composer.json
    @@ -13,12 +13,11 @@
    +      "php": "5.6.0"
    

    This makes sure when we regenerate our external dependencies, we do it using the oldest version we're purported to support. Which is 5.6 still w/ 8.7.x.

  3. +++ b/phpcs.xml.dist
    @@ -0,0 +1,7 @@
    +  <exclude-pattern>*/vendor/*$</exclude-pattern>
    

    We had failing PHPCS on the new vendor code. Here we get around that by using our own custom xml that excludes vendor folder.

  4. +++ b/phpcs.xml.dist
    @@ -0,0 +1,7 @@
    diff --git a/scripts/update_dependencies.sh b/scripts/update_dependencies.sh
    

    Nice little shell script to automate the generation of the needed dependencies.

  5. +++ b/src/Form/SettingsForm.php
    @@ -39,9 +39,7 @@ class SettingsForm extends ConfigFormBase {
    +    $instance->drupalRoot = (string) $container->get('app.root');
    

    Casting to string is intentional to set us up for better support in D9.

  6. +++ b/src/Services/InPlaceUpdate.php
    @@ -217,7 +217,7 @@ class InPlaceUpdate implements UpdateInterface {
    +      $this->logger->error('Can not update because %count files are modified: %paths', [
    

    This was wrong.

  7. +++ b/tests/src/Build/InPlaceUpdateTest.php
    @@ -178,8 +179,14 @@ class InPlaceUpdateTest extends QuickStartTestBase {
    +    $datum[] = [
    

    This proves we now support non-composer installs. Previously, 8.7.2 had changes to composer.lock that were breaking successful updates.

heddn’s picture

A new version of the polyfills was released overnight. As all the changes are generated via composer update inside of scripts/update_dependencies.sh, not uploading a new patch. Just know that what will get committed will be slightly different.

This is green and still in needs of reviews. I'll probably land it early next week and release a new alpha if I don't hear any concerns.

heddn’s picture

FileSize
48.54 KB
7.71 KB

Actually, there were some minor changes to things.

heddn’s picture

Status: Needs review » Fixed

  • heddn committed 4853a4e on 8.x-1.x
    Issue #3088095 by heddn: Automatic Updates isn't ready to handle...

Status: Fixed » Closed (fixed)

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