Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.4 | Remove use of xml_set_object() #172

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Sep 13, 2024

The XML Parser extension still supports a quite dated mechanism for method based callbacks, where the object is first set via xml_set_object() and the callbacks are then set by passing only the name of the method to the relevant parameters on any of the xml_set_*_handler() functions.

xml_set_object( $parser, $my_obj );
xml_set_character_data_handler( $parser, 'method_name_on_my_obj' );

Passing proper callables to the xml_set_*_handler() functions has been supported for the longest time and is cross-version compatible. So the above code is 100% equivalent to:

xml_set_character_data_handler( $parser, [$my_obj, 'method_name_on_my_obj'] );

The mechanism of setting the callbacks with xml_set_object() has now been deprecated as of PHP 8.4, in favour of passing proper callables to the xml_set_*_handler() functions. This is also means that calling the xml_set_object() function is deprecated as well.

This commit fixes these deprecations for this codebase.

Refs:

@jrfnl jrfnl requested a review from dd32 September 25, 2024 23:20
@jrfnl
Copy link
Member Author

jrfnl commented Sep 25, 2024

@dd32 While not strictly necessary for WP Core, I still think it would be good to tag a new release once this is merged to prevent end-user reports about the deprecations once PHP 8.4 gets released.

Also be aware that - aside from the other PRs I already opened, I have one more PR lined up (to enable running the tests against PHP 8.4, but it needs this PR to allow for a passing build).

A detailed analysis of WP vs PHP 8.4 is available here: https://core.trac.wordpress.org/ticket/62061

This is the summary from that report which relates to the Importer plugin:

WordPress Importer plugin ⚠

This plugin is used in the test suite.

Current status:

  • Tests are NOT (yet) being run against PHP 8.4.
  • Test coverage is ~68% (non-strict).
  • PHPCompatibility (bleeding edge): found one issue. PR to fix this has been submitted.
  • The plugin should tag a new version once this PR has been merged.
  • WordPress automatically installs the latest master of the plugin when the Docker container is installed, so once the above mentioned PR has been merged, the related Core automated tests will be able to pass.

Important:
Contributors not using Docker for their local dev environment, will need to update their copy of the plugin as used for the tests. This should be mentioned in a dev-note.
Also see: https://make.wordpress.org/core/handbook/contribute/git/#unit-tests

jrfnl added a commit to jrfnl/wordpress-develop that referenced this pull request Oct 3, 2024
PHP 8.4 is expected to be released at the end of November 2024.

Trac ticket 62061 contains a full analysis of the changes needed for WP to be ready for PHP 8.4 and all currently available patches for WP Core have been merged in the mean time.

Enabling the tests to run in CI on PHP 8.4 allows WordPress core to prevent new PHP 8.4 compatibility issues from being introduced.

Notes:
* While a [patch has been pulled](WordPress/wordpress-importer#172) for the Importer plugin, this patch has not been merged yet.
    This means there are currently still five failing tests. These failure are directly related to the Importer plugin and will be fixed once the PR for the plugin has been merged.
    To still allow WP Core to start structurally testing against PHP 8.4, these five tests have been set up now to be skipped on PHP 8.4.
    These tests skips should be removed again once the PR to the plugin has been merged.
* Xdebug-related tests will not be run on PHP 8.4 at this time as the Docker image for PHP 8.4 does not contain Xdebug yet. Once a stable release of Xdebug 3.4.0 is available, it can be added to the Docker image and the test step can then be enabled for PHP 8.4.
Copy link

@xxsimoxx xxsimoxx left a comment

Choose a reason for hiding this comment

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

At ClassicPress we have tested the same fix and works well. PHPUnit tests are passing with PHP 8.4.

@jrfnl jrfnl mentioned this pull request Oct 17, 2024
Copy link
Member

@dd32 dd32 left a comment

Choose a reason for hiding this comment

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

I approve, and agree it would be best to be included in the next minor release of the plugin.

@jrfnl
Copy link
Member Author

jrfnl commented Oct 18, 2024

@dd32 Want me to rebase the PR to make it mergable again ?

@ouikhuan
Copy link
Contributor

@jrfnl yes please, could you rebase it so it can be mergeable again? We'll include this in our next release. Thanks for pointing this out!

The XML Parser extension still supports a quite dated mechanism for method based callbacks, where the object is first set via `xml_set_object()` and the callbacks are then set by passing only the name of the method to the relevant parameters on any of the `xml_set_*_handler()` functions.
```php
xml_set_object( $parser, $my_obj );
xml_set_character_data_handler( $parser, 'method_name_on_my_obj' );
```

Passing proper callables to the `xml_set_*_handler()` functions has been supported for the longest time and is cross-version compatible. So the above code is 100% equivalent to:
```php
xml_set_character_data_handler( $parser, [$my_obj, 'method_name_on_my_obj'] );
```

The mechanism of setting the callbacks with `xml_set_object()` has now been deprecated as of PHP 8.4, in favour of passing proper callables to the `xml_set_*_handler()` functions. This is also means that calling the `xml_set_object()` function is deprecated as well.

This commit fixes these deprecations for this codebase.

Refs:
* https://wiki.php.net/rfc/deprecations_php_8_4#xml_set_object_and_xml_set_handler_with_string_method_names
* https://www.php.net/manual/en/function.xml-set-object.php
* https://www.php.net/manual/en/ref.xml.php
@dd32 dd32 force-pushed the feature/php-8.4-fix-xml-set-handlers branch from 4e9fb3f to f80b99b Compare October 21, 2024 05:23
@dd32 dd32 merged commit 5adf384 into master Oct 21, 2024
26 checks passed
@dd32 dd32 deleted the feature/php-8.4-fix-xml-set-handlers branch October 21, 2024 05:39
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.

5 participants