The following functions incorrectly assumes that menu_get_item() always returns an array, but this is not true: For 404 pages, for example, it returns FALSE. With PHP 7.4, these functions cause a notice.

With PHP 7.4, block_page_build() doesn't shown any block on 404 pages.

CommentFileSizeAuthor
#10 3085088-10.patch1.54 KBmcdruid
#10 interdiff-3085088-7-10.txt565 bytesmcdruid
#7 3085088-7.patch1.47 KBmcdruid
#7 interdiff-3085088-4-8.patch8.24 KBmcdruid
#4 drupal-menu_get_item_false-3085088-4.patch8.84 KBTaran2L
#2 drupal-block_page_build_null_as_array-3085088-2.patch3.08 KBTaran2L
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Taran2L created an issue. See original summary.

Taran2L’s picture

Taran2L
Primary language Ukrainian
Location Lviv
CreditAttribution: Taran2L at Lemberg Solutions commented
Status: Active » Needs review
FileSize
drupal-block_page_build_null_as_array-3085088-2.patch3.08 KB
Taran2L’s picture

Taran2L
Primary language Ukrainian
Location Lviv
CreditAttribution: Taran2L at Lemberg Solutions commented
Title: Trying to access array offset on value of type bool in block_page_build() » menu_get_item() might return FALSE; calling code should encounter for this
Issue summary: View changes
Taran2L’s picture

Taran2L
Primary language Ukrainian
Location Lviv
CreditAttribution: Taran2L at Lemberg Solutions commented
FileSize
drupal-menu_get_item_false-3085088-4.patch8.84 KB
apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno commented
Issue summary: View changes
MustangGB’s picture

MustangGB CreditAttribution: MustangGB commented
Title: menu_get_item() might return FALSE; calling code should encounter for this » menu_get_item() might return FALSE; calling code should take account of this

I guess this is what was meant.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
FileSize
interdiff-3085088-4-8.patch8.24 KB
3085088-7.patch1.47 KB

Wondering whether we can get away with a couple of very minimal changes which simply bail out if menu_get_item() returns FALSE.

Let's see what the tests say...

Couple of notes:

  • Neither of these functions exist in D8, so nothing to backport.
  • My editor's determined to remove a trailing whitespace around line 320 of menu.inc but trying to keep that out of the patch as it's unrelated.
mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented

Oops, the interdiff's obviously not meant to be a patch... bit distracted atm :)

Status: Needs review » Needs work

The last submitted patch, 7: 3085088-7.patch, failed testing. View results

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-3085088-7-10.txt565 bytes
3085088-10.patch1.54 KB

I'm actually surprised more tests didn't fail with that patch; I hadn't read the logic properly.

Here's a tweak to the change in block_page_build() which I've checked fixes at least one of the test fails.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Pending Drupal 7 commit

Tests look okay, I think.

Noting that I am sneaking in the removal of what looks like an unnecessary, duplicated call to menu_get_item() from block_page_build(). I don't see taking this out "while we're here" causing any harm, but we could leave it and file a followup if we want to keep this patch 100% about PHP 7.4

Hopefully this approach is easier to review than #4 which is a significantly bigger patch largely because of indentation changes.

AFAICS this is ready for final review.

MustangGB’s picture

MustangGB CreditAttribution: MustangGB commented

Noting that I am sneaking in the removal of what looks like an unnecessary, duplicated call to menu_get_item() from block_page_build().

Did wonder what that was in there for, thanks for mentioning why.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented

Confirmation that #10 resolves the PHP 7.4 exceptions from the two files we're changing (compared against the most recent baseline test in #3081386-60: [META] Fully support PHP 7.4 in Drupal 7 ):

$  curl -s https://www.drupal.org/pift-ci-job/1617902 | grep -o 'exception: .* Line [0-9]* of .*:' | perl -pe 's#<.*?>##g' | sort | uniq -c | sort -rn | diff - <(curl -s https://www.drupal.org/pift-ci-job/1632307 | grep -o 'exception: .* Line [0-9]* of .*:' | perl -pe 's#<.*?>##g' | sort | uniq -c | sort -rn)
4d3
<      47 exception: [Notice] Line 266 of modules/block/block.module:
15,17d13
<       4 exception: [Notice] Line 2494 of includes/menu.inc:
<       4 exception: [Notice] Line 2488 of includes/menu.inc:
<       2 exception: [Notice] Line 2490 of includes/menu.inc:
Fabianx’s picture

Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented
Assigned: Unassigned » mcdruid

Approved, let's get this in.

  • mcdruid committed 8b0048c on 7.x 
    Issue #3085088 by mcdruid, Taran2L: menu_get_item() might return FALSE;...
mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid as a volunteer and at Acquia commented
Assigned: mcdruid » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Thanks all; one more step closer!

Status: Fixed » Closed (fixed)

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