WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 12 months ago

Last modified 8 months ago

#50343 closed enhancement (fixed)

[PHP 8] Fix deprecation notices for optional function parameters declared before required parameter.

Reported by: ayeshrajans Owned by: SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch php8 has-unit-tests has-dev-note
Focuses: coding-standards Cc:

Description

PHP 8 deprecated required declaring functions/methods that have optional parameters before required ones.

See https://php.watch/versions/8.0/deprecate-required-param-after-optional

function foo($param_optional = null, $param_required) {
//           ^^ optional parameter , ^^ required parameter
}

Note that the PHP 8 tests still fail, but that is the bigger picture.
Snippet above will raise a deprecation notice in PHP 8:

Deprecated: Required parameter $param_required follows optional parameter $param_optional in ... on line ...

WordPress currently has 6 of such deprecation notices raised in PHP 8 CI tests: https://travis-ci.com/github/WordPress/wordpress-develop/jobs/345635046

I have gone through these 6 declarations, and updated their default values to stop this notice from being raised. The default values are determined by the caller/callee default values and return types.

Attachments (2)

50343.patch (3.2 KB) - added by ayeshrajans 14 months ago.
This patch fixes the 6 deprecation notices related to function parameter order (https://php.watch/versions/8.0/deprecate-required-param-after-optional)
50343-required-parameters-before-optional.patch (4.5 KB) - added by jrf 12 months ago.

Download all attachments as: .zip

Change History (23)

@ayeshrajans
14 months ago

This patch fixes the 6 deprecation notices related to function parameter order (https://php.watch/versions/8.0/deprecate-required-param-after-optional)

#1 @ayeshrajans
14 months ago

  • Focuses coding-standards added
  • Keywords has-patch added
  • Version set to trunk

#2 @desrosj
14 months ago

  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement
  • Version trunk deleted

Thanks for getting the ball rolling on this one, @ayeshrajans! This seems reasonable, but I am going to mark as Future Release.

Historically, changes are not made to WordPress to support new versions of PHP until those versions enter the feature freeze/beta part of the release cycle. The idea is that this prevents additional work if the upstream implementation changes, or if features happen to be removed entirely. It also allows all of the compatibility changes to be bundled (as much as possible) into a single release, which makes communication through dev notes and documentation much easier.

I've updated the PHP Compatibility and WordPress Versions page of the handbook to reflect this.

If there are more tickets like this, please do go ahead and make them. But just know the priority for these will be for WP 5.6 later this year.

#3 @ayeshrajans
14 months ago

Thanks for the quick response @desrosj - I understand this series wouldn't be for WordPress 5.5 this August, which by that time only a PHP 8 alpha would be out.

5.6 certainly seems like the ideal target because PHP 8 release and 5.6 release would align more or less.

#4 @jorbin
14 months ago

  • Keywords php8 added

Adding a custom keyword to make it easier once work on supporting php8 begins

#5 @desrosj
14 months ago

  • Milestone changed from Future Release to 5.6

Since there is now a 5.6 milestone, moving this there as that's the current target for exploring this.

This ticket was mentioned in PR #469 on WordPress/wordpress-develop by jrfnl.


12 months ago

  • Keywords has-unit-tests added

As it already wasn't possible to pass the required parameters without also passing the optional one anyway, I'm removing the default value for the (not so) optional parameters and documenting the expected value to "skip" the parameter in the docblock instead.

Trac ticket: https://core.trac.wordpress.org/ticket/50343

#7 @jrf
12 months ago

I've reviewed the patch and for the most part, I would like to suggest an alternative approach.

The original approach by @ayeshrajans was to make any parameters after the optional parameter, also optional.

Save for one specific case (rest api test), I propose doing the opposite: making the optional parameter declared before required parameters, required.

As it already wasn't possible to pass the required parameters without also passing the optional one anyway, removing the default value for the (not so) optional parameters should not be regarded as a BC break.
I've also updated the docblock @param tags with the expected value to "skip" the parameter.

Other notes regarding this PHP 8 change:

Both SimplePie as well as GetID3 also have (had) instances of this issue.
I have opened PRs in both projects to fix this.

The PR in GetID3 has been merged already and is expected to be included in the next release.

#8 @SergeyBiryukov
12 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#9 @ayeshrajans
12 months ago

Thanks for picking up this one, @jrf, it's nice to see we getting an early start for PHP 8!

As for the approach of making these problematic functions drop their default values, PHP 7.1 and later throw ArgumentCountError< so users are already aware of this. However, for PHP 5.6 and 7.0 only raise a warning if the user does not pass a required parameter: https://3v4l.org/o1TKv

I think dropping the default values can have non-zero number of users who might have ignored the warning, but get an exception in WordPress 5.6.

#10 @jrf
12 months ago

I think dropping the default values can have non-zero number of users who might have ignored the warning, but get an exception in WordPress 5.6.

If they did, IMO, they did so at their own peril and the plugin/theme would already break on PHP 7.1+.

As PHP < 7.1 support is slated to be removed from WP anyway, I don't think that plugins/themes which currently aren't compatible with PHP 7.1+ should be a concern.

Also note that _wp_delete_tax_menu_item() is marked as a private function, so has no promise of backward-compatibility anyhow.

Even so, for arguments sake, I've done a plugin + theme repository search to see if I could find any problematic plugins or themes and the net result is: 0.

Search results:

#11 @ayeshrajans
12 months ago

That's awesome, thank you.

#12 @SergeyBiryukov
12 months ago

50343-required-parameters-before-optional.patch looks great, I only have some notes on the docs:

  • _wp_delete_tax_menu_item() is a private function hooked to delete_term, which always passes a non-zero term ID, so it doesn't look like documenting "Pass the integer 0 if the object id is unknown" is necessary.
  • It might be a good idea to also make the $object_id parameter of _wp_delete_post_menu_item() required, for consistency. It's hooked to delete_post, which also always passes a non-zero post ID. Both functions were introduced in [14295]. Calling them without a valid object ID doesn't make much sense anyway.
  • get_comment_delimited_block_content() could use some clarification of the null value, something like: "Block name. Null if the block name is unknown, e.g. Classic blocks have their name set to null".

Going to handle these adjustments on commit.

#13 @SergeyBiryukov
12 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 48794:

Code Modernization: Fix PHP 8 deprecation notices for optional function parameters declared before required parameters.

As it already was not possible to pass the required parameters without also passing the optional one anyway, removing the default value for the (not so) optional parameters should not affect backward compatibility.

This change affects three functions in core:

  • get_comment_delimited_block_content()
  • do_enclose()
  • _wp_delete_tax_menu_item()

Props jrf, ayeshrajans, desrosj.
Fixes #50343.

#14 @jrf
12 months ago

Good points about the docs. Thanks @SergeyBiryukov for doing the finishing touches ;-)

#15 @SergeyBiryukov
12 months ago

In 48795:

Menus: Make the $object_id parameter of _wp_delete_post_menu_item() required, for consistency with _wp_delete_tax_menu_item().

The function is private (only intended for core usage) and is hooked to the delete_post action, which always passes a non-zero post ID.

Follow-up to [14295], [48794].

See #50343.

#16 @prbot
12 months ago

jrfnl commented on PR #469:

Patch committed. Closing.

#17 @jrf
12 months ago

  • Keywords needs-dev-note added

Adding needs-dev-note to make sure this change gets mentioned in one of the WP 5.6 dev posts.

#18 @jrf
10 months ago

SimplePie has released a new version containing the fix mentioned above. See #51521

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


9 months ago

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


8 months ago

#21 @daisyo
8 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.