The 'Update' page has no idea that some updates are incompatible

You could re-label the button to something like "Download selected updates". Then also have it so that any incompatible updates either do not have a checkbox next to them, or their checkbox is unticked+locked, or replace the checkbox with a red x. This would allow to keep the updates in a single table, which would simplify the UI.

Other things to consider if this ^^ works:

• also merging the manual updates in the same table - having a single table and a single submit button at the bottom of the page would improve UI consistency (no additional tables after the submit button)
• denoting incompatible rows with something like different background color and/or text color

Re:2 in #15 if we do stick to having separate table, I reckon having it under core's "Manual updates required" is more logically, as in term of relevance, modules that can be manually updated are far more relevant than those are are incompatible. Taking the order of tables as order of "importance" or "action-ability", it makes a lot more sense for that new table to stick to the bottom. Just my 2cents

For the cases where some extra explanation might be helpful, could it follow the model of the webform module's interface, where it commonly uses a question mark symbol you can hover over for additional help text? I like this model because experienced users get a nice clean interface, but less experienced users have the additional help text easily accessible.

Since #3102724: Improve usability of core compatibility ranges on available updates report was committed to 8.9.x and 9.0.x, we can un-postpone this issue. I am setting the status to NW and updating the title.

#8 mentions last week's meeting of the Usability team, and we discussed this issue again at today' meeting. Recordings (on YoiuTube):

• Drupal Usability Meeting - 2020-02-20
• Drupal Usability Meeting - 2020-02-27

We generally agree with the responses in #20 to some of the earlier suggestions. In particular, color and how to present the table description are up to the admin theme, so they are out of scope here.

We like the approach in #21:

• Separate table
• Below the "Manual updates" table
• No additional description

I am updating the "Remaining tasks" to match.

In particular, leaving off the description gives us flexibility for future enhancements. For example, we may find that a particular update has a new module or composer dependency that has to be installed. Then we can add appropriate text to the row without further changing the title/description of the table.

I will create a follow-up issue to make the cell widths more consistent.

Just one additional bullet on benji's excellent summary...

The reason "why" for the order shown in #21 is because it's both a) what chronologically makes sense (have to update core in order to not have versions incompatible with core), and b) there might potentially be 5-6 things in that "not compatible" list, and this puts the necessary core update's visibility higher in the list so it doesn't get lost.

Thanks for your work @dww. I reckon we also need a quick summary/description under the table heading to add on to what actions could be taken by the site admin. As you mentioned, this screen is more for less experienced users, so some guidance/explanation here would help/don't send them into panic mode.
#20 I agreed, it definitely needs a follow-up issue to clean up, a table with one row is not very useful, and also taking up a fair bit of space too.

Didn't mean to change the title and status, my google chrome restarted and was carrying data from pre#23 (???)

re #27I think it would make sense to extend \Drupal\Tests\update\Functional\UpdateContribTest::testCoreCompatibilityMessage() to handle also checking admin/modules/update. Might be able to leave that method untouched actually and just update \Drupal\Tests\update\Functional\UpdateContribTest::assertCoreCompatibilityMessage() which it uses.

testCoreCompatibilityMessage() already has test cases for compatible and NOt compatible modules so we just need to load admin/modules/update and make sure they show in the correct table and with the correct message if not compatible.

I chatted with @dww and I was going to add the test coverage as described in #29

1. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -196,10 +196,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      // Drupal core needs to be upgraded manually.
   -      $needs_manual = $project['project_type'] == 'core';
   +      // Drupal core always needs to be upgraded manually.
   +      $needs_manual = $project['project_type'] === 'core';
   

   Although this seem like good change it is out scope for this issue.

2. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -196,10 +196,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $not_compatible = $recommended_release['core_compatible'] === FALSE;
   

   If will give a warning for the core project itself because the key is not set. Fixed

3. Added the tests.

1. re #32
   

   +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -807,30 +810,57 @@ public function testUnsupportedRelease() {
   +    if (in_array($expected_release_title, ['Recommended version:', 'Security update:'])) {
   

   When I added this if here I missed that this actually will remove the existing test coverage for all other types of updates, such as latest version and "also available".

   Adding this for just the new parts of the test as the form at /admin/module/updates doesn't show all types of updates.

   Also returning early now if it is not in this list instead.

2. re #33 this change is better and more readable. I agree the extra page request is made up for by an easier to maintain and understandable test.
3. I think the fail in #34 was an unrelated random fail. It passes locally for me. We will see with this patch

This looks very close. I just have a few requests for making the code a little more readable.

1. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -199,7 +199,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   ...
   +      $not_compatible = isset($recommended_release['core_compatible']) && $recommended_release['core_compatible'] === FALSE;
       

   In ProjectCoreCompatibility.php, I see

           $release['core_compatible'] = $this->isCoreCompatible($release['core_compatibility']);
       

   If this key is set, then the value is already boolean. I think it would be simpler to avoid double negatives and do it this way:

         $compatible = $recommended_release['core_compatible'] ?? TRUE;
       

   and then use !$compatible instead of !$not_compatible later. Or you could name the variable $is_compatible, as in the test.

   Sorry for the nit, but at least it shows that I am reviewing carefully! ;)

2. +      if ($needs_manual || $not_compatible) {
   ...
   +        if ($needs_manual) {
   +          $projects['manual'][$name] = $entry;
   +        }
   +        else {
   ...
   +        }
          }
          else {
       

   The nested conditionals are hard to follow. It would be clearer to structure this as

   +      if ($needs_manual) {
   ...
   +      elseif (!$compatible) {
   ...
   +      }
          else {
       

   In order to be DRY, we could refactor the lines that restructure the render array for table instead of tableselect into a helper function. I consider that a down payment on refactoring the buildForm() method. With the current patch, that method runs to 252 lines, including 143 in a foreach loop.

   If we do this, then I think there is less need for the suggestion in #36.1.

3. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -555,6 +555,9 @@ public function testHookUpdateStatusAlter() {
   ...
      public function testCoreCompatibilityMessage() {
   +    $update_admin_user = $this->drupalCreateUser(['administer site configuration', 'administer software updates']);
   +    $this->drupalLogin($update_admin_user);
       

   I guess we need this because this method calls assertCoreCompatibilityMessage(), and that method now loads a more restricted page.

4. @@ -807,10 +810,15 @@ public function testUnsupportedRelease() {
       *   If the update is compatible with the installed version of Drupal.
       */
      protected function assertCoreCompatibilityMessage($version, $expected_range, $expected_release_title, $is_compatible = TRUE) {
   +    $this->drupalGet('admin/reports/updates');
   +    $assert_session = $this->assertSession();
   +    $expected_range_text = "Requires Drupal core: $expected_range";
   +    $compatible_table_locator = '[data-drupal-selector="edit-projects"]';
   +    $incompatible_table_locator = '[data-drupal-selector="edit-not-compatible"]';
       

   Most of these variables are not used until after the second drupalGet(). I suggest starting the function by defining $expected_range_text, since that is used on both pages. Then skip a line and do the tests on the first page. Then skip a line, define $assertSession and the locator strings, and test the second page. Consider replacing the several "Confirm ..." comments with a single, longer comment after the empty line. Related: do we often use "Confirm" in such comments? Maybe we can leave it out entirely ("There is no table ...") or replace it with "Assert".

5. +1 for the suggestion in #36.5.

The feedback in #40 was excellent, and mirrored my own thinking as I reviewed the previous patch. The new if/elseif/else structure is much clearer.
Thanks!

(cross post. TL;DR +1 for RTBC)

@dww:

Thanks for those updates. I think this is ready to go.

In addition to reviewing the patch in #43, I did one last manual test: download and extract tarballs for redis-8.x-1.2 and token-8.x-1.5; hack Drupal.php to pretend the version is 8.7.10. My Update form looks a lot like the screenshot in the issue summary.

The refactoring is a step in the right direction. We removed 10 lines from the foreach loop that I mentioned in #40. The new helper function (not counting the doc block) is a little less than that. Coincidentally, the net effect compared to HEAD is no change to the number of lines in the foreach loop; we did add lines to buildForm() overall, because of the new block at the end of that function.

+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -199,42 +199,42 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      // If a contributed project is not compatible with the currently installed
+      // version of core, list that project in a separate table, too.
+      $compatible = $recommended_release['core_compatible'] ?? TRUE;

This raised an eyebrow. Since if $recommended_release['core_compatible'] = NULL the $compatible will equal TRUE. This is the opposite of the behaviour in template_preprocess_update_version() which does $core_compatible = !empty($version['core_compatible']); BUT it is the same as the behaviour in core/themes/stable/templates/admin/update-version.html.twig which does {% if version.core_compatible is not defined or version.core_compatible %}

Do we have test coverage when 'core_compatible' is missing?

@alexpott:

I think the code you refer to was all added in #3102724: Improve usability of core compatibility ranges on available updates report. I reviewed that issue, but I may have neglected the templates and preprocess functions a bit.

The code we add in this issue does what we want. If the key is not present, we want the default assumption to be that the project is compatible.

The comment I made in #40.1 of this issue also applies to the preprocess function. That is, we should simply have $core_compatible = $version['core_compatible']; because of the early exit in the preprocess function:

function template_preprocess_update_version(array &$variables) {
  $version = $variables['version'];
  if (empty($version['core_compatibility_message'])) {
    return;
  }
  $core_compatible = !empty($version['core_compatible']);
  // ...

so we never get to the line in question unless $version['core_compatibility_message'] is set. But in ProjectCoreCompatibility.php (one more line of context than I gave in #40.1) we have

        $release['core_compatible'] = $this->isCoreCompatible($release['core_compatibility']);
        $release['core_compatibility_message'] = $this->createMessageFromCoreCompatibility($release['core_compatibility']);

so those two keys are either both set or neither is.

Do we have test coverage when 'core_compatible' is missing?

I am not sure what you want to test.

Note the doc block of update-version.html.twig:

 * Available variables:
...
 * - version:  A list of data about the latest released version, containing:
...
 *   - core_compatible: A flag indicating whether the project is compatible
 *     with the currently installed version of Drupal core. This flag is not
 *     set for the Drupal core project itself.

I am pretty sure that we have test coverage of this template for project = Drupal core. But that is out of scope for this issue, so probably that is not what you are looking for.

Reviewing this now.

Hiding some files since the IS formatting has made the last patch difficult to locate.

Meanwhile, crediting reviewers for helpful feedback, which turns out to be everyone but me. Were there any participants from the UX meeting in #23 who aren't included from the original credits added for #8?

Adding credit for @worldlinemine based on @benjifisher's summary of who was at the UX meeting. (Thanks Benji!)

Oh, and Gábor.

OK, that was a dense little 8 K patch. Note that I haven't read other reviews, #40, or any of the recent discussion yet -- I usually deliberately read the code first after the IS so that I don't bias my own review.

1. --- a/core/modules/update/src/Form/UpdateManagerUpdate.php
   +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   
   @@ -199,42 +199,42 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      // If a contributed project is not compatible with the currently installed
   +      // version of core, list that project in a separate table, too.
   +      $compatible = $recommended_release['core_compatible'] ?? TRUE;
   +
   

   This is one of those "shorter is not always better" situations IMO. The comment also doesn't actually explain what's happening on the following line.

   What's happening on the following line, reading out the null coalescing operator in words, is: If a core compatibility flag is set for the recommended release, then set $compatible to that; otherwise, set it to TRUE.

   Which is... code smell. Null value leads to very un-null Boolean value. "Yeah, having nothing there means it's compatible". For an undocumented magic key in the release data. ("Magic key" is just how things work because all this code dates from ArrayPI days; "undocumented" is perhaps out of scope for this patch.) But, at the least, we need a better inline comment justifying this. And tests for all three of TRUE, FALSE, and NULL logics.

2. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -199,42 +199,42 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          $entry['data']['recommended_version']['data']['#template'] .= ' <div>{{ core_compatibility_message }}</div>';
   +          $entry['data']['recommended_version']['data']['#context']['core_compatibility_message'] = $recommended_release['core_compatibility_message'];
   

   Why're we sticking it in a classless child div? Some divitis up in here.

3. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -199,42 +199,42 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      }
   -
   -      // Based on what kind of project this is, save the entry into the
   -      // appropriate subarray.
   -      switch ($project['project_type']) {
   -        case 'core':
   -          // Core needs manual updates at this time.
   -          $projects['manual'][$name] = $entry;
   -          break;
    
   -        case 'module':
   -        case 'theme':
   -          $projects['enabled'][$name] = $entry;
   -          break;
   -
   -        case 'module-disabled':
   -        case 'theme-disabled':
   -          $projects['disabled'][$name] = $entry;
   -          break;
   +        // Based on what kind of project this is, save the entry into the
   +        // appropriate subarray.
   +        switch ($project['project_type']) {
   +          case 'module':
   +          case 'theme':
   +            $projects['enabled'][$name] = $entry;
   +            break;
   +
   +          case 'module-disabled':
   +          case 'theme-disabled':
   +            $projects['disabled'][$name] = $entry;
   +            break;
   +        }
   

   Uhhh, what is going on with the diff here? As far as I can see, the only change is that the core case is being removed. Which... why? And unrelatedly, why is git detecting it as this massive change? Is there a different difference I missed?

4. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -297,10 +297,40 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    if (!empty($projects['not-compatible'])) {
   +      $form['not_compatible'] = [
   +        '#type' => 'table',
   +        '#header' => $headers,
   +        '#rows' => $projects['not-compatible'],
   +        '#prefix' => '<h2>' . $this->t('Not compatible') . '</h2>',
   +        '#weight' => 150,
   +      ];
   

   This is the core err, central change of the patch.

5. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -297,10 +297,40 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +   * Prepares a row entry for use in a regular table, not a 'tableselect'.
   +   *
   +   * There are no checkboxes in the 'Manual updates' or 'Not compatible' tables
   +   * so they will be rendered by '#theme' => 'table', not 'tableselect'. Since
   +   * the data formats are incompatible, this method converts to the format
   +   * expected by '#theme' => 'table'.
   

   There's a missing comma after "tables" here, but this is just being factored into a method from stuff that used to be inline, so that's at best scope-adjacent.

   That said, the one-line summary of this method is kinda bizarre. Out of context, it's totally unclear why we would care about or expect tableselects. I think we need to add something that says something like, "The data in the $foo array is primarily used by Bar::method() for baz_purpose, so it includes a checkbox element to perform qux_operation."

   Without having read the whole form builder, I'm guessing baz and qux are "In the main update table, where a site owner can select which updates to install."

6. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -555,6 +555,9 @@ public function testHookUpdateStatusAlter() {
       * Tests that core compatibility messages are displayed.
       */
      public function testCoreCompatibilityMessage() {
   ...
        $system_info = [
          '#all' => [
            'version' => '8.0.0',
   

   Hmm. No data provider. But is that maybe a faux-data-provider?

7. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -555,6 +555,9 @@ public function testHookUpdateStatusAlter() {
   +    $update_admin_user = $this->drupalCreateUser(['administer site configuration', 'administer software updates']);
   +    $this->drupalLogin($update_admin_user);
   

   And why didn't we need to add permissions before? Surely the update status report is restricted?

8. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -807,10 +810,13 @@ public function testUnsupportedRelease() {
   +    $expected_range_text = "Requires Drupal core: $expected_range";
   ...
   -    $this->assertContains("Requires Drupal core: $expected_range", $compatibility_details->getText());
   +    $this->assertContains($expected_range_text, $compatibility_details->getText());
   
   @@ -832,6 +838,34 @@ protected function assertCoreCompatibilityMessage($version, $expected_range, $ex
   +      $assert_session->elementTextContains('css', "$incompatible_table_locator tbody tr", $expected_range_text);
   

   Instead of defining $expected_range_text, I would assert Requires Drupal core: $expected_range in both cases. It's usually better for assertions to assert static things, except where they're varying with a data provider or parameter (which $expected_range itself does, but not the "Requires Drupal core:" part), or where they're verbose such that they they make the code harder to read (as with the CSS selectors). In this case, defining the additional variable is actually making the assertion harder to read.

   So, I would undo the change in the first half of the method, remove the variable declaration, and just repeat Requires Drupal core: in the new assertion.

9. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -807,10 +810,13 @@ public function testUnsupportedRelease() {
   +    $this->drupalGet('admin/reports/updates');
   

   ...And why didn't it do this before?

10. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -832,6 +838,34 @@ protected function assertCoreCompatibilityMessage($version, $expected_range, $ex
    +    $this->drupalGet('admin/reports/updates/update');
    

    Okay, this is... kind of weird. We're testing two different pages from within one assertion helper?

11. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -832,6 +838,34 @@ protected function assertCoreCompatibilityMessage($version, $expected_range, $ex
    +    if ($is_compatible) {
    ...
    +    else {
    

    The conditional is also not what I typically expect; hoping this is getting called from something with a data provider.

12. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -832,6 +838,34 @@ protected function assertCoreCompatibilityMessage($version, $expected_range, $ex
    +      $assert_session->elementsCount('css', "$compatible_table_locator tbody tr", 1);
    ...
    +      $assert_session->elementsCount('css', "$incompatible_table_locator tbody tr", 1);
    

    Hmm, hardcoding that there's only one table row makes me wonder about the extent of the test coverage here, and whether there should be more for more combinations.

13. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -832,6 +838,34 @@ protected function assertCoreCompatibilityMessage($version, $expected_range, $ex
    +      $assert_session->elementTextNotContains('css', $compatible_table_locator, 'Requires Drupal core');
    

    I like that this assertion is broader: If there were a bug or difference with the core version number and we asserted the whole thing, this could give a false positive.

Next I'm going to read the other reviews/comments. This is probably NW, but reading what others have said first.

OK, read #38 on which seemed to be the most relevant to the current patch.

1. For #40.1 / #46 / #55.1, two committers have WTFed at the line and asked for more test coverage. So let's fix the inline comment to reference the bit from ProjectCoreCompatibility that @benjifisher explained, and probably expand the test coverage.

   And actually based on #46 / #47 / #48 the situation is kinda worse than the patch alone indicates.

2. Regarding this in #47:

   The code we add in this issue does what we want. If the key is not present, we want the default assumption to be that the project is compatible.

   But why do we want that? We need to justify it (in the inline comment, if not just by refactoring it to be clearer).

3. #43.3 partly answers #55.7, but not entirely. While it makes sense for the page that lets you download code to the filesystem (or the next thing to it) to have the absolute highest permission restrictions, the admin status report should still have some permission requirement, so I'm still confused there. (It might make more sense once I read the whole test, which I'll do tomorrow.

Next I'm going to read both the test and the form builder with and without the patch applied to get more context for a few of my questions in #55, but that'll have to wait until tomorrow since it's late. Un-assigning for now, and NW for at least #55.1 (inline comment and test coverage, at a minimum, even if we scope refactoring things to followups), .5, and .8, as well as any other points for which someone has a fix.

Thanks @dww.

I think we might need FEFM feedback on the markup for the markup/themeability choice in #55.2; I'll ask about it tomorrow.

Sorry about the confusion for #55.6; I read patches bottom-up so my comments are out of order because Dreditor orders them top-down, not by timestamp. So I first said I hoped there was a data provider because of the if/else, and then looked for it and didn't find it.

#55.8 isn't a personal preference BTW; it's a testing best practice for core. There's a reasonable chance other committers would have given the same feedback.

#33 didn't really clear anything up for me, but hopefully reading the whole test will.

Re: point 11, asking for more test coverage is not scope creep. It's asking for more test coverage. And for point 12, tests are a gate; while we don't have to test the whole form here (the other two tables), we should test the possible scenarios for the part that we are changing, including at least two values and a "one of each" case.

Thanks @dww! This looks really promising; will start a review once @tedbow has had a chance to look at it.

Not that I didn't commit #3102724: Improve usability of core compatibility ranges on available updates report, but when the test was added there, it covered the test scenarios that were important for that issue and that page.. However, this test was adding a conditional if/else in the assertion helper, variables checked in assertions, etc., which are warning flags because it can mean we're testing the test (in a way that might give false positives or hide regressions in the future). So either asserting static things, or assuring all the tested cases are covered by a data provider, is usually the best way to go. Plus that weird null coalesce we've talked a lot about; glad to see #63 should provide coverage for it.

+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -199,42 +199,49 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      // If the recommended release for a contributed project is not compatible
+      // with the currently installed version of core, list that project in a
+      // separate table. To determine if the releases is compatible, we inspect
+      // the 'core_compatible' key from the release info array. If it's not
+      // defined, we're dealing with a release from a project that doesn't have
+      // any version-specific core compatibility requirements, so we have to
+      // assume it is compatible. This ensures backwards compatibility for
+      // projects that don't yet define 'core_version_requirement' and are
+      // relying on 'core'.

This comment is slightly wrong but is because of some complications from a few different areas. I just happened to work on some of the related issues.

core_version_requirement doesn't have to be set in a projects info.yml file for $recommended_release['core_compatible'] to be set.

In drupal.org update feeds core_compatibility is set based core_version_requirement if it is set but falls back to core if it is not.

You can see this in the Webform XML feed. Search for 8.x-5.0-rc31 and see that it has 8.x. The info.yml file for that release does not have core_version_requirements set. Because in semver(at least composer's version) this means it compatible with all versions of 8.

So in \Drupal\update\ProjectCoreCompatibility::setReleaseMessage() where we

if (!empty($release['core_compatibility'])) {
        $release['core_compatible'] = $this->isCoreCompatible($release['core_compatibility']);
        $release['core_compatibility_message'] = $this->createMessageFromCoreCompatibility($release['core_compatibility']);
      }

This will almost always be set. The only case it would not be set is
1) If there was problem fetching the drupal core project's XML but not another project's XML because it we can't determine available versions of core we won't set this. This seems unlikely but of course could happen
2) If an alternative update server was being used. Not likely but possible. see #3120655: Determine if there are other update servers besides updates.drupal.org, if not deprecate the ability to have different servers.

I will review the rest of this after breakfast

+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -199,42 +199,42 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-        case 'core':
-          // Core needs manual updates at this time.
-          $projects['manual'][$name] = $entry;
-          break;

This is an unrelated change. Though I agree it may be an improvement we should try make as few changes as possible. I looked this patch locally also ignoring whitespace. I know we have this big section in the diff because this switch block is now in indented. So I am aware reverting this change will not make the actually patch file smaller it is a change we don't need to make.

This case is removed regardless of indentation. But nothing related to this has changed to make us have to make this change.

In the patch we are now setting this above via

if ($needs_manual) {
  $this->removeCheckboxFromRow($entry);
  $projects['manual'][$name] = $entry;
}

This if statement was already present just some of the code was refactor and put into removeCheckboxFromRow(). We don't need to also move $projects['manual'][$name] = $entry; here.

🤔Maybe the only reason this case is removed and the setting changed is that in an earlier version of the patch we did had

-      $needs_manual = $project['project_type'] == 'core';
+      // Drupal core always needs to be upgraded manually. If a contributed
+      // project is not compatible with the currently installed version of core,
+      // list that project as requiring a manual update, too.
+      $needs_manual = $project['project_type'] === 'core' || $recommended_release['core_compatible'] === FALSE;

This was because we were adding the incompatible projects into the manual table with core. So in that version of the patch removing the case and moving to the if block made sense. It does not now.

We could keep all the assigning to $projects[*][$name] = $entry; in the switch statement by doing

case 'module':
        case 'theme':
          if ($compatible) {
            $projects['enabled'][$name] = $entry;
          }
          else {
            $projects['not-compatible'][$name] = $entry;
          }

It seems like that might be better so it would be easier to all the projects be assigned to the various tables in the switch statement.

This is all the non-test comments I have. I do like use having a dedicated for this and will look at the test next.

1. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   +namespace Drupal\Tests\update\Functional;
   +
   +
   +/**
   

   Extra blank line

2. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   + * @todo In https://www.drupal.org/project/drupal/issues/3117229 expand this.
   

   Do we need a todo if the whole purpose of that issue is expand the test coverage?

3. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   +class UpdateUpdateTest extends UpdateTestBase {
   

   I think this should be UpdateFormTest. UpdateUpdate is confusing

4. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   +  public static $modules = [
   

   Should be protected

5. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   +   * Instead of incurring the cost of a true dataProvider and re-installing
   +   * Drupal for every test scenario, setup the initial state of the site once,
   +   * then use a faux data provider to fetch different release history data and
   +   * assert that things behave as expected.
   ...
   +   * Provides data for test scenarios involving incompatible updates.
   ...
   +   * These test cases rely on the following fixtures containing the following
   +   * releases:
   ...
   +  private function getIncompatibleUpdatesTableTests() {
   ...
   +  /**
   +   * Tests the Update form for a single test scenario of incompatible updates.
   +   *
   +   * @param string $core_fixture
   +   *   The fixture file to use for Drupal core.
   +   * @param string $a_fixture
   +   *   The fixture file to use for aaa_update_test module.
   +   * @param string $b_fixture
   +   *   The fixture file to use for bbb_update_test module.
   +   * @param string[] $compatible
   +   *   Compatible recommended updates (if any). Keys module identifier ('AAA' or
   +   *   'BBB') and values are the expected recommended release.
   +   * @param array[] $incompatible
   +   *   Incompatible recommended updates (if any). Keys module identifier ('AAA'
   +   *   or 'BBB') and values are subarrays with the following keys:
   +   *   - 'recommended': The recommended version.
   +   *   - 'range': The versions of Drupal core required for that version.
   +   */
   +  private function doIncompatibleUpdatesTableTest($core_fixture, $a_fixture, $b_fixture, $compatible, $incompatible) {
   

   I think in general we should use an actual @dataProvider method when we have to go through this much complexity to avoid a dataProvider.

   I know we sometimes skip a dataprovider and just have an array of tests cases but when then have to make another doTest*() method as well as faux data provider method to make it understandable, I think we should just use @dataProvider and not worry that the test suite will take a little longer to run.

   That way we leverage more of PHPUnit and it is easier to understand and maintain for any developers who are new to this class. If we just use a standard dataProvider then it is just that much easier because it is pattern people have seen more.

6. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   +   * @see self::getIncompatibleUpdatesTableTests()
   +   * @see self::doIncompatibleUpdatesTableTest()
   

   I searched core I more occurences of
   * @see ::methodOnSameClass()
   97
   than
   * @see self::methodOnSameClass()
   52

   I don't actually know which 1 is more correct. Maybe somebody else does?

7. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   +      'only one compatible' => [
   ...
   +      'only one incompatible' => [
   ...
   +      'two incompatible, no compatible' => [
   

   I like the expanded number of test case 🎉

8. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   +        'b_fixture' => '1_0', // Up to date.
   ...
   +      'only one incompatible' => [
   ...
   +        'b_fixture' => '1_0', // Up to date.
   

   Comments can't appear at the end of the line. Should be moved to the line before

9. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
   @@ -0,0 +1,242 @@
   +    // Determine how many rows we're expecting in each table.
   +    $num_compatible = count($compatible);
   +    $num_incompatible = count($incompatible);
   

   I don't actually think we need to assign this and I don't think it makes it more clear.

   We use each of these 2x.

   Once in an if
   if ($num_compatible) {
   But since these are arrays I think this function the exact same way
   if (compatible) {

   The second time is
   $assert_session->elementsCount('css', "$compatible_table_locator tbody tr", $num_compatible);
   We could just do
   $assert_session->elementsCount('css', "$compatible_table_locator tbody tr", count($compatible));

   So aren't actually saving a call to count() by assigning the variable.

   I think it is more readable because $num_compatible is not as clear as count($compatible).

   Another reason is because if you are using an IDE invoking help on $compatible will give you the help next of @param vs nothing on $num_compatible because there is not doc text associated with it.

10. +++ b/core/modules/update/tests/src/Functional/UpdateUpdateTest.php
    @@ -0,0 +1,242 @@
    +        // Both contribs always use 8.x-1.0 as the currently-installed version.
    ...
    +        // Both contribs always use 8.x-1.0 as the currently-installed version.
    

    I only see "contribs" 1 other place core. Should be "contrib modules"

Here is an interdiff for 71-73.

@dww I putting

[diff]
  renames = copies

in your ~/.gitconfig
will allow it to find the renames. I am positive because I set it up awhile again

@dww thanks for updates
#73

1. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,239 @@
   +        // Both contribs always use 8.x-1.0 as the currently-installed version.
   ...
   +        // Both contrib modules use 8.x-1.0 as the currently-installed version.
   

   Missed 1 "contribs" to "contrib modules"

2. 5) I chatted more with @dww and @xjm about.

   I still think it makes more sense to use real @dataProvider. @dww you did document it well adn I don't think it will be hard to add ongoing test cases. I just think if we use @dataProvider we don't need the documentation because we are using standard feature of our testing framework.

   @xjm mention that FunctionalJavascript test are much slower so not using a dataProvider might make sense in that case.

   All that being said I won't block this issue on it.

couple things I missed before

1. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,239 @@
   +  protected function setUp() {
   

   Needs {@inheritdoc}

2. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,227 @@
   +  protected function setUp() {
   

   Needs {@inheritdoc}

3. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,227 @@
   +   * @param array[] $incompatible
   

   I think this should be @param string[][]

4. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,227 @@
   +  public function testIncompatibleUpdatesTable($core_fixture, $a_fixture, $b_fixture, $compatible, $incompatible) {
   

   Needs array type hint for $compatible, $incompatible. This problem was there on the non-dataprovider version. I just missed it

Just saw #75.
@dww thanks for making this version.

1. As you can see from the patch and interdiff, it doesn't actually save us much in the test class.

   To me this much clearer. I just see @dataProvider I don't have to know anything else to know the relationship between the 2 methods.

2. As the command output. I am not sure why I don't see the exact same out as you.

   In the older version I see "Time: 10.77 seconds"
   and the new version I see "Time: 28.87 seconds"
   so yes 20 seconds more but seems worth it to me.

Thanks @dww and @tedbow for all the effort you have put in here. I only have time to make a few suggestions.

First, the change discussed in #66, #67, and following comments. I agree with @dww on this point. In #44 and #45, @tim.plunkett and I reviewed and approved that bit of moving code around. In other words, we both decided that the change was in scope given the changes needed for this issue. I think that is the right standard to apply, not "as few changes as possible".

Second, we can add some comments as part of this issue, but a proper fix for the "code smell" (the term used in #55.1 for the line $compatible = $recommended_release['core_compatible'] ?? TRUE;) is outside the scope of this issue. I started to create a follow-up issue to do something about that, but then I realized how hard it will be to fix. Currently, ProjectCoreCompatibility::setReleaseMessage() is responsible for adding messages to projects that need a compatibility warning. Asking it to add a flag to projects that do not need such a warning seems like the wrong solution.

I think the right answer is to have a class that "owns" the update information for all projects and can answer questions like "is version foo of core compatible with version bar of the quux module?" What do you think?

Third and last point: I think the problem we are trying to solve with the removeCheckboxFromRow() method (#55.5) is that we have already prepared an array for '#type' => 'tableselect', and now we are trying to modify it for '#type' => 'table',. I would rather pass around a simpler data structure until we know whether it will be used for table or tableselect. Is that in scope for this issue? I think not, and if you agree then I am happy to create a follow-up issue for that.

@benjifisher thanks for the further review.

@dww thanks for the fixes.

1. All looks good to me. Just one other thing I missed that I just noticed.

   +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,230 @@
   +      'administer modules',
   

   We don't need the "administer modules" permission. I just ran the test locally without it and they all pass.

2. I would RTBC after the permission is fixed except for <div>/template question.

Generally we wouldn't want to add <div> elements without at least having an API that would allow adding attributes to it. Given that this issue is critical, I'd say adding the <div> here is fine. We could consider using a <p> element, which would feel slightly better solution in the sense that it usually has some default styling. However, this changes the design. I recommend that if we prefer the design as it is, we should move forward with the current solution and ensure we have opened a follow-up for refactoring the render array to use templates.

@dww thanks for sticking with it! RTBC 🎉

Reviewing now.

I chatted with @xjm and @tim.plunkett about this.

Just couple test changes and some nits

1. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,229 @@
   +      $i = 1;
   ...
   +        $compatible_row = "$compatible_table_locator tbody tr:nth-of-type($i)";
   ...
   +        $assert_session->elementTextContains('css', "$compatible_row td:nth-of-type(2)", "$module Update test");
   ...
   +        $assert_session->elementTextContains('css', "$compatible_row td:nth-of-type(3)", '8.x-1.0');
   +        $assert_session->elementTextContains('css', "$compatible_row td:nth-of-type(4)", $version);
   

   We were trying to figure out a way to make the test depend less on 1) the other of the modules in the table and 2) the particular markup.

   I have made some changes that do this but don't change the overall structure of the test.

2. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,229 @@
   +   * - aaa_update_test.cre_compatibility.8.x-1.2_8.x-2.2.xml
   

   Small typo here; I think it's core_compatibility and not cre_compatibility.

3. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,229 @@
   +      // We never want to see a compatibly range in the compatible table.
   

   compatibility range

4. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,229 @@
   +        // Both contrib modules use 8.x-1.0 as the currently-installed version.
   ...
   +        // Both contrib modules use 8.x-1.0 as the currently-installed version.
   

   Nitpick: "currently-installed" does not need to be hyphenated; "currently" is an adverb.

The test changes in #91.1 were based on the fact that the test was too tightly coupled to the specific markup and order of the modules; while there's a bug, it was on the right track. It is not desirable for tests to be coupled to markup any more than is necessary to ensure that we're testing the right thing. So marking NW and assigning to Ted to fix tomorrow. :)

1. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -199,42 +199,49 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $compatible = $recommended_release['core_compatible'] ?? TRUE;
   

   In #46 @alexpott asked if we have test coverage for $recommended_release['core_compatible'] not being set.

   In #55.1 @xjm also asked if we had test coverage for the TRUE, FALSE and NULL cases.

   I have checked and we do.
   aaa_update_test.8.x-1.2.xml has core_compatibility set will evaluate to $recommended_release['core_compatible'] === TRUE
   bbb_update_test.1_2.xml has core_compatibility set will evaluate to $recommended_release['core_compatible'] === FALSE
   bbb_update_test.1_1.xml does not have core_compatibility set and will mean $recommended_release['core_compatible'] will not be set.

2. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,229 @@
   +   * - bbb_update_test.1_1.xml
   +   *   - 8.x-1.1 is available and compatible with everything.
   ...
   +          'BBB' => '8.x-1.1',
   

   While this fixture does not have core_compatibility set and will therefore cover the test case of $recommended_release['core_compatible'] not being set this comment doesn't actually explain. Being "compatible with everything" could mean core_compatibility is not set but it also could mean
   <core_compatibility>^8</core_compatibility>
   Not everything, but all core version we would be testing. an all version of 8.
   or
   <core_compatibility>*</core_compatibility>
   Truely everything. Not a value hopefully people are using but it is value we do use other test modules info files.

   So I think unless this is well documented we could easily accidently add either 1 of the values above to bbb_update_test.1_1.xml especially in an issue like #3110917: [meta] Fix update XML fixtures bad data

   in that case all the tests would continue to pass but we would lose test coverage the case where $recommended_release['core_compatible']

   I think we should add a comment in bbb_update_test.1_1.xml explicitly saying core_compatibility can not be added to the release there and why.

   we are now relying on not being set where we weren't before this issue.

1. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,229 @@
   +        $assert_session->elementTextContains('css', "$compatible_row td:nth-of-type(2)", "$module Update test");
   +        // Both contrib modules use 8.x-1.0 as the currently installed version.
   +        $assert_session->elementTextContains('css', "$compatible_row td:nth-of-type(3)", '8.x-1.0');
   +        $assert_session->elementTextContains('css', "$compatible_row td:nth-of-type(4)", $version);
   ...
   +        // Both contrib modules use 8.x-1.0 as the currently installed version.
   +        $assert_session->elementTextContains('css', "$incompatible_row td:nth-of-type(2)", '8.x-1.0');
   +        $assert_session->elementTextContains('css', "$incompatible_row td:nth-of-type(3)", $data['recommended']);
   +        $assert_session->elementTextContains('css', "$incompatible_row td:nth-of-type(3)", 'Requires Drupal core: ' . $data['range']);
   

   I think @dww's comment in #101 shows there is some possible value to having the assertion that in asserting the particular column order. I am fine with this. This very unlikely to change.

2. +++ b/core/modules/update/tests/src/Functional/UpdateManagerUpdateTest.php
   @@ -0,0 +1,229 @@
   +      $i = 1;
   ...
   +        $compatible_row = "$compatible_table_locator tbody tr:nth-of-type($i)";
   

   While the order of the modules unlikely change I am less convinced that we need to assert the order all modules in this table.

   yes #3117229: Expand test coverage of Drupal\update\Form\UpdateManagerUpdate we will need to assert that security updates are before other updates it does not follow that we to test the exact order of other modules. For all we know we will have separate test method for that assertion anyways.

   Nowhere in the update functions involved does it document or guarantee they are returned in specific order.

   The order of the projects ultimately comes from our extension list classes which also do not document the order. and their unit/kernel tests don’t explicitly test the order.

   We could add back
   $incompatible_row = "$incompatible_table_locator tr:contains('$module Update test')";
   and get rid of $i++ logic and leave everything else basically the same.

Here is just a demo of what I was trying to explain in #106.2

If we update the fixtures in this way(not at all unlikely without strict comments) we lose test coverage.

@dww thanks for addressing #106.

1. +++ b/core/modules/update/tests/modules/update_test/bbb_update_test.1_1.xml
   @@ -0,0 +1,48 @@
   +To ensure we've got test coverage for the case where the '<core_compatibility>'
   +tag is not defined at all, this fixture does not include that value.
   +-->
   +<?xml version="1.0" encoding="utf-8"?>
   

   Comment looks but will throw an error because the
   <?xml version="1.0" encoding="utf-8"?> need to the first element in an xml file. Just moving that to the top.

2. addressing #107.2

Assign to @tim.plunkett for review

Nice work and thanks for checking #106.1. Can we add a brief column to the data provider documenting that coverage?

@dww, thanks for your reviews and feedback. It's okay to step back on an issue if it's burning you out. Please do let reviewers provide feedback and remember they also care both about landing this critical and ensuring we're not adding other technical debt when we do. And remember that this issue is an allowed change during beta, and actually since there's a regression we'll make an exception here to backport it to a patch release, so this change is not at risk of not getting in. I understand that it can be tough to balance the QA process and covering edgecases under time pressure, but it's okay also for this bug to be fixed and tested in a way that everyone who contributed is confident in it.

Since we do want to backport this to 8.8.x, I queued an 8.8.x test to check that the change does apply there also, and am setting the branch accordingly.

Actually setting the branch.

I think those docs are helpful, especially as they reference which fixtures are used.
The last missing piece is something inline at the top of incompatibleUpdatesTableProvider() that explains what the order of the keys means, essentially repeating the @param docs of testIncompatibleUpdatesTable().

For example, see \Drupal\Tests\layout_builder\Unit\OverridesSectionStorageTest, specifically how testExtractEntityFromRoute() providerTestExtractEntityFromRoute() have their docs done.

It's beautiful 😭😭😭

@tedbow and @dww I really appreciate both of your work on this. I think this is ready to go!

Sorry for being unclear; I am literally asking for the words from #106.1 to be put in inline comments on the specific array items in the data provider that test each of the three logics. That will make it easier to identify the test coverage for that one line, given the brittleness of the behavior that's out of scope to fix for this issue. Something like this...

I am just going off what @tedbow wrote in #106 here and have not validated this myself, so please check that the comments are true and in the right place. If others +1 that this small comment improvement is adding true information then I can still commit this. 🤞Or if it's wrong and someone else fixes it. Sorry for the confusion!