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:

Confused status report from a module with an invalid version string on a site 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 modules
  • Provide a patch #39 and #41
  • Bikeshed 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)

Available updates report for a module with an unknown version string (before issue #3074993 broke this)

Before / current (as of 8.8.3)

Available updates report for a module with an unknown version string (before fix)

After (with patch #41)

 llama' message) and an empty version string

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#88 3118087-88-9.0.x.patch3.81 KBtedbow
#86 3118087-67.8_8_x.patch3.71 KBdww
#86 3118087-67.patch3.74 KBdww
#82 3118087.67_82.interdiff.txt3.55 KBdww
#82 3118087-82.8_8_x.patch5.41 KBdww
#82 3118087-82.8_9_x.patch5.44 KBdww
#82 3118087-82.test-only.patch5.91 KBdww
#79 interdiff-67-79.txt6.33 KBjungle
#79 3118087-79.patch7.62 KBjungle
#62 3118087-62.after_.png43.92 KBdww
#60 3118087-60.status-report-confused.png40.11 KBdww
#59 3118087.58_59.interdiff.txt1010 bytesdww
#59 3118087-59.8_8_x.patch3.12 KBdww
#59 3118087-59.test-only.patch3.63 KBdww
#58 3118087.41_58.interdiff.txt2.99 KBdww
#58 3118087-58.patch3.16 KBdww
#54 Screenshot_2020-03-12 Available updates-before8.8.3.jpg678.86 KBJonMcL
#50 Schermafbeelding 2020-03-12 om 11.28.56-fullpage.png451.11 KBKingdutch
#47 3118087-47.token-llama-before-3074993.png13.88 KBdww
#45 3118087.41_45.interdiff.txt607 bytesdww
#45 3118087-45.full-8_8_x.patch2.3 KBdww
#44 3118087-42.seven-after.png24.44 KBdww
#44 3118087-42.seven-before.png15.81 KBdww
#41 3118087.40_41.interdiff.txt679 bytesdww
#41 3118087-41.full_.patch2.31 KBdww
#41 3118087-41.test-only.patch3.24 KBdww
#40 3118087.39_40.interdiff.txt616 bytesdww
#40 3118087-40.patch2.29 KBdww
#39 3118087.32_39.interdiff.txt1.61 KBdww
#39 3118087-39.full_.patch1.81 KBdww
#39 3118087-39.test-only-will-fail.patch3.22 KBdww
#32 update-undefined_index_status-3118087-32.patch540 bytesJoshaHubbers
#30 update-undefined_index_status-3118087-29.patch762 bytesJoshaHubbers
#28 update-undefined_index_status-3118087-28.patch3.31 KBJoshaHubbers
#27 update-undefined_index_status-3118087-27.patch3.3 KBJoshaHubbers
#25 CKEditor Anchor Link - For Drupal 8 - compataible - Available updates dev varbase8c.png25.1 KBRajab Natshah
#25 Webform - compatible - Available updates dev varbase8c.png26.27 KBRajab Natshah
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RajabNatshah created an issue. See original summary.

xjm’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Active » Postponed (maintainer needs more info)

Hi @RajabNatshah, can you give more information on the steps to reproduce this issue? For example:

  • Does it happen when you attempt to ugprade to 8.8.3?
  • Does a manual cache clear resolve the issue?
  • Which steps did you take prior to seeing the error?
Rajab Natshah’s picture

Thanks, Jess for following up

Does it happen when you attempt to ugprade to 8.8.3?

New install, with contrib modules and themes.

Does a manual cache clear resolve the issue?

Drush cr did not fix the issue.

Which steps did you take prior to seeing the error?

Go to any "/admin" page

looking at

/**
 * Orders projects based on their status.
 *
 * Callback for uasort() within update_requirements().
 */
function _update_project_status_sort($a, $b) {
  // The status constants are numerically in the right order, so we can
  // usually subtract the two to compare in the order we want. However,
  // negative status values should be treated as if they are huge, since we
  // always want them at the bottom of the list.
  $a_status = $a['status'] > 0 ? $a['status'] : (-10 * $a['status']);
  $b_status = $b['status'] > 0 ? $b['status'] : (-10 * $b['status']);
  return $a_status - $b_status;
}

The function was called in
function update_requirements($phase)
in

if ($phase == 'runtime')

Not sure if this making the call in every page.

        // Now, sort our $data array based on each project's status. The
        // status constants are numbered in the right order of precedence, so
        // we just need to make sure the projects are sorted in ascending
        // order of status, and we can look at the first project we find.
        uasort($data, '_update_project_status_sort');
Rajab Natshah’s picture

Issue summary: View changes
Rajab Natshah’s picture

Category: Task » Bug report
Status: Postponed (maintainer needs more info) » Active
Rajab Natshah’s picture

Attached the steps to reproduce
still troubleshooting the issue

broeker’s picture

Also 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.

xjm’s picture

Added this to the known issues for 8.8.3. Thanks!

xjm’s picture

One 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.

Nick Hope’s picture

Same issue here after updating from 8.8.2 to 8.8.3 using composer update then update.php (no updates were required). Issue remains after running drush cr. Same error as OP and #7. Here is my list of contrib modules from my composer.json file:

    "require": {
        "composer/installers": "^1.7",
        "cweagans/composer-patches": "^1.6.7",
        "drupal-composer/drupal-scaffold": "^2.5",
        "drupal/admin_toolbar": "^2.0",
        "drupal/allowed_formats": "^1.2",
        "drupal/alpha_pagination": "2.x-dev",
        "drupal/backup_migrate": "^4.1",
        "drupal/better_exposed_filters": "^4.0@alpha",
        "drupal/block_titlelink": "1.x-dev",
        "drupal/chosen": "^2.9",
        "drupal/ckeditorheight": "^1.8",
        "drupal/colorbox": "^1.4",
        "drupal/config_delete": "^1.16",
        "drupal/console": "^1.9.3",
        "drupal/core": "^8.8.3",
        "drupal/ctools": "^3.2",
        "drupal/editor_advanced_link": "^1.6",
        "drupal/extlink": "^1.2",
        "drupal/facets": "^1.4",
        "drupal/field_permissions": "^1.0@rc",
        "drupal/geocoder": "^2.9",
        "drupal/geofield": "^1.8",
        "drupal/geofield_map": "^2.57",
        "drupal/geolocation": "^3.0@rc",
        "drupal/hierarchical_term_formatter": "^1.1",
        "drupal/hierarchical_taxonomy_menu": "^1.38",
        "drupal/image_style_quality": "^1.3",
        "drupal/leaflet": "1.25",
        "drupal/linkit": "^5.0@beta",
        "drupal/media_entity_remote_image": "^1.0@alpha",
        "drupal/media_taxonomy_filter": "2.x-dev",
        "drupal/migrate_plus": "^4.2",
        "drupal/migrate_source_csv": "^3.2",
        "drupal/migrate_tools": "^4.5",
        "drupal/module_filter": "^3.1",
        "drupal/pathauto": "^1.6",
        "drupal/quicktabs": "^3.0@alpha",
        "drupal/search_api": "^1.15",
        "drupal/search_api_autocomplete": "^1.2",
        "drupal/shs": "^1.0@alpha",
        "drupal/superfish": "^1.3",
        "drupal/taxonomy_manager": "^1.0@alpha",
        "drupal/taxonomy_menu": "^3.4",
        "drupal/token": "^1.6",
        "drupal/tvi": "^1.0@beta",
        "drupal/video_embed_field": "^2.2",
        "drupal/views_arg_order_sort": "1.x-dev",
        "drupal/views_bulk_edit": "^2.4",
        "drupal/views_bulk_operations": "^3.6",
        "drupal/views_contextual_filters_or": "^1.1",
        "drupal/views_infinite_scroll": "^1.6",
        "bower-asset/jquery-ui-slider-pips": "^1.11.4",
        "jackmoore/colorbox": "^1.6.4",
        "vlucas/phpdotenv": "^2.5.1",
        "webflo/drupal-finder": "^1.1",
        "webmozart/path-util": "^2.3",
        "zaporylie/composer-drupal-optimizations": "^1.1",
        "harvesthq/chosen": "^1.8.7"
    },
    "require-dev": {
        "drupal/devel": "^1.2",
        "drupal/core-dev": "^8.8"
    }
Martin Mayer’s picture

Uninstalling 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.

wroehrig’s picture

Uninstalling 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.

Rajab Natshah’s picture

Issue summary: View changes
Rajab Natshah’s picture

Issue summary: View changes
mlozano7’s picture

Not 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.

wroehrig’s picture

I 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.

suit4’s picture

Same 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.

BrightBold’s picture

The fix in #15 also solved the problem for me. I had a custom module with no version key and adding that resolved the error.

Rajab Natshah’s picture

After 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

Rajab Natshah’s picture

Issue summary: View changes
Rajab Natshah’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

I'm not sure this is the correct solution:

Move the call for the function to the route for "admin/reports/updates"

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:

Undefined index: status in _update_project_status_sort() (line 634 of core/modules/update/update.module)

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:

  1. Enable the update module if it is not already.
  2. Clear the site cache.
  3. On admin/reports/updates, click the "Check manually" link to refresh the data from the update feed.
  4. See if the notice is still displayed once the update data has been refreshed.
  5. Repeat steps 2-4 once more.

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.

xjm’s picture

@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?

xjm’s picture

Issue summary: View changes
Rajab Natshah’s picture

Jess, 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

Webform - compatible - Available updates

A contrib module which we could test any change in it

CKEditor Anchor Link - For Drupal 8

CKEditor Anchor Link - For Drupal 8 - compatible - Available updates

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

kazajhodo’s picture

I'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.

function _update_project_status_sort($a, $b) {
  // The status constants are numerically in the right order, so we can
  // usually subtract the two to compare in the order we want. However,
  // negative status values should be treated as if they are huge, since we
  // always want them at the bottom of the list.

  if (!$a['status'] || !$b['status']) {
    ksm($a, $b);
  }

  $a_status = $a['status'] > 0 ? $a['status'] : (-10 * $a['status']);
  $b_status = $b['status'] > 0 ? $b['status'] : (-10 * $b['status']);
  return $a_status - $b_status;
}

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.

JoshaHubbers’s picture

Hm, 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.

JoshaHubbers’s picture

The return value 0, is stupid off course. Sorry. Switched it with the constant UPDATE_CURRENT. But should be with UpdateManagerInterface::CURRENT maybe?

JoshaHubbers’s picture

JoshaHubbers’s picture

Maybe it is better to ensure 'status' is always set in stead of checking if it is set?
this breaks things. Sorry.

JoshaHubbers’s picture

JoshaHubbers’s picture

New try, I think this is the right position to add the 'status'. ;-)

xmacinfo’s picture

When 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?

Kingdutch’s picture

I'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']);)

      foreach ($data as $m) {
        if (!isset($m['status'])) {
          var_dump($m);
        }
      }

This produces the following output:

/var/www/html/core/modules/update/update.install:50:
array (size=11)
  'name' => string 'social' (length=6)
  'info' => 
    array (size=6)
      'name' => string 'Social' (length=6)
      'version' => string '8.x-9.x' (length=7)
      'project' => string 'social' (length=6)
      'package' => string 'Other' (length=5)
      '_info_file_ctime' => int 1581597314
      'datestamp' => int 0
  'datestamp' => int 0
  'includes' => 
    array (size=1)
      'social' => string 'Social' (length=6)
  'project_type' => string 'module' (length=6)
  'project_status' => boolean true
  'existing_version' => string '8.x-9.x' (length=7)
  'existing_major' => string '9' (length=1)
  'install_type' => string 'official' (length=8)
  'title' => string 'Open Social' (length=11)
  'link' => string 'https://www.drupal.org/project/social' (length=37)

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.

JoshaHubbers’s picture

wxman’s picture

I 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

JonMcL’s picture

Patch 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.

wxman’s picture

Patch at #32 is working for me too!

I'm using a custom theme, but no custom modules. The patch worked great.

dww’s picture

Confirmed 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:

There was 1 error:

1) Drupal\Tests\update\Functional\UpdateContribTest::testNonStandardVersionStrings
Exception: Notice: Undefined index: status
template_preprocess_update_report()() (Line: 64)

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.

dww’s picture

Related issues: +#3100110: Convert update_calculate_project_update_status() into a class
FileSize
2.29 KB
616 bytes

Re: #39: https://www.drupal.org/pift-ci-job/1609529

There was 1 error:

1) Drupal\Tests\update\Functional\UpdateContribTest::testNonStandardVersionStrings
Exception: Notice: Undefined index: status
template_preprocess_update_report()() (Line: 64)

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...

dww’s picture

Some minor improvements to the new test:

  1. Might as well invoke $this->standardTests(); to make sure core's report is normal, even if aaa_update_test is busted.
  2. We can use elementTextContains('css', $this->updateTableLocator ...) instead of pageTextContains().

The last submitted patch, 39: 3118087-39.full_.patch, failed testing. View results

The last submitted patch, 41: 3118087-41.test-only.patch, failed testing. View results

dww’s picture

Issue summary: View changes
FileSize
15.81 KB
24.44 KB

Before/after screenshots of available updates with a module with an un-parse-able version string for the summary, and updates to remaining tasks.

dww’s picture

Tee 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.

dww’s picture

Issue summary: View changes
dww’s picture

For 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).

dww’s picture

To 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

Annelies Van der Wee’s picture

The steps in #22 did the trick for me! Thanks.

Kingdutch’s picture

@dww Below is an image of the available updates report for Open Social 8.0 on Drupal 8.8.2

Available updates for Open Social 8.0 on Drupal 8.8.2

Nick Hope’s picture

#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.

jungle’s picture

(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:

<strong>llama</strong> is an invalid version number

The problem is that llama is an invalid version number, so that, drupal/ the update-manager could not parse it and continue.

  • The cause: llama is an invalid version number
  • The result: Could not parse existing version: llama

To keep the message shorter, I'd keep the cause part. the result is obvious, the UNKNOWN STATUS

adamzimmermann’s picture

The patch in #45 works for me with 8.8.3.

I can now load up pages without scrolling past a bunch of errors.

JonMcL’s picture

Personally, 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!

Screenshot_2020-03-12 Available updates-before8.8.3

jungle’s picture

Issue summary: View changes

So propose changing the message to "llama" is an invalid version string?

jungle’s picture

Issue summary: View changes

Revert the change to IS unintentionally

tedbow’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -795,6 +795,26 @@ public function testUnsupportedRelease() {
    +    $system_info = [
    +      'aaa_update_test' => [
    +        'project' => 'aaa_update_test',
    +        'version' => 'llama',
    +        'hidden' => FALSE,
    +      ],
    +    ];
    

    Should we have test for when version is not set all or is empty?

    Do we want a different message for those cases?

  2. +++ b/core/modules/update/update.compare.inc
    @@ -196,6 +196,9 @@ function update_calculate_project_data($available) {
    + * NOTE: This function *must* set a value for $project_data['status'] before
    + * returning, or the rest of the Update Manager will break in unexpected ways.
    + *
    
    @@ -268,6 +271,8 @@ function update_calculate_project_update_status(&$project_data, $available) {
    +    $project_data['status'] = UpdateFetcherInterface::UNKNOWN;
    +    $project_data['reason'] = t('Could not parse existing version: @existing_version', ['@existing_version' => $project_data['existing_version']]);
    

    The fix and comment look good!

dww’s picture

Thanks 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.

dww’s picture

New test-only (for fun), and 8.8.x backport.

dww’s picture

Issue summary: View changes
FileSize
40.11 KB

p.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":

Confused status report from a module with an invalid version string

:(

The last submitted patch, 59: 3118087-59.test-only.patch, failed testing. View results

dww’s picture

Issue summary: View changes
FileSize
43.92 KB

New screenshot for the summary showing the latest UI changes.

dww’s picture

Issue summary: View changes

Crossing 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.

dww’s picture

Issue summary: View changes

Cleaning up Proposed resolution section.

jungle’s picture

Thanks @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.

+++ b/core/modules/update/update.compare.inc
@@ -261,11 +264,19 @@ function update_calculate_project_update_status(&$project_data, $available) {
   // Figure out the target major version.
+  // Off Drupal.org, '0' could be a valid version string, so don't use empty().
+  if (!isset($project_data['existing_version']) || $project_data['existing_version'] === '') {

A fun fact I never know!

tedbow’s picture

Status: Needs review » Needs work

@dww thanks for the testing of the empty string

Re the version being set I think there are 2 places this happens.

  1. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
    @@ -795,6 +795,41 @@ public function testUnsupportedRelease() {
    +        '#all' => [
    +          'version' => '8.0.0',
    +        ],
    

    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.

  2. The other place it would be set is because aaa_update_test.info.yml has
    version: VERSION
    so it will be set in \Drupal\Core\Extension\InfoParserDynamic::parse()
    if (isset($parsed_info['version']) && $parsed_info['version'] === 'VERSION') {
       $parsed_info['version'] = \Drupal::VERSION;
    }
    

    We can remove version from being set at all in aaa_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)

dww’s picture

@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:

    if (isset($info['version'])) {
      ...
    }
    else {
      // No version info available at all.
      $install_type = 'unknown';
      $info['version'] = t('Unknown');
      $info['major'] = -1;
    }

So we always end up with 'Unknown' as the version, no matter what. I guess we should test for that. /shrug

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@dww thanks for adding the test case! Looks good

The last submitted patch, 67: 3118087-67.test-only.patch, failed testing. View results

Gábor Hojtsy’s picture

Version: 8.8.x-dev » 8.9.x-dev
xjm’s picture

Version: 8.9.x-dev » 8.8.x-dev

This 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!

xjm’s picture

+++ b/core/modules/update/tests/modules/aaa_update_test/aaa_update_test.info.yml
@@ -2,5 +2,4 @@ name: 'AAA Update test'
 type: module
 description: 'Support module for update module testing.'
 package: Testing
-version: VERSION
 core: 8.x

I 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?

gunwald’s picture

I can confirm that the patch in #67 works for Drupal 8.8.

xjm’s picture

Tagging 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.

xjm’s picture

Crosspost 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.

jungle’s picture

Assigned: Unassigned » jungle

I am going to add a new fixture for testing. add revert the change pointed in #72

dww’s picture

Re: #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

jungle’s picture

Assigned: jungle » Unassigned

Tried 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.

jungle’s picture

Well, made one with a new fixture. Keep what #72 concerned untouched. But personally, I prefer the patch in #67

jungle’s picture

Status: Needs work » Needs review
dww’s picture

Assigned: Unassigned » dww
Status: Needs review » Needs work

@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.

dww’s picture

The last submitted patch, 82: 3118087-82.test-only.patch, failed testing. View results

dww’s picture

Title: Notice: Undefined index: status in _update_project_status_sort() » If any extension has a missing or invalid version, Update manager throws errors and is confused about site update status
Issue summary: View changes

Tentatively 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.

tedbow’s picture

re #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

  1. \Drupal\Tests\update\Functional\UpdateCoreTest::testFetchTasks() which does deal with version at all.
  2. \Drupal\KernelTests\Core\Common\LegacyFunctionsTest::testArchiverGetArchiver() which also does not deal with versions.
  3. \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
  4. \Drupal\KernelTests\Core\Common\LegacyFunctionsTest::testArchiverGetArchiver() no versions considered.

I think we should stay with #67

dww’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.74 KB
3.71 KB

Since the #67 patches were deleted in #74, re-uploading them and setting back to RTBC based on #85.

Thanks,
-Derek

Nick Hope’s picture

#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".

tedbow’s picture

Here is 9.0.x version

tedbow’s picture

xjm’s picture

Thanks @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 that VERSION 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:

There'd be no question of getting it into the next releases if we committed those patches instead of setting to NW for this.

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:

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.

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.

  • xjm committed a816f7e on 9.0.x
    Issue #3118087 by dww, JoshaHubbers, jungle, tedbow, RajabNatshah,...

  • xjm committed f448335 on 8.9.x
    Issue #3118087 by dww, JoshaHubbers, jungle, tedbow, RajabNatshah,...

  • xjm committed 643bea8 on 8.8.x
    Issue #3118087 by dww, JoshaHubbers, jungle, tedbow, RajabNatshah,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.