Skip to content

PHP 8.4 | Fix implicitly nullable parameters (Trac 62061)#7369

Closed
jrfnl wants to merge 2 commits into
WordPress:trunkfrom
jrfnl:trac-62061/php-8.4-implicitly-nullable-fixes
Closed

PHP 8.4 | Fix implicitly nullable parameters (Trac 62061)#7369
jrfnl wants to merge 2 commits into
WordPress:trunkfrom
jrfnl:trac-62061/php-8.4-implicitly-nullable-fixes

Conversation

@jrfnl

@jrfnl jrfnl commented Sep 17, 2024

Copy link
Copy Markdown
Member

PHP 8.4 | WP_HTML_Processor: fix implicitly nullable parameter [1]

PHP 8.4 deprecates implicitly nullable parameters, i.e. typed parameters with a null default value, which are not explicitly declared as nullable.

This commit the one instance of this in the WP_HTML_Processor class.

Fixed by adding the nullability operator to the type, which is supported since PHP 7.1, so we can use it now the minimum supported PHP version is PHP 7.2.

As this deprecation is thrown at compile time, it can be seen at the top of the test output when running on PHP 8.4 (which will be gone once this change has been committed). It is not possible to write a test to cover this.

Ref: https://wiki.php.net/rfc/deprecate-implicitly-nullable-types

PHP 8.4 | Tests_HtmlApi_WpHtmlProcessorComments: fix implicitly nullable parameters [2]

PHP 8.4 deprecates implicitly nullable parameters, i.e. typed parameters with a null default value, which are not explicitly declared as nullable.

The Tests_HtmlApi_WpHtmlProcessorComments test class contains one problematic parameter in the test_comment_processing() method declaration.

While this could be fixed by adding the nullability operator, I've elected not to do this, but to remove the type declarations in the test method instead, including other type declarations for this method and the second test method, which were not affected by the deprecation.

The reason for this is quite straight-forward: using type declarations in tests is bad practice and inhibits defense-in-depth type testing.

Using type declarations in tests prevents being able to test the "code under test" with unexpected input types as the values with unexpected (scalar) types will be juggled to the expected type between the data provider and the test method and the real data value would therefore never reach the method under test.

The knock-on effects of this are:

  • That the input handling of the "code under test" can not be safeguarded, whether this input handling is done via in-function type checking or via a type declaration in the "code under test".
  • That if such "unexpected data type" tests are added to the data provider, they will silently pass (due to the type being juggled before reaching the "code under test"), giving a false sense of security, while in actual fact, these data sets would not be testing anything at all and if, for instance, the type declaration in the "code under test" would be removed, these tests would still pass, while by rights they should start failing.

Also note that this problem would only be exacerbated if the file would be put under strict_types.

Ref: https://wiki.php.net/rfc/deprecate-implicitly-nullable-types

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


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

PHP 8.4 deprecates implicitly nullable parameters, i.e. typed parameters with a `null` default value, which are not explicitly declared as nullable.

This commit the one instance of this in the `WP_HTML_Processor` class.

Fixed by adding the nullability operator to the type, which is supported since PHP 7.1, so we can use it now the minimum supported PHP version is PHP 7.2.

As this deprecation is thrown at compile time, it can be seen at the top of the test output when running on PHP 8.4 (which will be gone once this change has been committed). It is not possible to write a test to cover this.

Ref: https://wiki.php.net/rfc/deprecate-implicitly-nullable-types
…ble parameters [2]

PHP 8.4 deprecates implicitly nullable parameters, i.e. typed parameters with a `null` default value, which are not explicitly declared as nullable.

The `Tests_HtmlApi_WpHtmlProcessorComments` test class contains one problematic parameter in the `test_comment_processing()` method declaration.

While this could be fixed by adding the nullability operator, I've elected not to do this, but to remove the type declarations in the test method instead, including other type declarations for this method and the second test method, which were not affected by the deprecation.

The reason for this is quite straight-forward: using type declarations in tests is bad practice and inhibits defense-in-depth type testing.

Using type declarations in tests prevents being able to test the "code under test" with unexpected input types as the values with unexpected (scalar) types will be juggled to the expected type between the data provider and the test method and the _real_ data value would therefore never reach the method under test.

The knock-on effects of this are:
* That the input handling of the "code under test" can not be safeguarded, whether this input handling is done via in-function type checking or via a type declaration in the "code under test".
* That if such "unexpected data type" tests are added to the data provider, they will silently pass (due to the type being juggled before reaching the "code under test"), giving a false sense of security, while in actual fact, these data sets would not be testing anything at all and if, for instance, the type declaration in the "code under test" would be removed, these tests would still pass, while by rights they should start failing.

Also note that this problem would only be exacerbated if the file would be put under `strict_types`.

Ref: https://wiki.php.net/rfc/deprecate-implicitly-nullable-types
@github-actions

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jrf.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@hellofromtonya hellofromtonya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and ready for commit 👍

@hellofromtonya

Copy link
Copy Markdown
Contributor

@hellofromtonya hellofromtonya deleted the trac-62061/php-8.4-implicitly-nullable-fixes branch September 18, 2024 15:04
@jrfnl

jrfnl commented Sep 18, 2024

Copy link
Copy Markdown
Member Author

Thanks @hellofromtonya!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants