Problem/Motivation

Now that #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches has been done core_version_requirment can be used to declare core compatibility via Composer semver constraint. It is technically already possible to declare that your module is already compatible with Drupal 9 only. For instance if a module requires one of the newly updated composer dependencies in Drupal 9.

This means that currently the update module could be listing contrib modules that are only compatible with Drupal 9. Previously this would not have been possible.

Once #3009338: [META] Support semantic versioning for extensions (modules, themes, etc) in Drupal core, and allow modules to be compatible with Drupal 8 and 9 at the same time is completed there will no correlation between the module version number and core compatiblity.

Therefore we should display the core compatibility of updates to display this information to the user.

In #3076183: [Meta] Determine how available updates for modules should be handled if they are not compatible with the current version of Drupal core we determined we want

  1. Should a message on all update that display their core compatibility based of the core_version_requirement in info.yml files. This information is now in the update XML from drupal.org as core_compatibility

    The message should be in this format:

    This module is compatible with Drupal core: 8.8.3 to 8.9.1

  2. As 2nd step will switch to a update XML feed that is not tied to CORE_COMPATIBILITY but rather all 8.x and future module updates.
    This will be handled in #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates

Original all this handled in #3074993 because only the /current feed had core_compatibility
but now this available in the current XML we can handle this first in it's own issue.

Proposed resolution

copied from #3076183: [Meta] Determine how available updates for modules should be handled if they are not compatible with the current version of Drupal core

  1. Done: in #3074998: Add explicit information about core compatibility to update data add a new core_compatiblity value for each update. This is will be based on core_version_requirement or if not available it will be based core: 8.x

    This is will be for the project not individual modules in the project. Details to be determine in that issue

  2. Display a message for each available project update that displays the core compatiblity range based on 1) actual available updates the update module has retrieved from the update server and 2) the core_compatiblity for the project update xml(see above)
  3. For most common possibilities for core_compatiblity this will be able to displayed as a simple range.

    For example
    Installed core: 8.8.3
    latest 8.x patch release: 8.9.1
    Drupal 9 not released yet
    project xml core_compatiblity: ^8 || ^9

    This module is compatible with Drupal core: 8.8.3 to 8.9.1

    If Drupal 9.0.3 had been released this would change to

    This module is compatible with Drupal core: 8.8.3 to 9.0.3

    if project xml wascore_compatiblity: ^: ^8.8.9 || ^9 (not using core_version_requirement because the value will be directly from the project xml not the info.yml files)
    This module is compatible with Drupal core: 8.8.9 to 9.0.3

  4. Some less common values for core_compatiblity will not be able to display by a simple range. In that case we should display them in a series of ranges.

    For example
    Given:
    Installed core: 8.8.3
    latest 8.8.x: 8.8.7
    latest 8.x patch release: 8.9.1
    Drupal 9 not released yet

    core_compatiblity: ~8.8.0 || ^9

    This module is compatible with Drupal core: 8.8.3 to 8.8.7

    Because 9 hasn't been released it won't come into play.

    If Drupal 9.0.3 had been released

    This module is compatible with Drupal core: 8.8.3 to 8.8.7 and 9.0.0 to 9.03

  5. In this first step no modules will be filtered out of the list.

    This will mean hypothetically you could get projects updates with the message like

    This module is compatible with Drupal core: 9.0.0 to 9.0.5

    In the near term this is very unlikely because 1) Drupal 9 is not out so no module should be compatible with it but not Drupal 8 and 2) 9.0.0 should be compatible with 8.9.0 for modules not using deprecated code. Therefore any module that is compatible with 9.0.0 because it remove deprecations should also be compatible with 8.9.0. So the project would likely have this in the project xml
    core_compatiblity: ^8.9 || ^9 or core_compatiblity: ^8.8 || ^9

Remaining tasks

Finish patch, review

User interface changes

New messages on available updates list

API changes

none

Data model changes

None

Release notes snippet

TBD

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Issue summary: View changes
Status: Active » Needs review

Here patch with tests. I need to clean the code to standards still so leaving assigned to myself.

tedbow’s picture

Nope here is patch

tedbow’s picture

Just a couple thoughts/questions. The patch is still rough so no need for a nit review yet.

  1. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,172 @@
    +  public function __construct(array $core_data, array $core_releases) {
    +    if (isset($core_data['existing_version'])) {
    +      $this->possible_core_versions = $this->getPossibleCoreUpdateVersions($core_data['existing_version'], $core_releases);
    +    }
    +  }
    +
    +  /**
    +   * Set core compatibility information for project releases.
    +   *
    +   * @param array &$project_data
    +   *   The project data as returned by
    +   *   \Drupal\update\UpdateManagerInterface::getProjects() and then processed
    +   *   by update_process_project_info() and
    +   *   update_calculate_project_update_status().
    +   */
    +  public function setReleaseRanges(array &$project_data) {
    

    Originally this class was just static methods. I changed it to have constructor because where the core info is set because that will not change per project.

  2. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,172 @@
    +    $possible_core_update_versions = array_filter($possible_core_update_versions, function ($version) {
    +      return VersionParser::parseStability($version) === 'stable';
    +    });
    

    Currently in the ranges the patch is not taking into account non stable core versions

    So if current version is 8.0.0, and there are updates 8.0.1, 8.0.2 and 8.1.0-alpha1 it will display the message

    This module is compatible with Drupal core: 8.0.0 to 8.0.2

    should this be

    This module is compatible with Drupal core: 8.0.0 to 8.1.0-alpha1

  3. +++ b/core/modules/update/templates/update-version.html.twig
    @@ -21,6 +21,9 @@
    +      {% if version.core_compatibility_message %}
    +        <span>{{ version.core_compatibility_message }}</span>
    +      {% endif %}
    

    If someone has opinions on what the markup should be that would be great.

  4. +++ b/core/modules/update/update.compare.inc
    @@ -90,9 +92,26 @@ function update_calculate_project_data($available) {
    +  $drupal_project_info = NULL;
    

    I no longer need to set this.

  5. +++ b/core/modules/update/update.compare.inc
    @@ -90,9 +92,26 @@ function update_calculate_project_data($available) {
    +  if (isset($projects['drupal']) && !empty($available['drupal']['releases'])) {
    +    $drupal_project_info = $projects['drupal'];
    +    unset($projects['drupal']);
    +    $projects = array_merge(['drupal' => $drupal_project_info], $projects);
    +  }
    

    I move the 'drupal' project to the beginning of the array so that it can calculated first. Before setting other module ranges we have to make sure core is set.

    The other option would just be to call update_calculate_project_update_status($projects['drupal'], $available['drupal']); before the loop below and just skip it in the loop.

    I am thinking now that would be better.

tedbow’s picture

  1. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,172 @@
    +        $release['core_compatibility_ranges'] = $this->createCompatibilityRanges($release['core_compatibility']);
    +        if ($release['core_compatibility_ranges']) {
    +          $release['core_compatibility_message'] = $this->formatMessage($release['core_compatibility_ranges']);
    

    I shouldn't have actually set core_compatiblity_ranges in the release array. Just the message is used later.

    If we put this array it will end up being "array as api" ☹️

    So let's just set the message.

    This allows keeping class level cached messages instead of the ranges so refactoring some other stuff in this class.

  2. fixing #4.5
  3. 
    --- a/core/modules/update/tests/modules/update_test/aaa_update_test.sec.8.x-1.2.xml
    +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.sec.8.x-1.2.xml
    
    @@ -17,6 +17,7 @@
    
    @@ -17,6 +17,7 @@
        <name>aaa_update_test 8.x-1.2</name>
        <version>8.x-1.2</version>
        <tag>DRUPAL-8--1-2</tag>
    +    <core_compatibility>^8.1.0</core_compatibility>
    

    This actually a change I didn't up needing because I not using this test file. Reverting.

Status: Needs review » Needs work

The last submitted patch, 5: 3096078-5.patch, failed testing. View results

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Needs work » Needs review
Related issues:
FileSize
10.26 KB
22.38 KB

Some general clean and the items below. Read for review I think.

  1. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,189 @@
    +    // 'recommended' and 'latest' will be single version numbers if set.
    ...
    +    // If set 'also' will be an array of version numbers.
    ...
    +    // If 'security updates' exists it will be array of releases.
    

    Removing this inline comment and replacing them with a better @param doc for $project_data that documents what the keys of the array that this method will use.

  2. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,189 @@
    +          if ($previous_version_satisfied) {
    +            // Make the previous version be the second item in the current
    +            // range.
    +            $range[] = $previous_version_satisfied;
    +          }
    

    This logic around $previous_version_satisfied is not longer needed. Removed

  3. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,189 @@
    +          $range_message .= " to {$core_compatibility_range[1]}";
    

    This part of the message also needs to be translated. fixed

  4. +++ b/core/modules/update/update.compare.inc
    @@ -90,9 +92,20 @@ function update_calculate_project_data($available) {
    +    $project_core_compatibility = new ProjectCoreCompatibility($projects['drupal'], $available['drupal']['releases']);
    

    $available['drupal']['releases'] is not set in all the test XML which caused the fails. This could happen for real I guess so will check first

Gábor Hojtsy’s picture

The patch looks good. I read the issue summary. It is not clear why do we need to add this new messaging though, that is not explained.

Also I don't think a Critical task is a valid combination.

tedbow’s picture

Priority: Major » Critical
Issue summary: View changes

@Gábor Hojtsy ok thanks. I update the summary to list why need this.

I marked this has critical because @xjm marked #3076183: [Meta] Determine how available updates for modules should be handled if they are not compatible with the current version of Drupal core as critical and this implements it's first step. Also in that issue @catch said these changes should get in even during 8.8.0-rc phase. So that would mean we have very little time

bnjmnm’s picture

Status: Needs review » Needs work
FileSize
22.4 KB
  1. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,187 @@
    +   *
    ...
    +   *   \Drupal\update\UpdateManagerInterface::getProjects() and then processed
    +   *   by update_process_project_info() and
    +   *   update_calculate_project_update_status(). If set the following keys are
    +   *   used in this method:
    

    Add a comma after "If set"

  2. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,187 @@
    +      // If we can't determine the existing version then we can't calculate
    +      // the core compatibility of based on core versions after the existing
    +      // version.
    

    Ultranit: "the" in the second line can be moved to the first.

  3. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,187 @@
    +      foreach ($project_data['security updates'] as &$security_update) {
    

    Is it necessary for $security_update to be passed by reference since it's not being changed, just added to an array?

  4. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,187 @@
    +   *   An array compatibility ranges. If a range array has 2 element then this
    

    Ultranit: pluralize "element"

  5. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,187 @@
    +        // If core version does not satisfy the constraint and there is a non
    +        // empty range add it to the list of ranges.
    

    Ultranit: add comma after "range"

  6. +++ b/core/modules/update/templates/update-version.html.twig
    @@ -21,6 +21,9 @@
    +      {% if version.core_compatibility_message %}
    +        <span>{{ version.core_compatibility_message }}</span>
    +      {% endif %}
    

    I feel like there there'd be benefit in adding this to Stable so it's visible to 8.x users in everyday use. Adding a span -seems- acceptable, but I looked through Stable's commit history for the past 2 years and couldn't find any firm precedent. The closest was https://www.drupal.org/node/77245, where a wrapper was added to messages.

  7. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -444,6 +444,39 @@ public function testHookUpdateStatusAlter() {
    +   * Tests that core compatibility messages.
    

    This comment seems incomplete.

  8. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -570,4 +603,18 @@ public function securityUpdateAvailabilityProvider() {
    +  /**
    +   * @param $release_label
    +   * @param $release_url
    +   * @param $compatibility_range
    +   */
    

    These need types and comments and $release_title needs to be added to meet coding standards

  9. +++ b/core/modules/update/tests/src/Unit/ProjectCoreCompatibilityTest.php
    @@ -0,0 +1,136 @@
    +   * Dataprovider for testSetProjectCoreCompatibilityRanges().
    

    Ultranit: make Dataprovider two words.

tedbow’s picture

Status: Needs work » Needs review
FileSize
9.44 KB
22.51 KB

@bnjmnm thanks for the review

  1. fixed
  2. fixed
  3. It is. It is added to the array by reference and then finally loop in the array loops through this array and sets the messages by reference. I did it this way because we are getting releases from 2 different places under $project_data but in both cases we want to check if $release['core_compatibility'] and then set the message if it is. Having 1 array, $releases_to_set that is an array of references to the releases allows us to have 1 loop that sets messages

    Right now there is test coverage if the loop doesn't set by reference \Drupal\Tests\update\Functional\UpdateContribTest::testCoreCompatibilityMessage() will fail. But right now \Drupal\Tests\update\Unit\ProjectCoreCompatibilityTest::testSetProjectCoreCompatibilityRanges() won't fail. Since the unit test provide a more clear documenting of \Drupal\update\ProjectCoreCompatibility::setReleaseMessage()'s functionality I will add test coverage there too. It just involves moving 1 of the releases under the expected project data to security updates.

  4. fixed
  5. fixed
  6. That sounds good. A changed the test's $defaultTheme to see how this looked in Bartik and the test failed ☹️

    Looks like stable and claro override this template so these will have to updated also. but we can't update claro correct? Anyways I am not addressing it in this patch.

  7. fixed
  8. Fixed to standards but looking at this method a made a couple other changes.

    I don't think we actually need to test the link href here because that is tested in other methods. This makes testCoreCompatibilityMessage() and assertCoreCompatibilityMessage() both simpler.

    changed $release_label to $versionand the last 2 parameters to $expected_compatibility_range, $expected_release_title.

  9. fixed
lauriii’s picture

We should definitely add this to Stable.

+++ b/core/modules/update/templates/update-version.html.twig
@@ -21,6 +21,9 @@
+      {% if version.core_compatibility_message %}
+        <span>{{ version.core_compatibility_message }}</span>
+      {% endif %}

I think we should add this to Stable too to make sure that this feature can be used in as many themes as possible. Theoretically, this could cause regressions on a heavily customized version of the page but it still seems like a better idea to add this to provide the feature to our users. I'm wondering if we actually need the span given that we are not adding any classes to it?

We should also update Claro 😇

tedbow’s picture

  1. Added a class to the template update-version.html.twig
  2. Added this template to stable and claro
bnjmnm’s picture

Status: Needs review » Needs work

One tiny nit and that should do it

+++ b/core/modules/update/src/ProjectCoreCompatibility.php
@@ -0,0 +1,187 @@
+   *   - latest (string): A project version number.

This should be latest_version

tedbow’s picture

Status: Needs work » Needs review
FileSize
858 bytes
24.27 KB

fixed

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

No more nits to be found, RTBC.

lauriii’s picture

+++ b/core/themes/claro/templates/admin/update-version.html.twig
@@ -19,6 +19,9 @@
+        <span class="project-update__core-compatibility-message">{{ version.core_compatibility_message }}</span>

+++ b/core/themes/stable/templates/admin/update-version.html.twig
@@ -19,6 +19,9 @@
+        <span class="project-update__core-compatibility-message">{{ version.core_compatibility_message }}</span>

+1 to adding the HTML class here because this template doesn't have Classy equivalent and it's more consistent with the rest of the template. Opened a follow-up to clean up that: #3100204: Remove visual CSS classes from update-version.html.twig.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 3096078-15.patch, failed testing. View results

bnjmnm’s picture

Status: Needs work » Reviewed & tested by the community

Switching back to RTBC, the fail was in an unrelated test of the unpredictable-FunctionalJavascript variety.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Reviews as well. Looks good to me, other than the following :)

+++ b/core/modules/update/src/ProjectCoreCompatibility.php
@@ -0,0 +1,187 @@
+          $range_messages[] = t('@start to @end', ['@start' => $core_compatibility_range[0], '@end' => $core_compatibility_range[1]]);
...
+      $this->compatibilityMessages[$core_compatibility_constraint] = t('This module is compatible with Drupal core:') . ' ' . implode(', ', $range_messages);

While t() is not deprecated, as per its docs:

When possible, use the \Drupal\Core\StringTranslation\StringTranslationTrait $this->t()

Also, the '@start to @end' string would be very little to provide context for translators as to what it means. Is it a distance or dates or percentages? It may need different translations based on that. I would suggest to change it to '@low_version_number to @high_version_number' or somesuch. Alternatively you can add an explicit context to the string but IMHO once we are specific enough with the placeholders, like @low_version_number, it will not need more context.

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
24.34 KB

@Gábor Hojtsy good catch. Yes I think was using StringTranslationTrait but ran into a problem with the unit test.

Didn't know I could just do

$project_compatibility->setStringTranslation($this->getStringTranslationStub());

Fixed. and used your suggested translation placeholders. Much clearer.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/src/ProjectCoreCompatibility.php
@@ -0,0 +1,190 @@
+          $range_messages[] = $this->t('@low_version_number to @high_version_number', ['@start' => $core_compatibility_range[0], '@end' => $core_compatibility_range[1]]);

Should be changed in the replacement array as well :D

tedbow’s picture

Status: Needs work » Needs review
FileSize
24.37 KB
996 bytes

🤦‍♂️ I ran the unit test locally after I switched to the trait but not after I update the placeholder 😞

fixed

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed the feedback from @Gábor Hojtsy was addressed.

  • Gábor Hojtsy committed 0168e96 on 9.0.x
    Issue #3096078 by tedbow, bnjmnm, Gábor Hojtsy, lauriii: Display core...

  • Gábor Hojtsy committed 5be273c on 8.9.x
    Issue #3096078 by tedbow, bnjmnm, Gábor Hojtsy, lauriii: Display core...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Superb, thanks for fixing that. No other complaints, and this is a fundamentally important improvement. Thanks a lot!

dww’s picture

Status: Fixed » Needs work

Super exciting to see this so far along! Mostly looks really good. I did *not* yet closely review all the test coverage. However, in the rest of the patch, I found some nits, and a few concerns of substance:

  1. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    + * Utility class to set core compatibility messages for module updates.
    

    Would love to have a little more substance in this comment. This class is doing some fairly complicated things, and it'd be nice to get an overview without having to read the whole thing...

  2. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    +class ProjectCoreCompatibility {
    ...
    +   * Constructs an UpdateProjectCoreCompatibility object.
    

    Comment doesn't match the class name.

  3. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    +   *   The project data for Drupal core as returned by
    ...
    +   *   The drupal core available releases.
    ...
    +   *   The drupal core available releases.
    

    Nit: We should be consistent with capitalization of "Drupal core".

  4. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    +    if (isset($core_data['existing_version'])) {
    +      $this->possibleCoreUpdateVersions = $this->getPossibleCoreUpdateVersions($core_data['existing_version'], $core_releases);
    +    }
    

    If there's no "existing_version", this whole class is meaningless, right? Shouldn't this throw an exception instead of quietly being a no-op class?

  5. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    +   * @param string $existing_version
    +   *   The core existing version.
    

    Don't we mean "The currently installed version of Drupal core" or something? Wouldn't this be better as $installed_core_version or so?

  6. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    +      // If we can't determine the existing version then we can't calculate the
    +      // core compatibility of based on core versions after the existing
    +      // version.
    

    "core compatibility of based on core" doesn't parse. Comment seems to be missing some word(s).

    Also, see above about if this should be throwing an exception instead of silently becoming a no-op class.

  7. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    +  /**
    +   * Sets core compatibility messages for project releases.
    +   *
    +   * @param array &$project_data
    +   *   The project data as returned by
    +   *   \Drupal\update\UpdateManagerInterface::getProjects() and then processed
    +   *   by update_process_project_info() and
    +   *   update_calculate_project_update_status(). If set, the following keys are
    +   *   used in this method:
    +   *   - recommended (string): A project version number.
    +   *   - latest_version (string): A project version number.
    +   *   - also (string[]): Project version numbers.
    +   *   - releases (array[]): An array where the keys are project version numbers
    +   *     and the values are arrays of project release information.
    +   *   - security updates (array[]): An array of project release information.
    +   */
    

    Maybe add @see comments at the end of this to make it easier to find where all these values are documented? "A project version number" on its own isn't particularly illuminating. But we don't necessarily want to re-document what "recommend" vs. "latest_version" vs. "also" actually means. So maybe @see as a compromise?

  8. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    +   * @param string $core_compatibility_constraint
    +   *   A Composer semantic version constraint.
    ...
    +   * @param string $core_compatibility_constraint
    +   *   A Composer semantic version constraint.
    

    To the untrained eye, it's not clear what "Composer semantic version constraint" means here. What's Composer-y about this? It's that we're using composer-esque constraint syntax, right? Is there a @see we should use as a reference? Or can we drop the Composer(tm) from this? Or better explain what we mean?

  9. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -0,0 +1,190 @@
    +      $this->compatibilityMessages[$core_compatibility_constraint] = $this->t('This module is compatible with Drupal core:') . ' ' . implode(', ', $range_messages);
    

    While we're considering translatability of this code, isn't this line impossible to properly translate in Arabic or another RTL language?

    Don't we need something like:

    $this->t('This module is compatible with Drupal core: @version_range_messages', ['@version_range_messages' => implode(', ', $range_messages)]);
    

    ?

The last point is the only thing that's truly NW-worth of this review. Everything else is mostly comments and cosmetic nits. But I don't see how RTL languages can make this work if it's hard-coded to have Text . ' ' . stuff ordered in the code.

p.s. Ugh, @Gábor Hojtsy already committed this while I was writing this comment. :(

dww’s picture

p.s. Is this a candidate for an 8.8.x backport? Wasn't that something @catch said about trying to get this round of update.module improvements in ASAP? I know that'd break normal commit strategy, but this is a bit special-case-y. Although with new strings to translate, it's a bit more disruptive to backport into 8.8.1 at this point.

Gábor Hojtsy’s picture

Just briefly documenting the discussion we had with @dww on slack. The lack of use of placeholder is not an RTL concern, the source order in LTR and RTL is the reading order and thus the same. The source order does not need to be in a different direction for RTL languages, the browser will display the content in a different direction from the source order though for RTL content.

Having a placeholder approach would be somewhat better for cases where you need to translate in a way that the version list is within a sentence, but Drupal is generally oriented to a Field name: value kind of display already, so I did not consider this a deal breaker. Doing that would allow for a translation in the style of This module is compatible with 8.8.0 to 8.9.3, 8.9.5 to 8.9.9, 9.0.0 to 9.0.10 versions of Drupal core, but making it a sentence would likely break scannability of the UI in such a translated case.

dww’s picture

Status: Needs work » Needs review
FileSize
5.57 KB

This fixes most of #28:

1. Fixed.
2. Fixed.
3. Fixed.
4. TBD
5. Hah! The "existing" terminology is from code I wrote a bazillion years ago in other parts up update.module and friends. ;) Leaving this mostly as-is, slightly improving the doc comment.
6. Fixed the comment. No exception thrown, pending #4.
7. Fixed.
8. Fixed: I removed 'Composer' from these comments.
9. Untouched. Per Slack discussion and #30 from @Gábor Hojtsy - I'm wrong and this actually does work for RTL. I confess to still not totally understanding why. ;) But if anyone would know, it's @Gábor Hojtsy, so I'll defer to his wisdom on this. ;)

lauriii’s picture

+++ b/core/modules/update/src/ProjectCoreCompatibility.php
@@ -21,14 +38,17 @@ class ProjectCoreCompatibility {
+   * Constructs a ProjectCoreCompatibility object.

Nitpick: Let's remove the class name from here. It doesn't provide any insights to the reader and it's hard to keep up to date.

tedbow’s picture

Status: Needs review » Needs work

@dww thanks for updated comments. Most look good to me just some thoughts on the class comment

  1. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -8,6 +8,23 @@
    + * This class checks all available updates for Drupal core and saves that into a
    + * protected class member variable ($possibleCoreUpdateVersions). The only
    + * public method, setReleaseMessage(), constructs a list of versions of Drupal
    + * core that a given release is compatible with. The compatible versions are
    
    @@ -21,14 +38,17 @@ class ProjectCoreCompatibility {
    +   * Cache of core compatibility messages per core version constraint.
    +   *
    +   * Keys are core version constraint strings, values are human-readable
    +   * messages about the versions of core that version constraint maps to.
    ...
       protected $compatibilityMessages = [];
    

    My preference would be that we don't document the internal workings of the class such as the protected member in the class doc.

    The updated description on $possibleCoreUpdateVersions seems like a good idea I don't think it also needs to be explained in the class doc. For a consumer of the class it isn't important and just is more to read to figure out how to use the class. Also from a maintainability standpoint we also have to update the class doc if the internals change.

  2. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -8,6 +8,23 @@
    + * displayed in ranges to minimize how much text is generated. The list of core
    + * versions compatible with a given core version constraint are cached in the
    + * $compatibilityMessages protected member variable (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.
    

    Again I don't think explaining the internal functioning of the class belongs in class doc. I think if this needs more explaining it should either be inline where we check

    if (!isset($this->compatibilityMessages[$core_compatibility_constraint])) 
    

    or in the $compatibilityMessages comment

  3. +++ b/core/modules/update/src/ProjectCoreCompatibility.php
    @@ -8,6 +8,23 @@
    + * The list of compatible core versions is used on the available updates report
    + * to show site builders which versions of core a given available update can be
    + * installed with. Since individual releases of a project can be compatible with
    + * different versions of core, and even multiple major versions of core (for
    + * example, 8.9.x and 9.0.x), this list will hopefully help site builders know
    + * which available updates they can upgrade a given project to.
    

    I feel like this description would be more useful where \Drupal\update\ProjectCoreCompatibility::setReleaseMessage() is called in update_calculate_project_data() rather than here with maybe a @see to that function

    This connects it directly to the available updates report and if we were to use this someone else in the future we would also have to add the use case of the new place we called it.

  4. re #28.4 Yes we could throw an exception but if we do we should also create a follow up do the the same in update_calculate_project_update_status which also will not function if this is not set. Or maybe we could handle that in #3100110: Convert update_calculate_project_update_status() into a class

    Another option for the current class might be to leave these lines the way it is but in \Drupal\update\ProjectCoreCompatibility::createMessageFromCoreCompatibility() if existing_version is empty set the message to be something like "Core compatiblity could not be determined because unable to determine installed version"

  5. I think we should take this opportunity to make this class @internal. I could see some changes that we might need to make in the future that would be easier as an internal class.

    For instance most of \Drupal\update\ProjectCoreCompatibility::setReleaseMessage() is logic to determine which releases will actually be shown on the report. I could see where we might need this somewhere else and want to take this logic out of this class.

dww’s picture

Assigned: Unassigned » dww

@lauriii re: #32 -- I think it's generally pointless to have a docblock for constructors at all. ;) But core seems to say "Constructs a X object." everywhere. If we want to stop doing that, seems we should change our standards and do it consistently and update everything. Special-casing single classes where we say "Constructs the object." (what else would it say?) seems a little weird at this point. Can you clarify?

@tedbow re: #33 -- Sure, makes sense. I tend to fall on the side of verbose comments, but yeah, you're probably right on all 5 points. I do think the class docblock needs more than the 1-liner ("Utility class to set core compatibility messages for module updates."), but yeah, we should move the explanation of the internals closer to where it's all happening.

Also:

+++ b/core/modules/update/src/ProjectCoreCompatibility.php
@@ -8,6 +8,23 @@
+ * $compatibilityMessages protected member variable (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.

While we're re-writing this and moving this text around, we should fix the fact I didn't close the parenthesis. ;)

I'll take another stab at this. Stay tuned.

dww’s picture

Assigned: dww » Unassigned
Status: Needs work » Needs review
FileSize
6.49 KB
3.73 KB

Fixes (I think) everything from #33 except point 4 (see also #28.4). I'm still not sure the right approach for that. Honestly, this seems like assert() territory to me, since I don't know how update.module can do anything if it can't figure out your current version of core. ;) But that's probably better handled in a follow-up. I guess leaving it empty/no-op is best. If you *were* on some bizarrely unknown core version, I'd rather the available updates stayed relatively quiet, instead of repeating "Core compatibility could not be determined because unable to determine installed version" next to every single release.

Speaking of which... did we do any usability review of this change? I see no screenshots. Having only looked at the code and not tested on a real site, it seems like we might be duplicating a *lot* of text on the available updates report now. I don't see any discussion of that above, nor any attempts to minimize the noise or make it less distracting.

Tempted to retroactively tag this for "Needs usability review".

tedbow’s picture

Status: Needs review » Needs work
FileSize
80.56 KB
  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.

Gábor Hojtsy’s picture

Status: Needs work » Fixed

Since this followup discussion is now going on for almost a week, it does not seem like the quick fix it was meant to be. Since it does not seem to concern the working parts of the patch, I don't see a reason to roll this back. It would therefore be more useful to open a followup for the fixes. Otherwise the additional work here messes with issue/commit credit data. The current patch is not a critical task, while the one committed was, etc.

Opened #3101547: Clean up code documentation for display of core compatibility ranges for available module updates and transferred the patch and credits.

dww’s picture

Status: Fixed » Needs review

Fair enough. I was thinking of doing the same, once I realized the RTL thing was a false alarm.

However what about this from #35?

Speaking of which... did we do any usability review of this change? I see no screenshots. Having only looked at the code and not tested on a real site, it seems like we might be duplicating a *lot* of text on the available updates report now. I don't see any discussion of that above, nor any attempts to minimize the noise or make it less distracting.

Tempted to retroactively tag this for "Needs usability review".

I'm still not convinced this patch should have been committed in the state it was. We missed 8.8.0. So there's not really a rush...

Thanks,
-Derek

tedbow’s picture

FileSize
90.81 KB
88.71 KB
  1. We missed 8.8.0. So there's not really a rush...

    I don't think we should rush things but the importance of this issue is that it blocks #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates which is the last blocking issue here: #2608062: [META] Requirements for tagging Drupal 9.0.0-alpha1

  2. I'm still not convinced this patch should have been committed in the state it was.

    I personally don't think the state was that bad. I think #3101547: Clean up code documentation for display of core compatibility ranges for available module updates will add better docs but I don't the docs were expectionaly lacking in the commit. Most of the stuff I see in is #3101547 is more explanation not doc standards problems or missing docs.

  3. it seems like we might be duplicating a *lot* of text on the available updates report now

    In hindsight I think we could have a usability review but again I think that could probably be said for most issues that add text to the page to any part of core. We should get better with that but I don't think it is reason to revert this issue.

    Here are 2 screenshots of the tests, 1 wide screen, 1 narrow. I don't think there are issues that couldn't be addressed in a follow-up
    wide screenshot
    narrow screenshot

    Right now there is a bug on drupal.org with the XML so we can't do a screenshot with live XML. https://www.drupal.org/project/drupalorg/issues/3074998#comment-13395145 . . It is fixed but will take ~8hrs to regenerate the XML.

    Yes, there will be text duplicated on the report but that is because many modules will have the same compatibility. I think we should prefer robustness here especially because after #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates we will have Drupal 8 listing Drupal 9 only updates and Drupal 9 listing Drupal 8 only modules. Again we can actually make the filtering better but you for the foreseeable you will see updates that aren't compatible with your current installed core version.

dww’s picture

Issue summary: View changes
Status: Needs review » Fixed

Re #39: thanks for the replies!

39.1: Ok, makes sense. I didn't realize this was the last blocker. Agreed.

39.2: Sorry that came across harsh. You're right, this wasn't in that bad shape. It's just that many other core patches are stopped for more trivial things than UX review + documentation. #3101547: Clean up code documentation for display of core compatibility ranges for available module updates is definitely not critical.

39.3: Agreed, moved to another child issue: #3102724: Improve usability of core compatibility ranges on available updates report Not sure if/where to add that as a "should have". ;)

Cheers,
-Derek

tedbow’s picture

Sorry that came across harsh.

No worries. Thanks for enagining in this issue and opening the follow-up

Status: Fixed » Closed (fixed)

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

xjm’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Closed (fixed) » Needs review
Issue tags: +Needs usability review

We're discussing backporting this to 8.8.x, which makes sense (even though it's a string addition and a UI change) because 8.8 users will need this info too until 8.8's end of support in December 2020.

However, I think this issue should probably have had a usability review before going in. I'd like to do that before backporting since we can refine it in as many issues as we want in 8.9/9.0, but once it's in 8.8 any further improvements would be a string break. So reopening as NR/Needs usability review and pinging someone who may have thoughts. :)

xjm’s picture

I missed #3102724: Improve usability of core compatibility ranges on available updates report, but let's apply the usability gate before commit next time. :)

xjm’s picture

Status: Needs review » Postponed
Issue tags: -Needs usability review
tedbow’s picture

tedbow’s picture

Status: Postponed » Patch (to be ported)

#3102724: Improve usability of core compatibility ranges on available updates report has been committed.
So not sure how this should work should this be committed to 8.8.x and then immediately #3102724 or a combined patch?

tedbow’s picture

Status: Patch (to be ported) » Needs review
FileSize
24.35 KB

8.8.x reroll

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Yay passing against 8.8.x

  • Gábor Hojtsy committed 5315039 on 8.8.x
    Issue #3096078 by tedbow, bnjmnm, Gábor Hojtsy, lauriii: Display core...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed this to 8.8.x, so we #3102724: Improve usability of core compatibility ranges on available updates report can be rolled against 8.8 as well. Better to use the same issue scopes and not intermix them.

Status: Fixed » Closed (fixed)

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