Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#3096078: Display core compatibility ranges for available module updates added displaying core compatibility ranges for available module updates. @dww had some followup concerns with the code comments in #3096078-28: Display core compatibility ranges for available module updates onwards. Transporting patches here for continued work.
Proposed resolution
Discuss, refine and commit.
Remaining tasks
Discuss, refine and commit.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A.
Comment | File | Size | Author |
---|---|---|---|
#21 | 3101547-8.8-reroll.patch | 6.01 KB | tedbow |
#20 | 3101547-8.8-plus-3096078-48.patch | 25.92 KB | tedbow |
#20 | 3101547-8.8.x-do-not-test.patch | 6.01 KB | tedbow |
#13 | interdiff_9-11.txt | 655 bytes | ravi.shankar |
#11 | interdiff_9-11.patch | 655 bytes | ravi.shankar |
Comments
Comment #5
Gábor HojtsyUploading @dww's patch from #3096078-35: Display core compatibility ranges for available module updates. Crediting all contributors to this followup.
Comment #6
dwwCopying over the NW review from @tedbow in #3096078-36: Display core compatibility ranges for available module updates so it's easier to act on this and get it RTBC:
I still think it is more helpful to have a shorter class doc. This describes
setReleaseMessage()
but is not on that method so any automated tool a developer where to use to see the doc for that method would not have this text.I use PHPStorm but I think this would be true for any IDE that lets you view the docs when looking at a method call.
above doesn't have this helpful description.
This would also be true from api.drupal.org. Example method with longer description: https://api.drupal.org/api/drupal/core%21modules%21update%21src%21Update...
Just check and we use the the term "administrator(s)" much for often in core comments than we do "site builder". I think it would be good to change to that.
Comment #7
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have uploaded a patch, please review it.
I have tried to make changes as suggested in #6.
Comment #8
tedbowThis sentence can just end with "compatibility constraint".
"we don't want to re-compute the same information over and over." is just the reason we cache anything. No need to state that here.
This sentence can just end with "compatibility constraint".
"we don't want to re-compute the same information over and over." is just the reason we cache anything. No need to state that here.
This comments go over the line char limit of 80.
This sentence can just end with "compatibility constraint".
"we don't want to re-compute the same information over and over." is just the reason we cache anything. No need to state that here.
just a nit on the interdiff
having the local files names in the interdiff means others simply apply the interdiff to confirm the changes since the last patch locally.(can't wait for gitlab merge requests😿)
Comment #9
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedThanks, @tedbow.
Here I have made changes as suggested in comment #8.
Comment #10
tedbow@ravi.shankar thanks for again for the patch.
We still need the first part of this sentence. So it would be
Other than that patch looks good.
Not sure what happened my review #8 🤦♂️ where I added the comment 3 times 🤷♂️
Comment #11
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedThanks, @tedbow
Here I have again made changes. Please review it.
Comment #12
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedSorry, I have mistakenly added .patch extension in "interdiff" of patch #11
Comment #13
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have added interdiff of patch #9 and #11
Comment #14
tedbow@ravi.shankar thanks! Looks good
Comment #17
Gábor HojtsySuperb, thanks all!
Comment #19
tedbowthese needs to be backported to 8.8.x or #3102724: Improve usability of core compatibility ranges on available updates report will be very hard to backport
Making critical because that issue is critical
Comment #20
tedbowComment #21
tedbow#3096078: Display core compatibility ranges for available module updates was committed to 8.8.x!
So now clean re-roll
check locally and this is the exact same file as 3101547-8.8.x-do-not-test.patch
Comment #22
tedbowComment #24
Gábor HojtsyThanks!