Problem/Motivation

In Package Manager and Automatic Updates, which are both planned for core inclusion, we need to the update_test module to

  1. Set project versions
  2. Set fixture update XML files to use

We are duplicating logic in the Update module for this. The Update module itself is also duplicating a lot of logic.

There around 30 instances of $this->config('update_test.settings')->set were we save either system_info or 'xml_map'. Even though I(tedbow) am an Update module maintainer I often just have to grep the code base to remember where these settings take effect(update_test_system_info_alter and \Drupal\update_test\Controller\UpdateTestController)

Proposed resolution

Create UpdateTestTrait which will have 2 methods to update system_info and 'xml_map'. Replace all calls to save 'update_test.settings' with calls to this trait.

The methods on this trait should have @see to where these settings are used in the update_test module

I think the name setSystemInfo is a bit confusing when it's just setting the info for the installed modules. The #all is hard to understand at first since I am making a wrapper anyway i could have the #all's version as an optional parameter to just add it it in the system_info array.

Remaining tasks

Issue fork drupal-3348234

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:

Comments

tedbow created an issue. See original summary.

omkar.podey’s picture

Assigned: Unassigned » omkar.podey

omkar.podey’s picture

Still need to replace all instances.

dww’s picture

Status: Active » Needs work

Thanks for working on this! Left some review points on the MR...

omkar.podey’s picture

I think the name setSystemInfo is a bit confusing when it's just setting the info for the installed modules. The #all is hard to understand at first since I am making a wrapper anyway i could have the #all's version as an optional parameter to just add it it in the system_info array.

omkar.podey’s picture

Assigned: omkar.podey » Unassigned
Status: Needs work » Needs review
omkar.podey’s picture

Assigned: Unassigned » omkar.podey
Status: Needs review » Needs work
omkar.podey’s picture

Assigned: omkar.podey » Unassigned
Status: Needs work » Needs review
omkar.podey’s picture

yash.rode’s picture

Assigned: Unassigned » yash.rode
yash.rode’s picture

Assigned: yash.rode » Unassigned

I think core/modules/update/tests/src/Functional/UpdateUploadTest.php can also use the newly created trait?

yash.rode’s picture

Status: Needs review » Needs work
omkar.podey’s picture

Assigned: Unassigned » omkar.podey
omkar.podey’s picture

Assigned: omkar.podey » Unassigned
Status: Needs work » Needs review
omkar.podey’s picture

Assigned: Unassigned » omkar.podey
Status: Needs review » Needs work
omkar.podey’s picture

I missed a comment nit, fixing it

omkar.podey’s picture

Issue summary: View changes
omkar.podey’s picture

Assigned: omkar.podey » Unassigned
Status: Needs work » Needs review
yash.rode’s picture

Assigned: Unassigned » yash.rode
yash.rode’s picture

Assigned: yash.rode » Unassigned

I can confirm all the instances are replaced, Looks good to me!

phenaproxima’s picture

Status: Needs review » Needs work

This is a good start but I think the comments in particular could use a lot of clarification.

I'm going to be pretty gung-ho about that, because much of the Update module is mysterious morass of spaghetti code, and the last thing we want to do is introduce more mystery. We need this to be crystal clear about what methods we're providing and how they should be used, and especially what they mean.

omkar.podey’s picture

Assigned: Unassigned » omkar.podey
omkar.podey’s picture

Assigned: omkar.podey » Unassigned
Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Appears to be 1 unresolved thread.

This isn't very clear. We could just say the keys are extension names (not just modules, as I understand it), and the values are arrays that will replace (or be overlaid? I'm not certain) the extension's info as known to Drupal. Basically, we are mocking the contents of an info file. An @see to where this is used (update_test_system_info_alter(), primarily, but there may be others) would also be handy.

If that could be answered think this could be good.

Know it's a test trait but may warrant a change record.

phenaproxima’s picture

Issue tags: +Needs change record

Agreed that a change record is a good idea.

omkar.podey’s picture

Issue tags: -Needs change record

created a change record [#3358840]

omkar.podey’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Made some small tweaks to the CR, just adding before/after code snippets. But rest looks good!

wim leers’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to un-RTBC this 😅🙈 I have some clarity concerns.

  1. The docblocks in the code: see the MR
  2. The change record is not yet clear enough IMHO:
    1. setAvailableReleasesMetadata() needs a before vs after example
    2. the mockInstalledModules() example isn't clear enough, because the "before" does not show the structure of the $system_info variable, and the "after" does not use it at all → the evolution is impossible to understand based on that information

    Both would benefit from a more complete before vs after example.

omkar.podey’s picture

Updated change record.

omkar.podey’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

This is really shaping up. I think it's clearer than before, but could be clearer still. :)

wim leers’s picture

Reviewed, spotted a bug 🤓

omkar.podey’s picture

Updated change record.

omkar.podey’s picture

Status: Needs work » Needs review

retesting because of random failure.

wim leers’s picture

Status: Needs review » Needs work

Posted 8-point review on the MR.

omkar.podey’s picture

The unrelated test is passing locally.

omkar.podey’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

So many threads haha

From what I can see all the threads have been answered or addressed so remarking this.

wim leers’s picture

2 more nits: 2 lost comments. Restored them.

Now I agree with RTBC! 😊👍

Thanks for helping making Drupal's tests more maintainable! 👏

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +ddd2023

Adding a small API for this makes sense given the syntax was somewhat arcane before.

Committed and pushed 64da2134 to 11.x. Thanks! (second time lucky, after a push conflict with catch)

longwave’s picture

Reworded the change record title a bit, updated the version numbers, and published it.

  • longwave committed 64da2134 on 11.x
    Issue #3348234 by omkar.podey, Wim Leers, yash.rode, phenaproxima,...
tedbow’s picture

Thanks everyone!

Status: Fixed » Closed (fixed)

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