WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 10 months ago

Last modified 10 months ago

#45018 closed defect (bug) (fixed)

Non-string needle parameters throw warning in PHP 7.3

Reported by: desrosj Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Menus Keywords: php73 has-patch
Focuses: Cc:
PR Number:

Description

In PHP 7.3, a new warning was added when a non-string is passed as a needle to strpos(), or another similar function (see the link for a full list).

String search functions usually operate on a string needle. However, if a non-string is passed, it will be converted to an integer and interpreted as an ASCII codepoint.

The warning will be removed in PHP 8 and the needle parameter will be changed to a string.

Currently, there is one test displaying this warning when tests are run on PHP 7.3.

Tests_Nav_Menu_Theme_Change::test_numerical_locations

stripos(): Non-string needles will be interpreted as strings in the future. Use an explicit chr() call to preserve the current behavior

It's possible there are more instances, but this is the only one being exposed with tests.

Attachments (3)

45018.diff (2.3 KB) - added by desrosj 11 months ago.
45018.2.diff (2.6 KB) - added by desrosj 11 months ago.
45018.3.diff (2.3 KB) - added by desrosj 11 months ago.
Backports r43899 to the 4.9 branch

Download all attachments as: .zip

Change History (16)

#1 @pento
12 months ago

  • Milestone changed from 4.9.9 to 5.0

#2 @desrosj
12 months ago

  • Keywords php73 added

This ticket was mentioned in Slack in #core by jorbin. View the logs.


12 months ago

#4 @jorbin
12 months ago

I've started working on a patch and the progress can be seen on https://github.com/WordPress/wordpress-develop/pull/28

@desrosj
11 months ago

#5 @desrosj
11 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

#6 @pento
11 months ago

@desrosj, @jorbin: Why is the new test in 45018.diff is set to fail? The same failure occurs if I run that test with the changes to src/wp-includes/nav-menu.php reverted.

#7 @desrosj
11 months ago

Sorry, my second sentence didn't post in my comment for some reason. The test still needs work. I am working on that tomorrow.

#8 @desrosj
11 months ago

Alright, updated patch incoming. If only the test is added, it will pass on all versions of PHP except 7.3. On 7.3, it will throw the same warning the other test method was throwing. The additional changes in the patch fix the issue for both tests.

Here are a few builds:

I fell into a bit of a rabbit hole trying to figure out why the test was failing and realized it was due to funky behavior with numeric menu slugs. I have opened #45361 to better address that.

This should also be the last change preventing the 7.3 job from passing. I removed 7.3 from the allowed failures list in the Travis configuration in the new patch.

@desrosj
11 months ago

#9 @pento
11 months ago

In 43899:

Nav Menus: Fix a PHP 7.3 error when switching themes.

When switching themes, wp_map_nav_menu_locations() is used to ensure nav menus are placed in the relevant menu location. Occasionally, menus are registered to locations with numeric slugs, rather than strings. wp_map_nav_menu_locations() assumed it would be the latter, and ran stripos() on those numeric slugs. This behaviour is deprecated in PHP 7.3.

As this is the last known PHP 7.3 incompatibility, this commit also removes PHP 7.3 from Travis' allowed_failures list.

Props desrosj, jorbin.
See #45018.

#10 @pento
11 months ago

  • Keywords fixed-5.0 added; needs-testing removed

@desrosj
11 months ago

Backports r43899 to the 4.9 branch

#11 @desrosj
10 months ago

In 44167:

Nav Menus: Fix a PHP 7.3 error when switching themes.

When switching themes, wp_map_nav_menu_locations() is used to ensure nav menus are placed in the relevant menu location. Occasionally, menus are registered to locations with numeric slugs, rather than strings. wp_map_nav_menu_locations() assumed it would be the latter, and ran stripos() on those numeric slugs. This behavior is deprecated in PHP 7.3.

As this is the last PHP 7.3 error in unit tests, this commit also removes PHP 7.3 from Travis' allowed_failures list.

Props pento, desrosj, jorbin.

Merges [43899] to trunk.

See #45018.

#12 @desrosj
10 months ago

  • Keywords fixed-5.0 removed
  • Resolution set to fixed
  • Status changed from new to closed

Fixed by [44167].

#13 @desrosj
10 months ago

  • Component changed from General to Menus
Note: See TracTickets for help on using tickets.