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
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:
Comments
Comment #2
tedbowComment #4
dwwSounds promising! I'll hopefully be able to look at the MR later today.
Comment #5
tedbowadding related #3095755: Tests should not do requests to updates.drupal.org
Comment #6
tedbowWe need unit test for the new class and a function tests to confirm that an exception is logged for an invalid release.
Comment #8
tedbowRemoving 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.
Comment #9
phenaproximaI think my feedback has been addressed. RTBC once tests pass!
Comment #10
tedbowThis will be very helpful for Automatic Updates
Comment #11
tedbowjust 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
Comment #13
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNice cleanup. Pushed to 9.3.x.
Comment #14
tedbow🎉 Thanks @effulgentsia and @phenaproxima
Comment #15
tedbowWould 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.
Comment #16
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIs 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?