WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 9 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:

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 2 years ago.
45018.2.diff (2.6 KB) - added by desrosj 2 years ago.
45018.3.diff (2.3 KB) - added by desrosj 2 years ago.
Backports r43899 to the 4.9 branch

Download all attachments as: .zip

Change History (17)

#1 @pento
2 years ago

  • Milestone changed from 4.9.9 to 5.0

#2 @desrosj
2 years ago

  • Keywords php73 added

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


2 years ago

#4 @jorbin
2 years ago

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

@desrosj
2 years ago

#5 @desrosj
2 years ago

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

#6 @pento
2 years 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
2 years 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
2 years 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
2 years ago

#9 @pento
2 years 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
2 years ago

  • Keywords fixed-5.0 added; needs-testing removed

@desrosj
2 years ago

Backports r43899 to the 4.9 branch

#11 @desrosj
2 years 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
2 years ago

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

Fixed by [44167].

#13 @desrosj
2 years ago

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