- πΊπΈUnited States dww
After 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 - πΊπΈUnited States dww
Thanks 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
... - π³πΏNew Zealand quietone
I think the bot is is mistaken. Anyway, I rebased and there were no conflicts.
The 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.