Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Discovered at #3111929-31: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported ... the patch that was already RTBC would have broken this block of code from update.compare.inc:
// First, if this is the existing release, check a few conditions.
if ($project_data['existing_version'] === $version) {
if (isset($release['terms']['Release type']) &&
in_array('Insecure', $release['terms']['Release type'])) {
$project_data['status'] = UpdateManagerInterface::NOT_SECURE;
}
elseif ($release['status'] == 'unpublished') {
$project_data['status'] = UpdateManagerInterface::REVOKED;
if (empty($project_data['extra'])) {
$project_data['extra'] = [];
}
$project_data['extra'][] = [
'class' => ['release-revoked'],
'label' => t('Release revoked'),
'data' => t('Your currently installed release has been revoked, and is no longer available for download. Disabling everything included in this release or upgrading is strongly recommended!'),
];
}
elseif (isset($release['terms']['Release type']) &&
in_array('Unsupported', $release['terms']['Release type'])) {
$project_data['status'] = UpdateManagerInterface::NOT_SUPPORTED;
if (empty($project_data['extra'])) {
$project_data['extra'] = [];
}
$project_data['extra'][] = [
'class' => ['release-not-supported'],
'label' => t('Release not supported'),
'data' => t('Your currently installed release is now unsupported, and is no longer available for download. Disabling everything included in this release or upgrading is strongly recommended!'),
];
}
}
% grep -ri revoke core/modules/update/tests
is empty. We have no test coverage of the above code block.
Proposed resolution
Add tests.
Remaining tasks
- Add tests:
Review.- RTBC.
- Commit.
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#66 | 3113292-66-8.8.patch | 30.15 KB | tedbow |
#61 | 3113292-58-8.8.x-reroll.patch | 29.46 KB | tedbow |
#58 | 3113292.55_58.interdiff.txt | 422 bytes | dww |
#58 | 3113292-58.88x.patch | 30.42 KB | dww |
#55 | 3113292.44_55.rawdiff.txt | 790 bytes | dww |
Comments
Comment #2
dwwComment #3
dwwNot just revoked. Better scope / title.
Comment #4
tedbowHere are some tests that will break with bug on #3111929-31: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported.
These test make sure "Revoked" and "Security Update Required" show even if the branch you are on is currently unsupported.
Comment #5
tedbowComment #6
dwwExcellent start! Very glad to see these test cases added...
These are driving me nuts. ;) I'm going to open a follow-up to purge this from all our fixtures.
<tag>
is totally ignored by all our tests, and all of Update module.Yeah, more of same. :/
@todo ;)
"The expected newer version"?
@todo
I don't understand the difference between these. Not clear why we need both. Let's either standardize on one, or add better comments explaining the difference.
Thanks,
-Derek
Comment #7
dwwRe: #6 1 and 2: #3113798: Remove unused (and generally wrong) <tag> markup from Update module test XML fixtures
Also, removing 'Needs tests' tag here, since #4 includes the test coverage we were missing.
Edit: I was wrong, see below.Finally, bumping to major since this is/should be blocking a critical.
Comment #8
dwwCrossing off some of the needed tests from the summary.
However, closer review shows we're still not testing this case / message:
So leaving that one not crossed off, and restoring the 'Needs tests' tag.
Thanks!
-Derek
Comment #9
dwwThis fixes all of #6 except point 6.
Using do-not-test since it's still incomplete...
Interdiff is again a bit confused, so I'm also attaching a raw diff of the 2 patch files. /shrug
Comment #10
dwwThis adds test coverage for individual releases marked unsupported. This all passes locally, so I'm going to let the bot chew on it, too.
Interdiffs (such as they are) from both #4 and #9.
Upon even closer inspection, I don't see where we're testing the case of the currently-installed release being marked as 'Insecure'. We've got lots of coverage of various cases where a newer release is a security update, which should basically always happen if an older release is marked 'Insecure'. But still, for completeness, maybe we want to add some explicit coverage of that here?
Comment #11
tedbowWe are testing this in
\Drupal\Tests\update\Functional\UpdateCoreTest::testSecurityUpdateAvailability()
and\Drupal\Tests\update\Functional\UpdateContribTest::testSecurityUpdateAvailability()
They both use
\Drupal\Tests\update\Functional\UpdateTestBase::assertSecurityUpdates()
which explicitly checksThe 2nd to last assert is based on the site being an insecure release. Just because there is newer security release it doesn't mean the installed release is in security. For instance
In this case 8.x-1.2 may still not be Insecure
testSecurityUpdateAvailability()
covers many variations of this.In
update_calculate_project_update_status()
if you comment outMultiple testcases for this method will fail.
Comment #12
tedbowre #6.6
This is following the existing pattern in a lot of the update tests where we use
assertText()
to specify text that should be on the and user should see and$this->assertRaw('check.svg', 'Check icon was found.');
for when we are checking for markup that should be on the page but is not text the user should be reading.This
assertText()
andassertRaw()
work fines for core because there is only 1 update table on the page so we don't have worry that expected text or markup being on the page but being in the other update table and therefore giving a false pass. So tests that need to apply to both core and contrib we are mostly not usingassertText()
andassertRaw()
.The test methods added here are going against contrib and core so I think it safer to test expliciting that the expected text/marked is on the element in
\Drupal\Tests\update\Functional\UpdateTestBase::$updateTableLocator
rather than just on the page.Comment #13
tedbowThe fact that 8.x-2.0 is shown as "Also available" points to another problem.
In this case there is no other update shown to the user so it should be recommended
Created #3114408: Update module labels updates as "Also available" even when there are no other possible updates
Here 2 it should not be shown as "also available" 8.x-1.1 is not shown to the user because it is "unsupported"
Here is screenshot
Comment #14
tedbowNeeds
string
parameter typeNeeds
string
parameter typeComment #15
dwwRe: #11: I'll have to dig closer when I have more time. Regardless, I don't think it's necessary to hold this up any more for that.
Re: #12: All makes sense, thanks. I just think we need better method names / comments to clarify the difference. See attached. Also more consistently formatting these two methods so neither one wraps more than 80 chars.
Re: #13: Good catch. Following. ;)
Re: #14: Good catches! Test bot also complained about an indentation bug in #10. Also fixed here.
Finally, based on Slack exchange, @tedbow and I agreed to restore/fix the
<tag>
entries in the fixtures for this patch, and leave removing them entirely to #3113798: Remove unused (and generally wrong) <tag> markup from Update module test XML fixtures. We might as well try to remain consistent, and if this lands but that doesn't, we'd be in a slightly weird state. Thankfully, that's trivial to re-roll with a perl 1-liner, so it's easy to refresh that patch after this lands.p.s. Interdiff is again being weird. *sigh*
Comment #16
tedbowI forgot to mention this in #11 when I was explaining that we do have test coverage for a site having a insecure release installed.
I add this test case which basically the test case before it but with installed release not only insecure but also on a unsupported branch. We still want to display "Security Update Required". This test case confirms it.
This cass would fail with the patch in #3111929-31: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported
Comment #17
tedbow1 final nit I think.
\Behat\Mink\WebAssert::elementTextContains()
which this method uses doesn't actually check if the text is visible.There are various reasons text could be not visible such as if it is out of the viewport, if the element is hidden via CSS, etc.
I don't think our function tests take this into account but we do in Javascript tests. But not to confuse developers into thinking we actually do/can check visibility here I think we should remove "visible".
Comment #18
dwwRe: #16 Duly noted. I'll dig later. Waist-deep in other (non-Drupal) muck right now. Happy for this to proceed as-is.
Re: #17 fair enough. I was trying to find the right adjective to concisely describe "human readable output text" and went with "visible", but I agree that potentially confuses the matter for all the reasons you laid out. +1 to #17. I believe my changes to the name and docs for
assertUpdateTableElementContains()
from #15 are sufficient to avoid confusion between the two methods now.Minor updates to remaining tasks. Since @tedbow and I have reviewed each other's stuff closely, I think it's safe to cross that out. Sadly, neither of us feel qualified to RTBC at this point, so leaving that for someone else...
Thanks!
-Derek
Comment #19
tedbow@tim.plunkett pointed this out
The last period was left off the branch. Should be
8.1.
.It still passes because of this is how determine if a version is in a branch
So version still starts with the branch string.
These need a todo to change to "Recommended version" in https://www.drupal.org/project/drupal/issues/3114408
Comment #20
xjmComment #21
xjmThis is blocked on #3111929: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported, because our changes to that issue introduced a regression to this functionallity.
Comment #22
dwwRe: #19 Interdiff all looks good. Nice fixes. RTBC+1, but I don't think I should actually change the status...
Re: #20, #21: Cool, agreed. ;)
Cheers,
-Derek
Comment #23
tim.plunkettAs pointed out in #19 I discussed this issue with @tedbow. I've reviewed all the patches and interdiffs going back to #14, and I agree with #22 that this is ready to go!
Comment #24
xjmOverall this looks great. Just a few questions, some of which I answered myself in the course of review:
Can we attach a failing patch (using the bug from the other issue) to prove the coverage here, or is the goal to add additional coverage in the other issue?
In this scenario, the unpublished release has a lower version number than the latest supported release. Should we also have a case foe when it is newer?Found it, see point 5.Wait... isn't this an identical scenario to the previous fixture? What did I miss?
Same question here (where it's the oldest release that's unpublished). I think we should have scenarios where the newer releases are unpublished as well.
Ah, here we go, this is the scenario I was asking for. An unpublished release between two published ones.
Do we have coverage for a revoked release between two supported releases?
There's no scenario here for the extended security coverage scenario where 8.0.2 is secure but 8.1.0 is not. I hope though that we added coverage for that originally when we first started supporting this.
If we don't have further test coverage for that it can be a followup as it's not related to the bug from the issue.
Comment #25
tedbowre #24
aaa_update_test.1_0-supported.xml has
<supported_branches>8.x-1.,8.x-2.</supported_branches>
aaa_update_test.1_0-unsupported.xml has
<supported_branches>8.x-2.</supported_branches>
So in 1 case the branch they are on is supported and 1 it is not. The tests are to make sure the "Revoked" and "Release not supported:" messages show up in both cases.
The extra test case added to
securityUpdateAvailabilityProvider()
was '0.0, 1.2, insecure-unsupported'. It uses the new fixturedrupal.sec.1.2_insecure-unsupported.xml
.The test case is identical to the existing case ''0.0, 1.2, insecure" except that the new fixture has different
support_branches
value. So the installed version is not in a supported branch. The new test case ensures that "Security Update Required" message still shows in that case.I would have to know the full details of the scenario you are describing to know if it is covered by
securityUpdateAvailabilityProvider()
. There are 17 test cases in that providerComment #26
dww@xjm re: #24:
Thanks for reviewing!
1: Here you go. This is patch #19 from here with the changes to update.compare.inc from #3111929-29: If no recommended update is found, Update Status recommends the latest release, even if it is unsupported, plus drupalci.yml hacks to only run update tests to save cycles.
Local results:
So yes, that really was a bug. :p If the revoked release is in an unsupported branch, we wouldn't mark it revoked with the original "fix" over there. ;)
p.s. Interdiff continues to be weird for me on this issue. I guess it's getting confused with the new files being added. /shrug
2: 👍
3:
So
aaa_update_test.1_0-unsupported.xml
is all the same release history, only with the8.x-1.
branch unsupported.This speaks to something that's been concerning me, too. I'd love it if we had clear comments at the top of each update test XML fixture explaining the scenarios that fixture provides... Trying to track all this stuff in your head is next to impossible.
4.
Probably wise. @todo
5. 👍
6. Nope. Also @todo
7. Not sure, @tedbow would know better. Sounds like this isn't a blocker for this issue, but I'll let Ted either explain the existing coverage or add the appropriate follow-up.
So, NW for 4 and 6. I'll let the bot mark it as such when this FAIL patch fails. ;)
Cheers,
-Derek
Comment #29
tedbowre #29
I agree though maybe we should open an issue to this to all the XML at once. Also it can be tricky because the same XML can be used in multiple test method and the scenarios are different based on the installed version.
Re
This refers to review in #24
I don't think we need more test coverage. The bug that was introduced in the original was pretty simple. I checked if the released was not in supported branch and continue onto the next release in the loop too earlier. I skipped this logic.
So the logic that was skipped only dealt with the installed version, no other versions. And the checks skipped dealt with 3 specific cases, unpublished, release type == unsupported and release type == insecure.
In the fix that @dww figure out we still continued on to the next release just after these checks.
I will attach a diff that goes from original bug to the fix.
If we need more tests cases I think we need explanation for why and what the exact scenario is.
Comment #30
tedbowHere is the diff between the original bug and the fix
Comment #31
tedbowInside this
if
block we have anothercontinue
.So we are still not doing any more consideration of the release.
Comment #32
xjmCan we put comments in the test to this effect, including comments that describe the contents of the fixtures? For all the scenarios added. (Not in the fixtures, in the tests.) Otherwise it's really hard to parse out what is supposed to be tested. Usually we'd put comments in a data provider to check it, but in this case that's not an option. However, "regardless of whether ____ is ____ or not" doesn't tell you which of those scenarios is the one being checked and could imply either. So the comments aren't giving the needed info there.
Regarding:
Weird, I thought you worked on that with me? #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already Under our policy, the typical pattern for core releases is that (say) 8.7.11 and 8.7.12 will be secure, 8.8.0 will be insecure, and 8.8.1 through 8.8.3 will be secure. I confirmed the original issue already introduced the correct test coverage for that, so no followup is needed.
The title of the issue implied this issue was adding test coverage for revoked releases "etc.". It didn't imply it was "adding test coverage only for cases covered by a specific regression in the other issue". I'm fine with it as-is too I guess, but I am not sure I agree with the pushback.
Leaving NW for better comments inside the tests.
Comment #33
tedbowworking on this
Comment #34
dwwRe: #29:
Completely agree. I don't think we want to do it here, just raising it as a concern.
Re: #32:
I added "(revoked, etc)" in #3 because it's not just unpublished, but also "Not supported" and "Insecure" release types. Seemed clumsy to enumerate them all in the title. But in all cases, all update.module cares about is if any of these conditions apply to your currently-installed version (which is what @tedbow is getting at).
That said, I'm not opposed to Moar Tests(tm) for the other scenarios you're asking for, I just don't think they're critical right now. The critical thing is that we don't have this coverage for an important feature / behavior, and we almost committed a change that would have broken it. The feature in question is 100% about your currently-installed version, which is why the coverage in this issue is sufficient to cover the code blocks mentioned in the summary.
The other tests would be nice for end-to-end testing to make sure the whole system is working as expected, but they're not going to exercise the code block in the summary.
Thanks,
-Derek
Comment #35
tedbowHere is some hopefully better comments. I have talked with @xjm about #32 and will comment next on why I think the existing test cases are sufficient.
Comment #36
tedbowI added this in #35
I read back through #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already
The problem before that issue was that we had
basically if we had any security updates newer than the installed version we considered the project insecure. But in that issue we solved this by relying on "Release type" of "insecure". So we have no logic around extended security coverage. If a site was on 8.1.10 and the xml didn't have
on that release you still would not have see "Security Update Required"
In
\Drupal\Tests\update\Functional\UpdateCoreTest::securityUpdateAvailabilityProvider()
we have cases for when the site is on a secure release for their minor and there is an newer security release for the next minor. In addition we have cases where there is newer security release for the existing minor. We have 17 cases in all.The only new thing we are testing here is the variation of when the release that is currently installed in not in a
supported_branch
.Of the 17 cases 6 involve a site being in a lower minor and have expected update in the next minor. I don't think we need to add a test case variation for each of these 6 cases where in additionally test the same exact situation except with the current version being in a branch that is not in
supported_minors
. I think the 1 case to confirm this enough. but we could add them or we could add them in follow up. In a follow up we could duplicate all 17 cases with a variations this would include case were the site is on the latest minor but it is an unsupported branch. This is very unlikely for Drupal core but I guess could happen in contrib.\Drupal\Tests\update\Functional\UpdateContribTest::securityUpdateAvailabilityProvider()
currently only has 7 cases partly because it doesn't yet have minor branches.Comment #37
tedbowI added this comment because we updated the XML fixture this test was using for the new test methods.
It doesn't change any assertions in the test because the 2 releases we added should never be listed as updates and this test is only confirming that there are no updates
Comment #38
dwwThanks for the updates and clarifications on test coverage!
A few nits with the new comments:
Seems we should wrap these to 80 chars, no?
Also: s/Therefore all/Therefore, all/
s/the the/the/
Almost copy/paste of above comment, with the same problems.
And here.
s/The both/They both/
s/The both/They both/
Something's not right.
8.0.2 is not "newer than '8.1.0'"...
s/The both/They both/
In this case, that'll cause "release" to exceed 80 chars, so we'll need to wrap that differently, too.
"The both"
Comment #39
dww@tim.plunkett pinged me in Slack asking me to re-roll this today since Ted's traveling. Should be easy fixes. Stay tuned.
Comment #40
dwwThis fixes everything from #38.
However, interdiff is still being annoying, and part of the problem is that in spite of the new comments, drupal.1.0.xml and drupal.1.0-unsupported.xml also differ because of legacy CVS values in the
<tag>
entries for the drupal.1.0.xml. So I'll upload another patch that cleans that up, too.Comment #41
dwwI'd rather all this went away via #3113798: Remove unused (and generally wrong) <tag> markup from Update module test XML fixtures
Until then, we should at least keep these consistent between the two "identical" fixtures, and closer to the actual updates.d.o release history feeds...
Comment #42
dwwWhoops, meant to attach these, too. Interdiffs for #41 relative to both #40 and #35...
Comment #43
xjmI think these changes are out of scope for this issue.
Comment #44
dwwRe: #43:
Okay, then here's patch #40 again, without the do-not-test suffix. ;)
Comment #46
dwwRandom fail:
Requeued...
Comment #48
tim.plunkett#3112295: Replace REQUEST_TIME in rest of OO code (except for tests) was just reverted in between those test runs. That caused the fail and isn't in the way anymore.
I agree with #43, those keep popping in and out of the patch over the course of the issue, glad they're gone again :)
This looks great, thanks to @tedbow, @dww, and @xjm for their extremely thorough reviews and explanations since #24.
Comment #49
dwwFYI: #3115435: Make clear why each XML update.module fixture is created the way it is now exists for #26.3 / #29 etc.
Thanks for opening that, @tedbow!
Comment #50
alexpottCommitted and pushed 831d6bccd9 to 9.0.x and 8015e8e3fc to 8.9.x. Thanks!
Comment #53
dwwYay, thanks!
Is this eligible for a backport to 8.8.x? It's adding test coverage of something we almost broke in another critical bug fix that I believe we're going to backport. Patch doesn't cleanly apply, but I could re-roll if appropriate. Please advise.
Thanks,
-Derek
Comment #54
alexpottBackporting tests to 8.8.x seems valuable.
Comment #55
dwwRe-rolled. Let's see what the bot says about this.
Comment #57
tedbowThis at least needs #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates because the new fixtures don't have
version_major
etc.Comment #58
dwwRe: #57: Oh, right. Bummer. I was hoping this could escape issue spaghetti, but I guess not. ;) No sense complicating the fixtures here for the backport, only to have to undo that when #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates lands. Agreed on postponed.
Meanwhile, this fixes the extra newline the bot found.
Comment #59
tedbowThis should be backported
Comment #60
tedbowThis needs #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates to be backported to 8.8.x also.
I have 8.8.x running tests now on that issue
Comment #61
tedbownew commits on 8.8.x here is reroll.
Comment #62
tedbowComment #64
dwwRandom fail. Re-queued #61. Will review shortly.
Comment #65
tedbowAssigning to myself to reroll
Comment #66
tedbowAnother reroll
Comment #67
dwwConfirmed the re-roll is clean and accurate. Only changes relative to #58 are the order of a few functions in a few files. But #66 has the order matching what we've got in the 8.9.x branch, so that's better than #58. RTBC.
Thanks!
-Derek
Comment #69
Gábor HojtsyThanks!