Problem/Motivation
In Package Manager and Automatic Updates, which are both planned for core inclusion, we need to the update_test module to
- Set project versions
- 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
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 #3
omkar.podey commentedComment #5
omkar.podey commentedStill need to replace all instances.
Comment #6
dwwThanks for working on this! Left some review points on the MR...
Comment #7
omkar.podey commentedI think the name
setSystemInfois a bit confusing when it's just setting the info for the installed modules. The#allis 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 thesystem_infoarray.Comment #8
omkar.podey commentedComment #9
omkar.podey commentedComment #10
omkar.podey commentedComment #11
omkar.podey commentedComment #12
yash.rode commentedComment #13
yash.rode commentedI think
core/modules/update/tests/src/Functional/UpdateUploadTest.phpcan also use the newly created trait?Comment #14
yash.rode commentedComment #15
omkar.podey commentedComment #16
omkar.podey commentedComment #17
omkar.podey commentedComment #18
omkar.podey commentedI missed a comment nit, fixing it
Comment #19
omkar.podey commentedComment #20
omkar.podey commentedComment #21
yash.rode commentedComment #22
yash.rode commentedI can confirm all the instances are replaced, Looks good to me!
Comment #23
phenaproximaThis 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.
Comment #24
omkar.podey commentedComment #25
omkar.podey commentedComment #26
smustgrave commentedAppears to be 1 unresolved thread.
If that could be answered think this could be good.
Know it's a test trait but may warrant a change record.
Comment #27
phenaproximaAgreed that a change record is a good idea.
Comment #28
omkar.podey commentedcreated a change record [#3358840]
Comment #29
omkar.podey commentedComment #30
smustgrave commentedMade some small tweaks to the CR, just adding before/after code snippets. But rest looks good!
Comment #31
wim leersSorry to un-RTBC this 😅🙈 I have some clarity concerns.
setAvailableReleasesMetadata()needs a before vs after examplemockInstalledModules()example isn't clear enough, because the "before" does not show the structure of the$system_infovariable, and the "after" does not use it at all → the evolution is impossible to understand based on that informationBoth would benefit from a more complete before vs after example.
Comment #32
omkar.podey commentedUpdated change record.
Comment #33
omkar.podey commentedComment #34
phenaproximaThis is really shaping up. I think it's clearer than before, but could be clearer still. :)
Comment #35
wim leersReviewed, spotted a bug 🤓
Comment #36
omkar.podey commentedUpdated change record.
Comment #37
omkar.podey commentedretesting because of random failure.
Comment #38
wim leersPosted 8-point review on the MR.
Comment #39
omkar.podey commentedThe unrelated test is passing locally.
Comment #40
omkar.podey commentedComment #41
smustgrave commentedSo many threads haha
From what I can see all the threads have been answered or addressed so remarking this.
Comment #42
wim leers2 more nits: 2 lost comments. Restored them.
Now I agree with RTBC! 😊👍
Thanks for helping making Drupal's tests more maintainable! 👏
Comment #44
longwaveAdding 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)
Comment #45
longwaveReworded the change record title a bit, updated the version numbers, and published it.
Comment #47
tedbowThanks everyone!