Make WordPress Core

Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#60786 closed defect (bug) (fixed)

[PHP 8.4] Fix implicit nullable parameter type depcation

Reported by: ayeshrajans's profile ayeshrajans Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: General Keywords: php84 has-patch
Focuses: php-compatibility Cc:

Description

In PHP 8.4, declaring function/method parameters with a default value of null is deprecated if the type is not nullable. RFC and PHP 8.4: Implicitly nullable parameter declarations deprecated

Out current minimum PHP requirement is PHP 7.0, but nullable types are only supported in PHP 7.1 and later.

Submitting the PR with the fixes and to start a conversation, but merging these changes without bumping minimum PHP requirement to 7.1 will break things on PHP 7.0.

Change History (15)

#1 @ayeshrajans
4 months ago

(gentle ping to @jrf, I'm sure Juliette was working on an automated fix, but I couldn't find a ticket. If there is one, I apologize and will happily close this one)

This ticket was mentioned in PR #6280 on WordPress/wordpress-develop by @ayeshrajans.


4 months ago
#2

Fixes all issues that emit deprecation notices on PHP 8.4 for implicit nullable parameter type declarations.

See:

Exclude potential changes in the Requests library and sodium_compat.

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

#3 @audrasjb
4 months ago

  • Version trunk deleted

Thanks for the ticket and patch.
Just noting that this implementation would require to drop support for PHP 7.0 ahead of the change.

#4 @jrf
4 months ago

  • Version set to trunk

@ayeshrajans Thanks for the ping. I'm not working on an automated solution (as the solution should be decided on a case-by-case basis), though I have written a sniff to reliably find the issues already ;-)

I've had a look at your patch, but as the minimum PHP version is currently still 7.0, the patch is not viable.

Aside from the export.php and the media.php file, the other files are relatively new files, which should never have been allowed to be committed with scalar type declarations as without enforcing strict types throughout the eco-system (which we can't), that's next to useless and will hide bugs, so in my opinion, removing the type declarations from the function signatures in those files would be the correct solution direction.

As for export.php and media.php, as the involved functions are global functions, there is no risk of the function being overloaded in a child class, so removing the type and optionally, selectively casting to array from within the function should allow for a workable solution.

#5 @jorbin
4 months ago

  • Milestone changed from Awaiting Review to 6.6

As there is a discussion about raising the minimum version during 6.6, I'm moving this to that milestone.

#6 @audrasjb
4 months ago

  • Version trunk deleted

Removing again trunk version label as this is not something only introduced in trunk.

#7 @jrf
4 months ago

As the version bump to a minimum of PHP 7.2 has been approved, let's keep an eye on the bump patch going in and get this patch committed as soon as the version drop has been committed to trunk.

#8 @SergeyBiryukov
3 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 58009:

Code Modernization: Fix implicit nullable parameter type deprecation on PHP 8.4.

In PHP 8.4, declaring function or method parameters with a default value of null is deprecated if the type is not nullable.

PHP applications are recommended to explicitly declare the type as nullable. All type declarations that have a default value of null, but without declaring null in the type declaration, will emit a deprecation notice:

function test( array $value = null ) {}

Deprecated: Implicitly marking parameter $value as nullable is deprecated, the explicit nullable type must be used instead

Recommended Changes

Change the implicit nullable type declaration to a nullable type declaration, available since PHP 7.1:

- function test( string $test = null ) {}
+ function test( ?string $test = null ) {}

This commit updates the affected instances in core to use a nullable type declaration.

References:

Follow-up to [28731], [50552], [57337], [57985].

Props ayeshrajans, jrf, audrasjb, jorbin.
Fixes #60786.

@SergeyBiryukov commented on PR #6280:


3 months ago
#9

Thanks for the PR! Merged in r58009.

#10 follow-up: @GaryJ
3 months ago

@SergeyBiryukov The RFC gives an example code change, that drops the = null when the nullable type is present. Since the default value is null when another value is not assigned to a function parameter, could we update the entries you changed from (for example):

- function test( ?string $test = null ) {}
+ function test( ?string $test ) {}

#11 @SergeyBiryukov
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#12 in reply to: ↑ 10 ; follow-up: @jrf
3 months ago

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

Replying to GaryJ:

@SergeyBiryukov The RFC gives an example code change, that drops the = null when the nullable type is present. Since the default value is null when another value is not assigned to a function parameter, could we update the entries you changed from (for example):

- function test( ?string $test = null ) {}
+ function test( ?string $test ) {}

@GaryJ Making a parameter nullable does not make it optional. Removing the default value is a signature change which will break code: https://3v4l.org/L9ZAP

The only time this change can/should be made is when the "optional" (implicitly nullable) parameter was _before_ a required parameter.

In all other cases, removing the default value is a breaking change.

@SergeyBiryukov Please do NOT action this.

#13 in reply to: ↑ 12 @SergeyBiryukov
3 months ago

Replying to jrf:

Making a parameter nullable does not make it optional. Removing the default value is a signature change which will break code: https://3v4l.org/L9ZAP

Thanks for the clarification!

#14 follow-up: @GaryJ
3 months ago

I think there are three instances of the changeset that could be updated.

#15 in reply to: ↑ 14 @jrf
3 months ago

Replying to GaryJ:

I think there are three instances of the changeset that could be updated.

Which ones are you talking about ? Looking at [58009], I only see parameters which are at the end of the parameter list for a function (i.e. truly optional) and for which this would be a breaking change.

Note: See TracTickets for help on using tickets.