Problem/Motivation

The Update module mostly deals in the information from Drupal.org Update XML as nested arrays. This code works and has not changed that much over the years so there has not been a great need to update it.

Now as we are starting to work on Automatic Updates for core will will need to parse that data in a whole new module. This would be good time to start to move the arrays to objects where it makes sense. This way the new module does not have also parse arrays and depend on getting the array keys correctly. Also the new code will be more self documenting because it will not need to explain what will be in the array.

Also the code can be more concise and readable. For instance we could move from

    elseif ($release['status'] == 'unpublished' ||
            !$is_in_supported_branch($release['version']) ||
            (isset($release['terms']['Release type']) &&
             (in_array('Insecure', $release['terms']['Release type']) ||
              in_array('Unsupported', $release['terms']['Release type'])))
    }

to

    elseif ($release->isUnpublished() ||
            !$is_in_supported_branch($release->getVersion()) ||
            $release->isUnsupported() ||
            $release->isInSecure()
    ) {

Proposed resolution

Create a new ProjectRelease class with appropriate getter methods.

Remaining tasks

User interface changes

None

API changes

TBD

Data model changes

None

Release notes snippet

None

Issue fork drupal-3206293

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Issue summary: View changes

dww’s picture

Status: Active » Needs review

Sounds promising! I'll hopefully be able to look at the MR later today.

tedbow’s picture

tedbow’s picture

Issue tags: +Needs tests

We need unit test for the new class and a function tests to confirm that an exception is logged for an invalid release.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tedbow’s picture

Issue tags: -Needs tests

Removing the "Needs tests" tag.

I added the unit test \Drupal\Tests\update\Unit\ProjectReleaseTest
I don't think this needs any functional tests because it does not change any functionality. We already have pretty good functional tests for this part of the Update module.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I think my feedback has been addressed. RTBC once tests pass!

tedbow’s picture

This will be very helpful for Automatic Updates

tedbow’s picture

just rebased because #3041885: Display relevant Security Advisories data for Drupal introduced \Drupal\Core\Extension\ExtensionVersion which replaces \Drupal\update\ModuleVersion which we used.

Leaving RTBC

  • effulgentsia committed 2e7bd80 on 9.3.x
    Issue #3206293 by tedbow, phenaproxima: Create ProjectRelease class to...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Nice cleanup. Pushed to 9.3.x.

tedbow’s picture

🎉 Thanks @effulgentsia and @phenaproxima

tedbow’s picture

Would it be possible to backport this to 9.2.x? It seems like it will hard to work in the contrib Automatic Updates project, like #3220205: Proposal: As pathway to Drupal core use new 2.x.x branch for Composer Based Updates if the changes we need in the Update module for back to 9.2.x

If we require 9.3.x for the contrib module and there aren't any actual core release for 9.3.x then people will not be able to really test the module to do actual updates until 9.3.0 come out in 6 months.

effulgentsia’s picture

Is there a reason that the contrib module can't include a copy of this class in its codebase, and dynamically load it if the Drupal version is <9.3?

Status: Fixed » Closed (fixed)

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