Simplify the wording of messages on the status report about security coverage for the site's installed minor version

Posted this at #2991207-220: Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases, but adding it here, too:

Do we expect end users to understand semver enough to know what "minor version" means? Should we find another way to word these messages to avoid using (introducing?) that terminology on this report?

Had another UX meeting on this today. With @xjm, @webchick, @benjifisher, me, and @ckrina

Here the consensus we came to:

1. for messages where the heading on right in the screenshots in "Supported Minor" change this to one of the following
   1. Covered until 8.3.0
   2. Covered until 2020-Feb-11 (this uses the date format that is already on updates report
   3. Covered until 2021-Feb (to keep similar to above)
2. Remove the first paragraph of text now that heading will provide more information.
3. Make the 2nd and 3rd paragraphs 1 paragraph
4. We did not have time cover the text when the minor is unsupported.
   Here is the current:
   
   But it suggested we try to do something similar, make the heading more informative, combine the 2 & 3 paragraphs

crediting more from the UX meeting

I am working on this.

I am still working on a patch for this issue, but I wanted to point out a problem caused by the decision in #12. I used my WIP and hacked Drupal.php, setting const VERSION = '8.7.7';. This is what I see on my status report:

We now have two different links to /admin/reports/updates/update under the error "Drupal Core Update Status" (link text "Not secure! (version 8.8.2 available)" and "available updates") and one link to /admin/reports/updates under the warning "Drupal Core Security Coverage" (link text "Update to 8.8 or higher").

I think it is confusing to have links to two different tabs on the same page. I also think it is confusing to have links to the same page with different link text, but that is out of scope for this issue. (I think we already have an issue for that anyway.)

On second thought, it was even worse before the decision in #12, since then we had the same link text ("available updates") going to two different tabs ("Available updates" and "Update").

I would like to change the link so that it goes to the same place (Update tab) as the links under "Drupal Core Update Status". Is that in scope for this issue?

Here is a test-only patch. Watch me fail!

I copied the "after" screenshots from #2991207: Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases into the "User interface changes" section of the issue summary for use as the "before" screenshots.

I am still doing some code cleanup, and then I will have to add "after" screenshots.

Here is the full patch, and the testbot is not quite done with the test-only patch.

I still need to add some screenshots. I am not sure when I will have time for that, so I will un-assign myself.

@benjifisher thanks for the patch and the comments

1. re #15

   We now have two different links to /admin/reports/updates/update under the error "Drupal Core Update Status" (link text "Not secure! (version 8.8.2 available)" and "available updates") and one link to /admin/reports/updates under the warning "Drupal Core Security Coverage" (link text "Update to 8.8 or higher").

   I thought this feedback sounded familiar so I checked #2991207: Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases

   Sure enough @benjifisher pointed the same thing a year ago when we were first crafting this message https://www.drupal.org/project/drupal/issues/2991207#comment-12765333

   I do not like having two links to the same page, with different link text. (Although same text, different destinations would be even worse.) Maybe make "Update to 8.6 or higher soon" plain text, not a link. Or we can just delete this line.

   So basically we had already considered the suggestion in #12 in making the message in first place but decided against it for this reason. I would be in favor of not doing #12

Another reason not to link to /admin/reports/updates/update for messages about core is that the "Update" form can't handle updating core. It just says "Manual updates required", and provides significantly less info than the "Available updates" report does for core releases. /shrug

I am setting the status to NW for screenshots and for restoring the second sentence (#20).

@tedbow, thanks for looking up that comment. At least I am consistent!

I am adding some screenshots.

We discussed this issue at the Drupal Usability Meeting - 2020-02-20. Here is what we decided:

1. "Drupal core security coverage" error: Change "No security updates" to "Coverage has ended"
2. Make sure the "Visit release cycle overview" is always a sentence part and not a para (some of them looked questionable)
3. Don't link to the update form on core update message (separate issue) because that form doesn't let you update core.
4. Revert the change to the link target from the patch in #17. After the previous point, both the "Drupal core security coverage" and the "Drupal Core Update Status" messages on the Status report will link to the same thing.

I added #3115145: For core updates, link to list instead of update form as a follow-up issue. That takes care of point (3).

The decision in (4) removes the reason for my objection to removing the second sentence. The link text "Update to [version] or higher" in the "Drupal core security coverage" message is close enough to "Not secure! (version [version] available)" from the "Drupal Core Update Status" message that I do not think it will lead to confusion.

Re: #20. If I promise to stop arguing about it (see above) can we skip the prototype that restores the second sentence? I do not think we are doing anything that relies on the other message. We are not, for example, leaving off a link under the assumption that it will be in that message. We just want to avoid confusion in cases where both messages are present.

I am assigning this issue to myself to work on these points, and to add screenshots.

I think the attached patch addresses all the points raised in #24. I will let the testbot do it thing while I take screenshots.

I made one change that is out of scope:

+++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
@@ -395,8 +395,9 @@ public function testSecurityCoverageMessage($installed_version, $fixture, $requi
     // Ensure that messages are under the correct heading which could be
     // 'Checked', 'Warnings found', or 'Errors found'.
     $requirements_section_element = $requirements_details->getParent();
+    $this->assertCount(1, $requirements_section_element->findAll('css', 'h3'));
     $this->assertCount(1, $requirements_section_element->findAll('css', "h3:contains('$requirements_section_heading')"));

That is: we already check that we have the expected <h3> headers; the additional assertion is that no unexpected ones crept in. I am attaching an updated test-only patch (mostly because of the other updates to the test, not this one).

I created the screenshots by setting protected $defaultTheme = 'seven'; in the test class and then running the test. (I just double checked that I did not commit that little hack.)

Now that we have screenshots, this is really ready for review, unless the testbot disagrees.

BTW the patch in #26 removes some stray <p> tags. The comment in #24 was right that those were not just wrapping lines.

I am adding attribution.

I forgot to unassign myself.

1. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -138,11 +138,16 @@ private function getVersionEndRequirement() {
   -        $requirement['severity'] = $this->securityCoverageInfo['additional_minors_coverage'] > 1 ? REQUIREMENT_INFO : REQUIREMENT_WARNING;
   ...
   +        $requirement['severity'] = $this->securityCoverageInfo['additional_minors_coverage'] > 1
   +          ? REQUIREMENT_INFO
   +          : REQUIREMENT_WARNING;
   

   This line change is out of scope and just adds line breaks.

2. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -181,9 +185,9 @@ private function getVersionEndCoverageMessage() {
   +    $message = ltrim("$message ") . $this->getReleaseCycleLink();
   
   @@ -212,35 +216,36 @@ private function getDateEndRequirement() {
   +    $description = ltrim("$description ") . $this->getReleaseCycleLink();
   

   It took a second for me figure out why ltrim() was being used here. It seems hard to read because the visible space is on the right.

   I realize now it is because it will trim to an empty string if the string is empty otherwise it will put a space between the 2 string.

   I am guessing if it was hard for me to figure out it would be hard for others.

   To me this would be easier to read
   $message .= ($message ? ' ' : '') . $this->getReleaseCycleLink();

   I checked and I only found pattern is only used 1 once(but it might be hard to find)

Sorry I have finished reviewing but didn't want to lose this

I reverted the out-of-scope change aimed at keeping code lines to 80 characters or less.

I used the ltrim("$var ") pattern twice. The attached patch replaces both with a ternary expression, as suggested in #31.2.

I considered making the two functions where ltrim() was used more consistent by changing the variable names to be consistent. I guess that is also out of scope for this issue.

Mostly looks great. Definitely an improvement for the UI!

A few tiny questions on the tests:

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -395,8 +395,9 @@ public function testSecurityCoverageMessage($installed_version, $fixture, $requi
   +    $this->assertCount(1, $requirements_section_element->findAll('css', 'h3'));
        $this->assertCount(1, $requirements_section_element->findAll('css', "h3:contains('$requirements_section_heading')"));
   

   Do we want/need both of these?

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -421,7 +422,7 @@ public function testSecurityCoverageMessage($installed_version, $fixture, $requi
   +    $coverage_ended_message = 'Coverage has ended';
   
   @@ -482,7 +483,7 @@ public function securityCoverageMessageProvider() {
   +        'message' => "$coverage_ended_message $update_asap_message $release_coverage_message",
   

   Do we actually want $coverage_ended_message as a local variable? All the other assertions include the title of the warning/error as human-readable text at the start of each 'message' declaration. Maybe we should re-use that pattern for the few cases we test coverage ended? The string literal takes 5 fewer chars to write, too. ;)

3. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -429,35 +430,35 @@ public function securityCoverageMessageProvider() {
   -        'message' => "The installed minor version of Drupal (8.1), will stop receiving official security support after the release of 8.3.0.Update to 8.2 or higher soon to continue receiving security updates. $see_available_message$release_coverage_message",
   +        'message' => "Covered until 8.3.0 Update to 8.2 or higher soon to continue receiving security updates. $release_coverage_message",
   

   Jah be praised! Since there are no longer multiple <p> we no longer need the weirdness of $see_available_message$release_coverage_message and friends. Huzzah! :)

@dww:

• 35.1: I explained my reason in #26. If that is not worth testing, then I can remove that line.
• 35.2: The other messages are not identical. If you think it is substantially clearer to be more explicit and less DRY, then I can change it. Mostly, I am afraid that the next Usability meeting will require me to change the text again, and I want to do it in one place. ;)
• 35.3: Woot!

1. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -212,35 +214,36 @@ private function getDateEndRequirement() {
   -    $security_coverage_end_timestamp = \DateTime::createFromFormat('Y-m-d', $full_security_coverage_end_date)->getTimestamp();
   ...
   +      $security_coverage_end_timestamp
   +        = \DateTime::createFromFormat('Y-m-d', $full_security_coverage_end_date)
   +          ->getTimestamp();
   

   This line was just move but then line breaks were add. I know that it was indented and makes it more over 80 but I think we should change as little as we can. I don't think this breaks are needed for readability.

2. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -212,35 +214,36 @@ private function getDateEndRequirement() {
   -    $comparable_request_date = $date_formatter->format($time->getRequestTime(), 'custom', $date_format);
   ...
   +    $comparable_request_date = $date_formatter
   +      ->format($time->getRequestTime(), 'custom', $date_format);
   

   This line change is out of scope

3. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -395,8 +395,9 @@ public function testSecurityCoverageMessage($installed_version, $fixture, $requi
   +    $this->assertCount(1, $requirements_section_element->findAll('css', 'h3'));
   

   This may be worth testing but it is out of scope here.

4. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -421,7 +422,7 @@ public function testSecurityCoverageMessage($installed_version, $fixture, $requi
   +    $coverage_ended_message = 'Coverage has ended';
   

   re #35.2 I agree with @benjifisher that we should add the variable to keep it similar to how we are handling other messages that are always the same.

@tedbow, thanks for the review!

Personally, I feel that it should be in scope to improve compliance with coding standards when moving lines around, but I will not fight about it here.

The attached patch makes the changes requested in #37. In particular (#37.4) I did not make the change requested in #35.2. I think we have a majority agreement on that point but not a consensus. Maybe that is good enough for such a minor point.

Looks good to me. RTBC 🎉

rather than passing around and concatenating variables.

I still think it makes sense to assign to variables because they may or may not be set.

1. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -183,9 +183,9 @@ private function getVersionEndCoverageMessage() {
   +      '#markup' => '<p>' . ($message ? ' ' : '') . $this->getReleaseCycleLink() . '</p>',
   
   @@ -239,8 +239,9 @@ private function getDateEndRequirement() {
   +      '#markup' => '<p>' . ($description ? ' ' : '') . $this->getReleaseCycleLink() . '</p>',
   

   These actually don't print $message or $description.

2. Talk to @xjm about this we should be concatenating via multiple render array elements
3. Also we don't actually need the <p> tags anymore that is not 2 paragraphs. I checked other status messages and they don't use them

1. +++ b/core/lib/Drupal.php
   @@ -82,7 +82,7 @@ class Drupal {
   -  const VERSION = '8.9.0-dev';
   +  const VERSION = '8.7.11';
   

   Local cruft?

2. +++ b/core/modules/update/src/ProjectSecurityRequirement.php
   @@ -183,9 +183,17 @@ private function getVersionEndCoverageMessage() {
   +    return $render_array;
   +
      }
   

   Nit, only since you have to post a new patch: blank line

Otherwise, looks good to me.

#45 fixed

#46 Fix to use the separate string vars but the render array doesn't need to initialized first because we are adding a new key regardless

unused use statement

Patch looks great, I think this deserves some screenshots (which also would serve as manual testing).

I am updating the screenshots. I am using the same trick as in #27: setting $defaultTheme = 'seven' and running the automated test, so I am removing the "Needs screenshots" tag but leaving "Needs manual testing" for now.

Here are the latest screenshots. @benjifisher beat me to it.

1. The only difference you will see with previous ones that were just in the summary is there is not space on the right side that was caused by the <p> tags. This is not more like all the other messages.
   Here is example with the message next to other messages.

   
   You can see none have the space.

2. If anyone is interested in doing their own manual test you can either follow the instructions in the issue summary or I am also including a patch with the changes I made to make these screenshots
   It does the following
   • changes the test to a javascript test so you can see the browser
   • makes the test use Seven theme
   • pauses the test on the screen with the message(just close the browser, the test will end and fail )
   • change \Drupal\Tests\update\Functional\UpdateCoreTest::securityCoverageMessageProvider() only return the 1 case I want to look at

I don't think need Manual Testing now

Thanks to both of you! I think this is ready to go

Bot is confused by the 3110186-make-screenshots_0.patch from #52. Re-uploading exactly #49 so the RTBC re-testing stuff is happening on the last patch in the issue.

p.s. Re: #38:

I did not make the change requested in #35.2. I think we have a majority agreement on that point but not a consensus. Maybe that is good enough for such a minor point.

#35.2 Was only a question and a "Maybe we should..." suggestion. I'm totally fine with the majority agreement on keeping it as it was. Let's call it consensus after all. ;)

Thanks,
-Derek

When a 'custom' date format is used, that is not at all translated. A named date format (one that has a corresponding config entity) would be translated. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Datetime%...

string $type:
(optional) The format to use, one of:

• One of the built-in formats: 'short', 'medium',
  'long', 'html_datetime', 'html_date', 'html_time',
  'html_yearless_date', 'html_week', 'html_month', 'html_year'.
• The name of a date type defined by a date format config entity.
• The machine name of an administrator-defined date format.
• 'custom', to use $format.

Defaults to 'medium'.

I don't think you would want to use any of the html_* formats as those are for dates that go direct into markup (eg. HTML attributes) and should not be translated. I don't think any of the other three formats match what you want here. So translatability would need new named date formats (shipped date format config entities) for these purposes here.

Whether that is in the scope of this issue or not would be a release manager question probably :)

Opened #3117120: Project update related date formats are not properly localisable for date formats.

I added #3117157: Refactor Drupal\update\ProjectSecurityRequirement to remove methods returning string to address #56.2 (referred to as #57.2 in #58). I am removing the "needs followup" tag and setting the status to NW for #56.1.

In the follow-up issue, I suggested refactoring/removing both getVersionNoSecurityCoverageMessage() and getReleaseCycleLink(). I hope that is OK, and that my proposed resolution is in line with what you had in mind.

So here is the fix for #56.1

I checked the patch in #63, and it does what it is supposed to do. I would set the status to RTBC, but I contributed earlier patches to this issue.

A warning to anyone else who wants to check this: the interdiff utility gets confused when comparing the patches in #49 and #63. If you look at a diff (not interdiff) between the two patches, you will see just the change in @tedbow's interdiff-49-63.txt and a bunch of changed line numbers.

#63 Solves #56.1.
#56.2 is at #3117157: Refactor Drupal\update\ProjectSecurityRequirement to remove methods returning string
#56.3 is at #3117120: Project update related date formats are not properly localisable

So that's everything since the last RTBC. I did another review and I don't see anything to complain about (which is rare). ;)

Bot is happy. No CS warnings. Back to RTBC.

Thanks, all!
-Derek

Yay thanks all! Let's start this issue domino and see where we end :D