PHP Warning in template_preprocess_update_report(): Invalid argument supplied for foreach()

What did you do, if anything, to see this message?

This is perhaps a duplicate of #2715499: update module fails due to queue.expire & key_value records

Shouldn't be considered a duplicate!
The programming error is quite clear: Don't treat something as an array that isn't an array. The fix is simple: make sure it is an array.
Line 21 of template_preprocess_update_report in core/modules/update/update.report.inc
$data = is_array($variables['data'])?$variables['data']:[];

Alternatively it could be casted to an array. Now it is considered as no updates. Upstream errors still need to be addressed.

Attached a patch to remove the warning.
I reproduced this error after importing production database dump to local environment and updated core to 8.8.2. Could not reproduce when updated from local installation. Any pointers how to debug the root of this issue?

The missing patch.

I got this error in Drupal 8.8.5. Should that be fixed in 8.8.6, 8.9 and 9?

Updated the fix and added testing. The original fix from comment #5 fails the test.

Disagree with the change, should be combination of isset and is_array. It doesn't deal with the issue. Foreach is having a problem with something other than an array.
Tests should also include ['data'] to contain integer or string.

Updated the patch with the suggestion of @mmjvb.

1. +++ b/core/modules/update/tests/src/Kernel/UpdateReportTest.php
   @@ -0,0 +1,69 @@
   +   * Tests that template_preprocess_update_report() with wrong data.
   

   This summary doesn't parse.

2. +++ b/core/modules/update/tests/src/Kernel/UpdateReportTest.php
   @@ -0,0 +1,69 @@
   +    template_preprocess_update_report($variables);
   ...
   +    $expected += $variables;
   +    $this->assertEquals($expected, $variables);
   

   This seems a bit wonky for a test. We process the variables, then have an array of expected outcome, then we merge what we were passed with what we expect, then we compare the two arrays. Strange. I'm not sure this is what we want to be testing.

3. +++ b/core/modules/update/update.report.inc
   @@ -20,7 +20,7 @@
   +  $data = isset($variables['data']) && is_array($variables['data']) ? $variables['data'] : [];
   

   This is addressing the symptom of the php warning without understanding or addressing the root cause. I'm not thrilled about this approach. I'd rather we understood why we're invoking this with empty data in the first place.

Thanks,
-Derek

Working on it

@dww i have added #1 is working fine and tested .
#2No need of this $expected += $variables;
#3 changed

#11 valid again!
Not dealing with the issue. Still goes wrong when ['data'] contains a string.

@mmjvb added a check for ['data'] should be array.

fixed some minor issues on core/modules/update/update.report.inc and added array before $variables.
line 23 Space found before semicolon; expected "];" but found "] ;"
39 Expected newline after closing brace
40 Line indented incorrectly; expected 4 spaces, found 5
62 Case breaking statement indented incorrectly; expected 10 spaces, found 11
77 Closing brace indented incorrectly; expected 5 spaces, found 4

@shaktik found two test cases failed
These test case issues has been fixed in this patch.
Thanks @shaktik

Afraid this is going to be considered scope creep. The original bug report is quite clear: a string is treated as an array. Solution simple as well: only process as an array when you know it is an array.

Consider most of the changes an improvement but out of scope for this issue. Consider proper dealing with empty in scope.

Hi @mmjvb thanks
Now we are only checking the array and avoid string as ['data]
i hope this will meet #27

@pavnish and @shaktik: Thanks for trying to help move this forward.

A) #14.1 "doesn't parse" was about the grammar, not the PHP syntax. ;) It's a comment, of course PHP sees no syntax error. I meant this isn't a proper English sentence.

B) #14.3 is nowhere addressed in any of the patches / comments since #14.

C) #23 is indeed scope creep. We need a separate follow-up issue to fix existing code standards violations in this file. Of course, we need to fix the coding standards of anything we're actively changing as part of this patch. But please don't go off to "fix" everything you see in the same file.

D) #29 passes because you've completely left out the new test class from the patch. :(

So, back to needs work. However, I suggest you stop uploading patches and start debugging the root cause of this. Only *after* that's done would any new patches be helpful. If you're not up for it, no problem. But slinging yet more patches at this without getting to the bottom of this bug is going to be a waste of effort on your part, wasted bot cycles for the infrastructure, and wasted time for folks reviewing + following the issue.

We need to understand why 'data' isn't an array before we try to use it. Clearly, this was written to expect an array. Most of the time, it is. The problem isn't that this template preprocess function assumes an array. The problem is that the calling code doesn't handle some yet-to-be-known error condition that leaves empty something we expect to be an array. That is what we need to find, solve, and have test coverage of.

Thanks,
-Derek

Hi @dww thanks for review this.
As per the @mmjvb comment on #27 we only handle the array ['data'], it should be an array so i remove the test case on #29
I have also check this on #25 with test case
@dww please suggest if we need test case then we can change the #14.1

Regards
Pavnish

@pavnish: TL;DR:

I suggest you stop uploading patches and start debugging the root cause of this. Only *after* that's done would any new patches be helpful.

The problem isn't that this template preprocess function assumes an array. The problem is that the calling code doesn't handle some yet-to-be-known error condition that leaves empty something we expect to be an array. That is what we need to find, solve, and have test coverage of.

Seriously, with what kind of authority do you say #32.2 ?

Absolutely, disagree with that assessment. Addressed that twice calling that implicitly scope creep. It is clear we disagree on that.

For the record, just sharing my expertise here, no authority whatsoever in this community. Just trying to help fix and prevent mistakes.

With the authority of:
A) The person who wrote the update manager.
B) The person who's been off/on the maintainer of it in core for the last 12+ years.
C) As someone who contributes to a ton of core issues, sees/remembers the concerns that core committers raise when reviewing patches.

Other similar issues "php warning: invalid whatever" in core often get a very similar response from the core committers: "Sure, that patch would hide the warning, but that might just be masking a deeper bug".

/shrug

Indeed it MIGHT, that is why it needs to be investigated. Again, scope creep. Possible bugs need to be investigated. Actual bugs need to be fixed.
My understanding is that RTBC is for getting the opinion of core committers when there are different opinions on what needs done. Obviously, one of us is going to disagree. But it is their call.

Thanks for elaborating, what is still unclear is B. Are you on or off at this moment? Wasn't asking for justification of your opinion, was asking for the weight of your actions. Surely, you understand people want to know whether you are the one that is going to block this as core committer or whether you collaborate to get mistakes fixed.

@pavnish #25 Suggest to create a separate issue for those quality improvements. Haven't looked at all the changes, but that is for the new issue. You might want to postpone it on this issue and reference it here as related. That is what i meant with scope creep.

@shaktik #22,23 Same thing for you. Suggest to propose the refactoring considered out of scope with a new issue.

@all experiencing this bug
Could you provide a backtrace and the contents of ['data'] to determine whether the information passed is in error. This way it can be determined where to file bug reports if any.
The bug in update.module is evident for most of us, there might also be bugs in other parts of core or extensions. Would be nice to identify them and try to fix those as well.

@dww: First let me apologize for my fellow countryman. We the Dutch are not very good in dealing with authority (that is including myself).

For #14.1: Fixed.

For #14.2: Fixed.

For #14.3: I would very much like to fix the underlying source of this bug. The problem I see is that all the information that we can use should be in $variables['data'] and that is empty. The only thing left is to return message/notice with the debug_backtrace(), only I am not sure if this is what you would like.

@daffie Such statement say more about you than about me.

My problem is not with authority, it is with unprofessionalism and poor communication.

You can lead a horse to water, you can't make him drink.

I strive to be a clear communicator, supportive, professional, inclusive and compassionate. I often come up short, and I'm always open for constructive feedback. I just re-read all my comments in this issue (#14, #30, #32, #34). I thought #14 and #30 were professional and mostly clear. I think #32 was a bit rude, apologies to @pavnish for letting some impatience influence my tone. Although #33 was fairly abrasive, I thought #34 was basically calm, not rude or trying to escalate anything. I ended with /shrug -- guess I should have pasted ¯\_(ツ)_/¯ to include the smiling face.

Re :#35: I'm not trying to block anything. I'm trying to get this issue ready for the core committers to look at it. RTBC means we're reasonably sure, as a community, that everything obvious has already been addressed. We try to give them every possible reason to actually commit it, fix/solve the issue, and move on. Every time a core committer has to review an RTBC issue and set it to NW is time they could have been fixing another issue, instead. That's why I do such thorough (at times, pedantic) reviews, b/c that's what core committers (which I'm not, and never have been) do.

So, before this is really RTBC, I believe the following would help:

1. +++ b/core/modules/update/tests/src/Kernel/UpdateReportTest.php
   @@ -0,0 +1,57 @@
   +   * @dataProvider providerTemplatePreprocessUpdateReportWithWrongData
   ...
   +  public function testTemplatePreprocessUpdateReportWithWrongData($variables) {
   ...
   +  public function providerTemplatePreprocessUpdateReportWithWrongData() {
   

   I think we can lose "WithWrongData" from both the test and provider function names.

2. (From the interdiff)
   

   +++ b/core/modules/update/tests/src/Kernel/UpdateReportTest.php
   @@ -20,30 +21,17 @@
   -      'no_updates_message' => _update_no_data(),
   ...
   +    // Test that the key "no_updates_message" has been set.
   +    $this->assertArrayHasKey('no_updates_message', $variables);
   

   Thanks, this is definitely better than the previous test. However, shall we also assert that $variables['no_updates_message'] is equal to the output from _update_no_data()?

3. +++ b/core/modules/update/tests/src/Kernel/UpdateReportTest.php
   @@ -0,0 +1,57 @@
   +   * Data provider for expected version information.
   

   "Data provider for testTemplatePreprocessUpdateReport()."

4. +++ b/core/modules/update/tests/src/Kernel/UpdateReportTest.php
   @@ -0,0 +1,57 @@
   +   *   Array of $variables.
   

   "Array of $variables for template_preprocess_update_report()."

   ?

For #14.3: All I was asking was for someone to dig a little deeper. I just did and the answer is very simple. This theme template is only invoked in one place in all of core, core/modules/update/src/Controller/UpdateController.php:

  public function updateStatus() {
    $build = [
      '#theme' => 'update_report',
    ];
    if ($available = update_get_available(TRUE)) {
      $this->moduleHandler()->loadInclude('update', 'compare.inc');
      $build['#data'] = update_calculate_project_data($available);
    }
    return $build;
  }

If update_get_available() fails or returns empty, this controller callback still returns a render array with #theme => 'update_report' but #data will be empty. Point addressed. No shouting required. 😉 Sorry for the false alarm.

If it was much more complicated than that, I would have probably opened a follow-up to investigate and try to get this fix RTBC. Glad we don't need to.

Thanks,
-Derek

@dww Thanks
Please find new patch with changes as suggested by you :)
RE #40.1 : Done
RE #40.2 : Not sure
RE #40.3 : Done
RE #40.4 : Done

However, shall we also assert that $variables['no_updates_message'] is equal to the output from _update_no_data()?

That is what I did in the patch from comment #12 and you (@dww) did not like that. The other solution is to hard code the return value of _update_no_data(), only that is not an easy thing to do.
In all 3 testcases in the provider method should result in that the value of $variables['data'] will be an empty array. The key $variables['no_updates_message'] will only be set when the value of $variables['data'] is empty. Therefor asserting that the $variables['no_updates_message'] is equal to the output from _update_no_data() will not improve the testing of the bug fix.

The other 3 points of @dww from comment #40 are addressed.

I leave it up to @dww to change status to RTBC.

Thanks for the new patch in #42 and the review in #44.

Re: #40.2 / #44: I was talking about doing something like:

    // Test that the key "no_updates_message" has been set.
    $this->assertArrayHasKey('no_updates_message', $variables);
    $this->assertEquals(_update_no_data(), $variables['no_updates_message']);

Maybe that's not needed. I'm not sure I follow the analysis about data being empty vs. invalid. Seems like in all cases, we should see the same end user text. /shrug Regardless, the above is an improvement over how this test was written in previous patches.

Re: #40.3 sorry, I missed. :( I forgot such comments always need to start with a verb. I should have said:
Provides data for testTemplatePreprocessUpdateReport().

Finally, this patch only applies cleanly to 9.1.x branch. A separate patch for 9.0.x applies cleanly all the way back to 8.8.x.

These patches re-fix #40.2 and should apply everywhere we need them. I'm fine to ignore #40.2, but I'm open to adding it if a core committer prefers.

Since this is a simple doc-only change from the previous patch, and a trivial re-roll for other branches, I'm going to self-RTBC once the bot comes back green (unless someone else wants to do it). I don't see anything else to improve in here.

Thanks,
-Derek

Bot's happy.
No CS errors/warnings.
All backport patches are ready.
This is 98% other people's code.
I've done a series of thorough and pedantic reviews, nothing else to complain about that I can see.

Although it's still somewhat worrisome that update_get_available() is sometimes returning nothing (which I can't reproduce locally, even with a totally cleared cache, laptop offline, etc), it clearly can/does happen, and we shouldn't assume we've got data when we don't. This fix is not masking deeper problems. If we want to have a follow-up to investigate when/how update_get_available() returns nothing, we can, but that's not a reason to delay fixing this bug any further.

Therefore, RTBC.

Thanks,
-Derek

p.s. Also crediting @mmjvb for reviews and contributions.

After updating Guzzle to 6.5.4 this error is completely gone. I can now resume running the /admin/reports/updates page successfully.

Please note that I add this error on a single Drupal 8 installation (on Drupal 8.8.5). So it may be hard to reproduce.

For those still experiencing the issue, can you update Guzzle and test?

@xmacinfo: What version of Guzzle were you using when it didn't work? That could be useful info.

Thanks,
-Derek

@xmacinfo that is interesting -- if you could come up with steps to reproduce we could open a followup.

I read over the whole of template_preprocess_update_report() and this looks like the right change. It even checks whether the data is empty a few lines after it throws the notice.

I too would like to know why empty data is making its way in to start with, especially under these weird specific circumstances, and I came to this issue fully expecting to NW for that. It is important to understand where unexpected data is coming from, in order to decide whether to add a new code path, change the callers, throw an exception, etc. But the current code already has a check for emptiness of the data -- just the wrong kind of check. So initialization is an OK choice based on that.

@pavnish and @shaktik, thanks for your attempts to help with this issue. In the future please:

• Include an interdiff each time you upload a new patch.
• Carefully read the feedback on the issue. :)
• Don't include fixes that are not related to the exact problem.

I am tagging the issue "Needs followup" for the coding standards changes you proposed earlier. It would be best done as part of a change to all of core for each rule. So all these things:

39 Expected newline after closing brace
40 Line indented incorrectly; expected 4 spaces, found 5
62 Case breaking statement indented incorrectly; expected 10 spaces, found 11
77 Closing brace indented incorrectly; expected 5 spaces, found 4

Instead of fixing all that on this one file, you'd take e.g. "39 Expected newline after closing brace" and create a patch that fixes that everywhere in core. An issue might already exist for that also and I encourage you to participate in #2571965: [meta] Fix PHP coding standards in core and #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard.

The patch itself looks great. Committed to 9.1.x, 9.0.x, 8.9.x, and 8.8.x.

As I mentionned earlier (with a typo):

…I had this error on a single Drupal 8 installation (on Drupal 8.8.5)

It is very hardto reproduce.

Short story

I applied some composer updates to upgrade Drupal 8.8 on a local environment and in production. This is basically the same site, same feature, theme and content but updated through Composer, no pipeline or git used.

Local

Running composer updates a few times on local to finally be able to upgrade Drupal and vendor dependencies and the sites works, any Guzzle version.

Production

Running the same composer updates gave me a bit of hard time, some dependencies mixup. But I finally was able to update Drupal and vendor dependencies. However, running /admin/reports/updates gave me two levels of errors:

1st run
I got a Guzzle error stack (sorry I did not copy the stack).

Subsequent runs
Instead of the Guzzle error, I got the error related to this issue: PHP message: PHP Warning: Invalid argument supplied for foreach() in core/modules/update/update.report.inc on line 43

For the Guzzle story, I took some hints from: https://www.drupal.org/project/drupal/issues/2715499 as Guzzle is used on the update page.

Now, what is strange

On the production site, no matter how I tried to reinstall Composer dependencies, I was stuck with the error.

But yesterday, while updating Guzzle to 6.5.4, everything was fixed without applying a single patch. Composer also updated a few symfony/polyfill alongside.

And what is even stranger, rollbacking to Guzzle 6.5.3 did not break the page. In fact, I am still on 6.5.3 now.

I have no clue as to why yesterday's Guzzle update fixed the issues, and I am in the dark as to why it works with the earlier version.

@xjm Thanks
I will take care #53 in future :)

Thanks
Pavnish

@xjm Thanks

I will take care #53 in future.

Thanks,
Shakti

@xmacinfo, thanks for the followup. I wonder if the first, broken update saved broken project data that got cached, and that caused the error? Or something along those lines.

@xjm: If I remember correctly, while I tried to investigate:

Deleting database content in the key_value table “ WHERE collection = ‘update_fetch_task’ gave me the Guzzle error in the first run, and then the error if this issue when running the update status again.

Doing a Drush CR did not solve the issue. But then again, Drush CR may not refresh all caches.

Finally, cache or not, using a different version of Guzzle fixed the site. Maybe it forced something in the database and once fixed, any version of Guzzle would work, as I am still on 6.5.3.

I am affraid I did not collect more info.