Problem/Motivation
Background:
Given that the PHP version is "PHP7.3"
Installed Drupal with the "standard" installation profile with number of contrib modules
Enabled "Update Manager" module
Having "All messages, with backtrace information" for "Error messages to display" in "Logging and errors"
And login as the site admin or user number 1
Scenario:
When we go to any "/admin/*" page
Then we should not have any Notices
Notice: Undefined index: status in _update_project_status_sort() (line 634 of core/modules/update/update.module).
_update_project_status_sort(Array, Array)
uasort(Array, '_update_project_status_sort') (Line: 55)
update_requirements('runtime') (Line: 186)
update_page_top(Array) (Line: 336)
Drupal\Core\Render\MainContent\HtmlRenderer->buildPageTopAndBottom(Array) (Line: 135)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 156)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 67)
Drupal\simple_oauth\HttpMiddleware\BasicAuthSwap->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 694)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
If your site is missing security updates and hits this case, the system status report has a very confusing message that visually implies an error, but masks the fact that you're missing security updates:
Proposed resolution
If we can't parse a version string, instead of leaving the project status empty and letting the rest of Update Manager blow up in unexpected ways, we should always set a status. In the case of an un-recognized version string, use UPDATE_UNKNOWN
(or UpdateFetcherInterface::UNKNOWN
for >=8.9.x).
If the version is empty, we print "Empty version".
If the version is defined, but can't be parsed, we print "Invalid version: X".
(Screenshots below).
Remaining tasks
Troubleshoot with contrib and custom modulesProvide a patch#39 and #41Bikeshed about the new message when we can't parse a version string.- Other reviews, improvements.
- RTBC.
- Commit (e.g. #58 to 9.0.x branch and cherry-pick to 8.9.x, and commit the 8.8.x-specific patch (e.g. #59 to 8.8.x
User interface changes
Adds a new message on the Available updates report for projects where we can't parse the currently installed version string:
Pre #3074993 (e.g. 8.8.2)
Before / current (as of 8.8.3)
After (with patch #41)
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#88 | 3118087-88-9.0.x.patch | 3.81 KB | tedbow |
#86 | 3118087-67.8_8_x.patch | 3.71 KB | dww |
#86 | 3118087-67.patch | 3.74 KB | dww |
Comments
Comment #2
xjmHi @RajabNatshah, can you give more information on the steps to reproduce this issue? For example:
Comment #3
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedThanks, Jess for following up
New install, with contrib modules and themes.
Drush cr did not fix the issue.
Go to any "/admin" page
looking at
The function was called in
function update_requirements($phase)
in
Not sure if this making the call in every page.
Comment #4
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #5
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #6
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedAttached the steps to reproduce
still troubleshooting the issue
Comment #7
broeker CreditAttribution: broeker at Electric Citizen commentedAlso trying to troubleshoot this on a recent upgrade from 8.8.2 on a site running PHP 7.3 -- same exact errors/notices on all admin pages, regardless of admin theme.
Not sure if this is related but we are also now seeing this listed under "ERRORS FOUND" on Status Report:
MODULE AND THEME UPDATE STATUS
Up to date
See the available updates page for more information.
But yet core+all modules are currently up-to-date (as the status indicates, not sure why it is under ERRORS FOUND.
Comment #8
xjmAdded this to the known issues for 8.8.3. Thanks!
Comment #9
xjmOne further thing that might help us here is determining which modules or themes might be triggering the notice, so that we can see whether it's an issue with the new update feed for those projects or whether it's something that's regressed in the update module itself due to recent changes.
Is it possible to insert some debugging to see which projects have the empty key and therefore are triggering the notice?
Barring that, a list of installed contrib would at least be a starting point to help find the source of the issue.
Comment #10
Nick Hope CreditAttribution: Nick Hope commentedSame issue here after updating from 8.8.2 to 8.8.3 using
composer update
thenupdate.php
(no updates were required). Issue remains after runningdrush cr
. Same error as OP and #7. Here is my list of contrib modules from my composer.json file:Comment #11
Martin Mayer CreditAttribution: Martin Mayer as a volunteer commentedUninstalling the Upgrade Status module solved this problem for me. It is the module which checks for incompatibilities to Drupal 9.
A friend says, uninstalling the Update Manager did the trick for him.
Comment #12
wroehrigUninstalling Update Manager eliminated these errors for me. They originally appeared after updating to 8.8.3 using composer. The errors begin again upon re-installing Update Manager.
Comment #13
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #14
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #15
mlozano7 CreditAttribution: mlozano7 commentedNot sure if this is the same case.
I found the same message when I upgraded to 8.8.3.
The way I fixed the errors was by adding "version" key value to the info.yml of my custom theme.
I didn't investigate this much deeper but it seems that now the version is validated in all modules and themes.
Hope this helps.
Comment #16
wroehrigI am using pdf_reader 8.x-1.x-dev module. It does not have "version" key in info.yml file. Adding "version" key and value to info.yml for that module eliminated these errors even with Update Manager enabled.
Comment #17
suit4 CreditAttribution: suit4 commentedSame issue here upgrading to 8.8.3
Adding a version-Key to my custom theme did not help, as there seem to be other modules missing a version information (which is odd).
Uninstalling Update Manager might be a mere workaround, as it removes that error message, but the functionality it provides is quite useful.
Comment #18
BrightBoldThe fix in #15 also solved the problem for me. I had a custom module with no version key and adding that resolved the error.
Comment #19
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedAfter more troubleshooting
When I go to "/admin/reports/updates"
Then I see "Compatible" 22 times for my case of listed and used extentions
But I see the following notices in the same page
"Notice: Undefined index: status in template_preprocess_update_report
Notice: Undefined index: status in template_preprocess_update_project_status
Notice: Undefined index: status in template_preprocess_update_project_status"
When I go to any other "/admin/*" pages
Then I see the "Notice: Undefined index: status in _update_project_status_sort" 22 times
Comment #20
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #21
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #22
xjmI'm not sure this is the correct solution:
update_requirements()
is required to generate that page, and that and its runtime use of_update_project_status_sort()
have not changed. Based on the error:It sounds to me like there's actually an issue of incomplete project data for one or more projects in the array, probably following #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates.
One additional thing to try:
admin/reports/updates
, click the "Check manually" link to refresh the data from the update feed.This should make sure that the site has fetched data from the updated XML feed rather than the old one. If the problem persists after this, it may mean that one or more installed modules or themes are missing data in the new feed, and we'll need to figure out which projects those are and troubleshoot it. The problem could either be in core or in the data from Drupal.org.
If anyone encountering this could insert some debug code into
update_requirements()
temporarily to try and verify which module or modules are missing the status data, that would help us debug it on the core side as well.Comment #23
xjm@RajabNatshah, from your comment in #19, it sounds like you have 22 projects installed and all of them might be showing the notice? Could you let us know what one of them is so we can try to debug this further?
Comment #24
xjmComment #25
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedJess, thanks for follow up with the issue
I followed the steps in comment #22
Resulted with the same notice
I do have number of custom contrib modules
We can take Webform for example
A contrib module which we could test any change in it
CKEditor Anchor Link - For Drupal 8
Recently worked on #3087583: Drupal 9 compatibility for [CKEditor Anchor Link] with Drupal coding standard and practice
For both branches 8.x-1.x and 8.x-2.x
Comment #26
kazajhodo CreditAttribution: kazajhodo commentedI've encountered same issue as the op.
A variety of notices like these, with different lines, but its all the same root issue.
Notice: Undefined index: status in /core/modules/update/update.module on line 640
Notice: Undefined index: status in/core/modules/update/update.install on line 117
To troubleshoot I just threw a if !$a['status'] or b status, into the method, to see only what modules were throwing the error.
To show:
Added to update.module.
Likely this will be a variety of different modules per your install.
For me it was the livechat module in all instances. For instance if you review the output of the ksm, per item, one will have a status set and one will not, hense the error message. Its the one that does not we are after.
This code is attempting to compare status values, and in my case the livechat doesn't have one, so the code is pitching a fit.
Now I think its something required in the livechat module that is missing, so I check if there is an update for this particular module. There is, apply that... errors are gone, ksm outputs nothing, check the modules settings... good to go.
This does not mean the module causing this for your install will be livechat, in my case it was only because it was extremely outdated. However this does mean it could be any outdated module.
Using the method above you'll be able to figure out which module that is, and work with focus from there.
Comment #27
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedHm, I suppose the site should not break down if 'status' is not set. This looks to me like some exist-checking should be added. This patch adds the checks to keep the site working. In my case the site completely broke on this error.
Comment #28
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedThe return value 0, is stupid off course. Sorry. Switched it with the constant UPDATE_CURRENT. But should be with UpdateManagerInterface::CURRENT maybe?
Comment #29
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #30
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedMaybe it is better to ensure 'status' is always set in stead of checking if it is set?this breaks things. Sorry.
Comment #31
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #32
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedNew try, I think this is the right position to add the 'status'. ;-)
Comment #33
xmacinfoWhen visiting "/admin/reports/updates", I got the same
Undefined index: status
errors as in https://www.drupal.org/project/drupal/issues/3118087#comment-13494928 but only on a single project:drupal/git_deploy 2.x-dev 00023b8 2.x-dev 4344b22
After running “composer update drupal/git_deploy”, the
Undefined index: status
errors are gone.Composer status show: Updating drupal/git_deploy dev-2.x (00023b8 => 4344b22): Checking out 4344b22f91
Note that between those two git commits (00023b8 => 4344b22), only two characters were added to the info file, which does not explain why updating this module removes the missing index error.
Would reinstalling a module set its status correctly?
Comment #34
KingdutchI've run into this error after updating Drupal core to 8.8.3.
Added the following snippet on line 48 of update.install (just before
unset($data['drupal']);
)This produces the following output:
Code of Open Social can be found here: https://github.com/goalgorilla/open_social (8.x-9.x is the current development branch).
I'd expect to be able to develop against a checked out version of the profile without issues and did not previously run into any warnings that we see in this issue.
Comment #35
JoshaHubbers CreditAttribution: JoshaHubbers at iO commentedComment #36
wxman CreditAttribution: wxman commentedI don't know if this helps at all, but I just installed a new 8.8.3 site on a test site. I checked the logs, and the errors didn't show up. I installed one module, and still no errors.
I had just updated two others of my sites from 8.8.2 to 8.8.3 and the logs are loaded with the errors.
The server is Linux, and running PHP 7.3. I installed everything with: composer create-project --prefer-dist drupal/recommended-project
Comment #37
JonMcL CreditAttribution: JonMcL commentedPatch at #32 is working for me!
I had only one custom module and one custom theme. Adding a version property to the info.yml files did not help. This patch did.
Comment #38
wxman CreditAttribution: wxman commentedPatch at #32 is working for me too!
I'm using a custom theme, but no custom modules. The patch worked great.
Comment #39
dwwConfirmed this was broken by #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates
#32 is nearly the right fix, thanks @JoshaHubbers! We just need to use a different constant. While we're at it, we should provide some info on the report about what happened.
Here's a test-only that will fail, demonstrating the problem (with drupalci.yml configured to only run UpdateContribTest). Without the fix, this fails like everyone's seeing:
And a full patch with the new test and the improved fix.
#NeedsBikeshed about the wording of the message on the report when we can't parse the version string.
Enjoy,
-Derek
p.s. Sorry, was working in 8.9.x branch, so the test-only that's changing drupalci.yml doesn't apply on the 8.8.x branch. Re-queued to run on 8.9.x. The full patch should work fine on all branches.
Comment #40
dwwRe: #39: https://www.drupal.org/pift-ci-job/1609529
Yay, as expected.
Perhaps the scope police won't like this, but here's a minor update to the docblock for
update_calculate_project_update_status()
to warn people about returning early and this bug. Marking it for do-not-test for now, since it's only doc changes relative to #39 and it's pointless to waste bot cycles on it if it's going to be rejected. We can stick with #39 if that's "better". /shrug.Also adding #3100110: Convert update_calculate_project_update_status() into a class as related...
Comment #41
dwwSome minor improvements to the new test:
$this->standardTests();
to make sure core's report is normal, even if aaa_update_test is busted.elementTextContains('css', $this->updateTableLocator ...)
instead ofpageTextContains()
.Comment #44
dwwBefore/after screenshots of available updates with a module with an un-parse-able version string for the summary, and updates to remaining tasks.
Comment #45
dwwTee hee. ;) #32's constant is what we need in 8.8.x branch, since the whole UpdateFetcherInterface is new for 8.9.x and beyond. So here's the 8.8.x backport. Testing this vs. 8.8.x and I'm re-queuing #41 for 8.9.x and 9.0.x.
Comment #46
dwwComment #47
dwwFor comparison, here's what happens with a 'llama' versioned module in 8.8.2 (before #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates landed).
Comment #48
dwwTo anyone who experienced this bug:
a) If it's possible for you to install a version of the site from a backup before you upgraded to 8.8.3, I'd love to see a screenshot of your available updates report.
b) If you tried adding a 'version' key and it didn't solve the problem, I bet it's because the version string you used didn't match what Update Manager now assumes is the format of a version string (e.g. either "8.x-1.2" or "3.0.1", but not things like "8.x-9.x"). If you wanted to try setting the version string to something valid and see if that solves the problems, that'd be really good information, too.
c) Of course, would love to hear that patch #45 (8.8.x) is working for you. ;)
Thanks!
-Derek
Comment #49
Annelies Van der Wee CreditAttribution: Annelies Van der Wee commentedThe steps in #22 did the trick for me! Thanks.
Comment #50
Kingdutch@dww Below is an image of the available updates report for Open Social 8.0 on Drupal 8.8.2
Comment #51
Nick Hope CreditAttribution: Nick Hope commented#45 works for me with D8.8.3. Thank you very much @dww,
I think my culprit *may* have been the Views Argument Order Sort module, which has long been showing an "Unknown" version number. I don't know why that is so, and I raised an issue about it here: https://www.drupal.org/project/views_arg_order_sort/issues/3060415 It's still showing red and "Unknown" but the accompanying message is now "Could not parse existing version: Unknown" rather than "Invalid info". Not sure if that changed after the 8.8.3 update or after this patch.
Comment #52
jungle(Screenshot in #42)
The message: Could not parse existing version: llama, personally, I'd like to adjust this message to a non-dev-friendly one. For instance:
The problem is that llama is an invalid version number, so that, drupal/ the update-manager could not parse it and continue.
To keep the message shorter, I'd keep the cause part. the result is obvious, the UNKNOWN STATUS
Comment #53
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedThe patch in #45 works for me with 8.8.3.
I can now load up pages without scrolling past a bunch of errors.
Comment #54
JonMcL CreditAttribution: JonMcL commentedPersonally, I think "llama" needs to become a valid version string.
My available updates report, before upgrading to 8.8.3, is attached. (I know, I have a lot of updates to install and test). In my case, it was probably the "Views Argument Order Sort" module which has no version code in it's dev release.
Patch at #45 worked for me.
Thanks!
Comment #55
jungleSo propose changing the message to
"llama" is an invalid version string
?Comment #56
jungleRevert the change to IS unintentionally
Comment #57
tedbowShould we have test for when version is not set all or is empty?
Do we want a different message for those cases?
The fix and comment look good!
Comment #58
dwwThanks for the review, @tedbow.
#57.1: We can't test if version is not set at all, since there's code somewhere that sets version = VERSION if it's not defined. Haven't dug deeper. But here's at least a test with '' as the version.
#57.2 👍
#55: Sure. 'llama' isn't exactly "invalid", it's just not understandable by update manager. But that's a philosophical debate at this point. ;)
I'll re-roll for 8.8.x later, gotta run now.
Comment #59
dwwNew test-only (for fun), and 8.8.x backport.
Comment #60
dwwp.s. I noticed this bug is borderline critical, since the presence of *any* module or theme with an unrecognized version string renders the status report totally confused about the state of your site. If you're missing security updates, but you've got a llama-versioned contrib somewhere, the status report thinks you're "Up to date":
:(
Comment #62
dwwNew screenshot for the summary showing the latest UI changes.
Comment #63
dwwCrossing off
Bikeshed about the new message when we can't parse a version string.from remaining tasks, since I hope we're done with that. ;)Updating the to-be-committed patch references.
Comment #64
dwwCleaning up Proposed resolution section.
Comment #65
jungleThanks @dww! RTBC +1.
My concern in #52 and #54 about the error message was addressed, and better than what I proposed.
Invalid version: X
-- clean and simple.A fun fact I never know!
Comment #66
tedbow@dww thanks for the testing of the empty string
Re the version being set I think there are 2 places this happens.
By setting this any module that doesn't have a version set will will get this value set
update_test_system_info_alter()
. Since we aren't testing core this doesn't need to be set in this test.The current test passes without this.
aaa_update_test.info.yml
hasversion: VERSION
so it will be set in
\Drupal\Core\Extension\InfoParserDynamic::parse()
We can remove
version
from being set at all inaaa_update_test.info.yml
This was set way back in #2095943: Core modules and themes missing the version attribute in info.yml but this module is a special case because we are always calling
\Drupal\Tests\update\Functional\UpdateTestBase::refreshUpdateStatus()
when using for testing so it will always have a dynamic value set.I think with those 2 changes we can have a test case where no version is set. @BrightBold specifically mentions having this test case in #18 and this patch fixing it. @mlozano7, @wroehrig, @suit4 also mentioned the same problem in #15. so I think we should cover it.
Crediting a bunch of people gave specific cases and suggestions(including myself)
Comment #67
dww@tedbow re: #66
1. Yeah, I added the
#all
after I gave up on a NULL version test, since I wanted core to be valid for this test. But probably it doesn't matter.2. Right, that makes sense. I didn't even look in the raw .info files. ;) Thanks!
However, thanks to
update_process_project_info()
we'll never see a NULL version:So we always end up with 'Unknown' as the version, no matter what. I guess we should test for that. /shrug
Comment #68
tedbow@dww thanks for adding the test case! Looks good
Comment #70
Gábor HojtsyComment #71
xjmThis affects 8.8.x and is a bugfix for a regression we introduced in an 8.8 patch release, so it absolutely needs to be fixed in 8.8. Thanks!
Comment #72
xjmI always get nervous when we change existing valid test fixtures rather than adding new ones. Is there a reason we're not adding a new fixture here?
Comment #73
gunwald CreditAttribution: gunwald commentedI can confirm that the patch in #67 works for Drupal 8.8.
Comment #74
xjmTagging for visibility. Hopefully we can have this issue solved before 8.9.0-beta1, but it's also allowable after since it's fixing an introduced regression.
Comment #75
xjmCrosspost from hell; I didn't intend to delete any files, just to tag the issue. That said, NW is reasonable to address #72, whereupon we will hopefully have new patches.
Comment #76
jungleI am going to add a new fixture for testing. add revert the change pointed in #72
Comment #77
dwwRe: #72
Reasons:
A)
version = VERSION
is not valid for contrib test fixtures.B) All the tests that use this fixture set a valid version themselves.
C) We can't have a NULL version with the fixture like this.
D) @tedbow told me to do it. ;)
E) All the tests still pass with the change.
F) It's the smallest patch that demonstrates and fixes the bug.
I believe #67 is still RTBC. There'd be no question of getting it into the next releases if we committed those patches instead of setting to NW for this. /shrug
Comment #78
jungleTried with a new one, tests broken on my local. can not get it right in a short time. and @dww already explained. So I am unassigning myself.
Comment #79
jungleWell, made one with a new fixture. Keep what #72 concerned untouched. But personally, I prefer the patch in #67
Comment #80
jungleComment #81
dww@jungle: Thanks for helping to move this forward. However, we don't really want to do that, since we'd be adding ddd_update_test to everything, potentially breaking other tests and other assumptions. If we have to do anything beyond #67, we should be more careful. I already started this, so I'll finish up what I've got and post it. Stay tuned.
Comment #82
dwwLike so.
I, too, prefer #67.
Comment #84
dwwTentatively giving this a more descriptive title. I hope folks searching for "Undefined index: status in _update_project_status_sort()" will still find this from the summary and other comments.
Also, adding screenshot of the status report page from #60 to the problem/motivation section.
Comment #85
tedbowre #72 I agree with @dww that it is ok to remove
version
In all of the tests in
UpdateContribTest
we rely on the verison for the module being changed. We don't rely on the version being set by\Drupal\Core\Extension\InfoParserDynamic::parse()
as I detailed in #66.2 This version value is always changing because it is set as$parsed_info['version'] = \Drupal::VERSION;
so we could rely on it for test where the only point is we want to mock a specific version. That is why we alway call\Drupal\Tests\update\Functional\UpdateTestBase::refreshUpdateStatus()
to set the version.If any of these test methods relied on the version not being mocked the version being used would change with every minor, from 8.8.0-dev to 8.9.0-dev etc, and known of test would work.
aaa_update_test only used in few other test classes
\Drupal\Tests\update\Functional\UpdateCoreTest::testFetchTasks()
which does deal with version at all.\Drupal\KernelTests\Core\Common\LegacyFunctionsTest::testArchiverGetArchiver()
which also does not deal with versions.\Drupal\Tests\update\Functional\UpdateUploadTest::testUploadModule()
which only checks an existing module can't be installed via update tar file functionality. Versions aren't considered\Drupal\KernelTests\Core\Common\LegacyFunctionsTest::testArchiverGetArchiver()
no versions considered.I think we should stay with #67
Comment #86
dwwSince the #67 patches were deleted in #74, re-uploading them and setting back to RTBC based on #85.
Thanks,
-Derek
Comment #87
Nick Hope CreditAttribution: Nick Hope commented#86 working fine for me in D8.8.3. Thank you. My error message for the Views Argument Order Sort module changed from "Could not parse existing version: Unknown" to "Invalid version: Unknown".
Comment #88
tedbowHere is 9.0.x version
Comment #89
tedbowCreated follow up #3120961: Remove VERSION from update test modules that receive dynamic version numbers in tests
Comment #90
xjmThanks @dww, @jungle, and @tedbow for documenting all that!
It's a good point that
version: VERSION
isn't valid for contrib/custom modules; at most, there will be a random coincidence IRL thatVERSION
is the same as contrib's, and having it in the fixture is the kind of thing that will introduce random failures or false passes. For that reason, I asked @tedbow a followup to address this for all three fixtures. (Since addressing all three fixtures is out of scope here.)In general, I do want to reiterate that reusing test fixtures and modifying them for a different issue than the one that added them is risky/fragile, and not a best practice. Since it is a red flag it also can increase the time-to-commit, because a reviewer has to investigate all the other ways the fixture is used and validate that they are not adversely affected by the change.
Responding briefly to:
As a release manager, I have as part of my responsibilities evaluating risks and tradeoffs for an issue being committed with an outstanding concern versus getting it in later, and I have final discretion over both what issues are allowable in what release phase and what each issue's priority is, so rest assured this is something I take into account when providing a review on a critical during the week of a beta window.
Also, as a core committer, I have it within my purview to set an issue to NR or NW for anything I'm concerned about. It's always helpful to get responses on my feedback as well as other perspectives, but less helpful to to push back telling me I should just commit something despite my own concerns or unanswered questions.
Also to:
This is more of an implementation issue and actually in itself may indicate we're coupling the test coverage too much. Generally, we should be able to add a decoupled test with its own fixture when needed.
All that said, I'm comfortable with the explanations provided in #77 and #85 so long as we have that followup, but for future reference, a new fixture usually really is best. The smallest patch does not always mean the simplest, safest, or surest.
Thanks everyone for taking the time to discuss and document this. Going to hit save before I crosspost and accidentally eat the patches again, then will update issue credits.
Comment #94
xjmCommitted to 9.0.x, 8.9.x, and 8.8.x. Thanks to everyone who helped fix this regression, as well as to everyone who took the time to report it and verify the fix resolved their site's issue!