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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy created an issue. See original summary.

Gábor Hojtsy credited dww.

Gábor Hojtsy’s picture

Uploading @dww's patch from #3096078-35: Display core compatibility ranges for available module updates. Crediting all contributors to this followup.

dww’s picture

Copying 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:

  1. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -9,29 +9,21 @@
    + * The only public method, setReleaseMessage(), constructs a list of versions of
    + * Drupal core that a given release is compatible with. The compatible versions
    + * are displayed in ranges to minimize how much text is generated. This list is
    + * used on the available updates report.
    

    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.
    setReleaseMessage doc in phpstorm
    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...

  2. +++ b/core/modules/update/update.compare.inc
    @@ -110,6 +111,12 @@ function update_calculate_project_data($available) {
    +      // Inject the list of compatible core versions to show site builders which
    ...
    +      // example, 8.9.x and 9.0.x), this list will hopefully help site builders
    

    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.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
4.88 KB

Here I have uploaded a patch, please review it.
I have tried to make changes as suggested in #6.

tedbow’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/src/ProjectCoreCompatibility.php
@@ -14,21 +14,28 @@ class ProjectCoreCompatibility {
    * @var string[]

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.

  1. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -14,21 +14,28 @@ class ProjectCoreCompatibility {
    +   * This list is cached since many project releases will use the same core
    +   * compatibility constraint and we don't want to re-compute the same
    +   * information over and over.
    

    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.

  2. +++ b/core/modules/update/update.compare.inc
    @@ -110,6 +111,12 @@ function update_calculate_project_data($available) {
    +      // Inject the list of compatible core versions to show administrator(s) which
    ...
    +      // example, 8.9.x and 9.0.x), this list will hopefully help administrator(s)
    

    This comments go over the line char limit of 80.

  3. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -8,27 +8,43 @@
    +   * This list is cached since many project releases will use the same core
    +   * compatibility constraint and we don't want to re-compute the same
    +   * information over and over.
    

    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

diff --git a/Users/ravi/Downloads/3101547-3096078-35.follow-up-doc-fixes.patch b/Users/ravi/Desktop/contrib-patch/3101547-7.patch
index 7edbb6de66..af84957362 100644
--- a/Users/ravi/Downloads/3101547-3096078-35.follow-up-doc-fixes.patch
+++ b/Users/ravi/Desktop/contrib-patch/3101547-7.patch

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😿)

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
5.94 KB
2.11 KB

Thanks, @tedbow.

Here I have made changes as suggested in comment #8.

tedbow’s picture

Status: Needs review » Needs work

@ravi.shankar thanks for again for the patch.

+++ b/core/modules/update/src/ProjectCoreCompatibility.php
@@ -26,9 +26,7 @@ class ProjectCoreCompatibility {
-   * This list is cached since many project releases will use the same core
-   * compatibility constraint and we don't want to re-compute the same
-   * information over and over.
+   * Compatibility constraint.

We still need the first part of this sentence. So it would be

This list is cached since many project releases will use the same core compatibility constraint.

Other than that patch looks good.

Not sure what happened my review #8 🤦‍♂️ where I added the comment 3 times 🤷‍♂️

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
655 bytes

Thanks, @tedbow

Here I have again made changes. Please review it.

ravi.shankar’s picture

Sorry, I have mistakenly added .patch extension in "interdiff" of patch #11

ravi.shankar’s picture

FileSize
655 bytes

Here I have added interdiff of patch #9 and #11

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@ravi.shankar thanks! Looks good

  • Gábor Hojtsy committed 29cf0b5 on 9.0.x
    Issue #3101547 by ravi.shankar, Gábor Hojtsy, tedbow, dww, lauriii:...

  • Gábor Hojtsy committed c217188 on 8.9.x
    Issue #3101547 by ravi.shankar, Gábor Hojtsy, tedbow, dww, lauriii:...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Superb, thanks all!

Status: Fixed » Closed (fixed)

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

tedbow’s picture

Version: 8.9.x-dev » 8.8.x-dev
Assigned: Unassigned » tedbow
Priority: Normal » Critical

these 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

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Closed (fixed) » Needs review
FileSize
6.01 KB
25.92 KB
tedbow’s picture

#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

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

  • Gábor Hojtsy committed ff08c34 on 8.8.x
    Issue #3101547 by ravi.shankar, Gábor Hojtsy, tedbow, dww, lauriii:...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks!

Status: Fixed » Closed (fixed)

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