In #2991207: Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases, we implemented messages for the site owner about the security coverage dates for their installed release. The implementation assumes each minor receives security coverage until two minors later. However, the last 1-2 minor versions of the major series need to have specific dates hardcoded, because there is no "two minors later".
This was done for D8 and D9.
(More details in comment #4.)
Adding appropriately named constants will automatically generate the correct messages (as shown in the test cases).
Add a test version of getCurrentTime().
Change date test so that the warning message is shown on the last day of coverage.
Needs code review.
Manual testing requires faking update module data, because the message is not displayed unless the branch has a stable release.
Alternately, manual testing with changes similar to the below changes (from D9) will display the message as if 10.1 and 10.2 were the final releases instead. (This works because Drupal.org already has stable releases for those branches.)
diff --git a/core/modules/update/src/ProjectSecurityData.php b/core/modules/update/src/ProjectSecurityData.php
index 6c927c5e0f..03e6cd3ab6 100644
--- a/core/modules/update/src/ProjectSecurityData.php
+++ b/core/modules/update/src/ProjectSecurityData.php
@@ -39,13 +39,13 @@ final class ProjectSecurityData {
*
* @see \Drupal\update\ProjectSecurityRequirement::getDateEndRequirement()
*/
- const SECURITY_COVERAGE_END_DATE_9_4 = '2023-06-21';
+ const SECURITY_COVERAGE_END_DATE_9_2 = '2023-06-21';
- const SECURITY_COVERAGE_ENDING_WARN_DATE_9_4 = '2022-12-14';
+ const SECURITY_COVERAGE_ENDING_WARN_DATE_9_2 = '2022-12-14';
- const SECURITY_COVERAGE_END_DATE_9_5 = '2023-11';
+ const SECURITY_COVERAGE_END_DATE_9_3 = '2023-11';
- const SECURITY_COVERAGE_ENDING_WARN_DATE_9_5 = '2023-05-14';
+ const SECURITY_COVERAGE_ENDING_WARN_DATE_9_3 = '2023-05-14';
/**
* The existing (currently installed) version of the project.
Drupal 10.last-1 and Drupal 10.last will properly inform the user of accurate EOL dates.
None.
TBD
Covered until 2024-Dec-09. Update to a supported minor version soon to continue receiving security updates. Visit the release cycle overview for more information on supported releases." />
| Comment | File | Size | Author |
|---|---|---|---|
| #38 | 3378393-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #36 | 3378393-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #31 | 3378393-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #29 | 3378393-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !8184compare 
Comments
Comment #2
quietone commentedComment #3
catchWith https://www.drupal.org/blog/drupal-10-will-be-supported-until-the-releas... I am not sure the best release to add these warnings to any more. 10.3 or 10.4 seems too soon given the longer support cycle, maybe 10.5 or 10.6?
Comment #4
xjmMoving to beta2.
There's another issue somewhere to make this less ooglay, but for D10 we really should just get the EOL dates in there, so de-uglification can happen in D11+.
Based on an August release of 11.0.0 which is now locked in barring some horrible catastrophe, there are two possible paths forward. Both include:
If 12.0.0 comes out in June or August, we don't create a 10.7.0 because we don't want to issue releases for three branches at once.
If we miss the April-May-ish deadline for an August release of 12.0.0, there might be some case for creating a 10.7.0 with backported APIs. However, I think this would be a bad idea since it'd only be supported for six months and would mean managing all of the following branches at once in the spring:
So, even in a December 12.0.0 release scenario, I think we should avoid issuing a 10.7.0. We should just provide security coverage for non-Symfony-6 issues through to the 12.0.0 release date. Besides, it's always better to warn site owners too early over too late.
We also might not need a 10.6. If we decide not to release one, we can simply extend 10.5's security coverage through to the Symfony 6 EOL date.
So I agree that 10.5 and 10.6 are the branches to deal with here.
Comment #5
xjmThese changes don't go into 11.x.
Comment #7
xjmComment #8
xjmComment #10
xjmComment #11
xjmRight, this is actually tested:
Comment #12
xjmAlso I realized my dates for 10.6 are actually wrong; we're not constrained by the Symfony 6 EOL at all.
Comment #13
xjmShould be fixed now.
Comment #14
xjmStill failing for some reason:
Comment #15
quietone commentedI've spent too much time on this today, partially due to having problems with debugging not working.
All failing tests are all using the fixture 'sec.10.6.0'. Locally I get a 'No data available' warning. It seems that when getting the data from KeyValueExpirable, the data is already expired so nothing is returned. If I workaround that and run the test, the display shows "Out of date (version 10.6.0 available)" which is the correct message though.
Comment #16
quietone commentedIt looks like the calculation of the expired time changed in #2991207: Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases. If I revert the changes that commit made to UpdateProcessor.php then the one test case I am testing with passes. That is '10.5.0, supported'
Comment #17
quietone commentedThis was calculating the date difference using the mock date and the current time. With the mock date in the future the time difference was always great enough for the update data in keyvalueexpirable was always expired. When the test got that data it would return an empty array and thus tests were failing when mock dates were used. The solution here is to add a test version of getCurrentTime that will return the mock date.
Tests were failing for the test of the messages on the last day of security coverage. According to the test on the last day of security coverage there should be a warning message. I took that as the intention and changed the relevant if statement in \Drupal\update\ProjectSecurityRequirement::getDateEndRequirement to allow that.
Comment #18
xjm@quietone Thank you for fixing that silly test.
Comment #19
xjmUpdated the IS for the reasoning for the dates.
Comment #20
xjmComment #21
xjmComment #22
xjmConfirmed the date expectations with @catch.
Comment #23
smustgrave commentedSo still not 100% how to test this one
I applied a script that @xjm was kind enough to share
php -r "include 'vendor/autoload.php'; \Drupal\Composer\Composer::setDrupalVersion('.', '10.whichever.whatever');"
And set to 10.5.0
I altered the variables SECURITY_COVERAGE_END_DATE_10_5 and SECURITY_COVERAGE_ENDING_WARN_DATE_10_5 to go back 1 year.
But with or without the patch the page on /admin/reports/updates remains the same
Comment #24
xjmSorry, I didn't do a very good job of explaining that there are two different ways of manually testing this. One involves faking the release data (both in core itself and also what it would get from Drupal.org), the way that the Update module does in its tests.
The other method is to install production versions of core, and hack the constants and dates so that those production versions (say, 10.1 and 10.2, or starting later this week, 10.2 and 10.3) are the ones with specific end-of-life dates (rather than 10.5 and 10.6). The messages won't show up if Drupal thinks you're on an unsupported/invalid release; with the current logic, being on an unsupported release supercedes providing any info about security coverage. (I don't agree with this, FWIW, but it is out of scope just as it was the first time around).
So here's testing it the second way which is probably easier to understand if you haven't worked with the Update module before, the way I suggested in the IS:
Before patch
git checkout 10.2.7; composer installphp core/scripts/drupal quick-start standardNavigate to
/admin/reports/status.Observe the following default message:
With modified patch (pretending 10.2 is what 10.6 will be, and adjusting constants accordingly)
git checkout 10.4.x(so that the patch will apply)curl https://git.drupalcode.org/project/drupal/-/merge_requests/8184.diff | git apply --index -rm -rf vendor; composer installModify the patch with the following diff, to pretend our faux-10.2.7 is behaving as we expect 10.6.7 to (as per the IS suggestion):
With the current date, we now expect a warning about 10.2's encroaching EOL.
php core/scripts/drupal quick-start standardNavigate to
/admin/reports/status.Observe the following warning:
You can continue to adjust dates and versions from there to see what the messages would look like before the warning date, or after the EOL date. It "just works" with the appropriately named constants and appropriate dates.
Comment #25
xjmOh also note that this has nothing to do with
admin/reports/updates. It affects a status report message shown for stable, known core versions.Comment #26
xjmComment #27
xjmComment #28
xjmComment #29
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #30
quietone commentedComment #31
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #32
quietone commentedI think the bot is is mistaken. Anyway, I rebased and there were no conflicts.
Comment #33
dwwThanks for working on this! These tests and the logic they're testing are pretty confusing and weird. I tried to review as best I could. I think there are some errors in the MR as currently written. Although I'll quickly grant I could be confused myself. 😅 Anyway, I think this needs another look before it's RTBC. Bare minimum there's a faulty comment to remove (nit). But in addition to changing the dates, the MR is also currently changing the behavior of the requirements messages, and I'm not sure it should be doing that. Locally, by fixing a few of the mock dates in
UpdateSemverCoreSecurityCoverageTest, everything passes with no logic change incore/modules/update/src/ProjectSecurityRequirement.php...Comment #34
dwwAfter having only looked at the MR, I've now read the issue and comments, and see that my concern about the logic change was explained in #17. I'm on the fence if I agree with it. But I don't want to die on that hill. If we want to start showing the "Coverage has ended" error as soon as you hit the "coverage ends on" date, that seems fairly reasonable, even if it is a bit of scope creep.
Thanks for #17 also explaining why we're adding
TestTime:: getCurrentTime(). I started down the path of re-debugging why that was needed, but got distracted by the other changes and forgot to open a thread about it. 😅 It might be nice to add a comment about it, but I certainly wouldn't require it here. Could always be a follow-up if we're anxious to get this committed ASAP.Thanks again,
-Derek
Comment #35
quietone commented@dww, thanks for the review.
Comment #36
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #37
quietone commentedRebased and no conflicts.
Comment #38
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #39
quietone commentedAgain, the bot was incorrect.
Comment #40
longwaveI feel like there should be an easier way of doing this, where we just set a constant for "last minor" and the code can calculate everything else by itself from there, which would then be unit testable, but that can wait for next time around I guess.
Otherwise this looks correct to me for Drupal 10 so let's ship this with 10.4.
Comment #43
catchCommitted/pushed to 10.5.x and cherry-picked to 10.4.x, thanks!
Comment #46
xjmAmending attribution.
Comment #47
cilefen commentedIn #3562560: Show both Minor and Major version EOL information on Status Report people have observed that 10.5 is telling them to upgrade, while as I write this, 10.6 is not released.