WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 19 months ago

#31982 closed task (blessed) (fixed)

WordPress PHPUnit Tests should run error free on PHP7 - 4.3 tracking ticket

Reported by: jorbin Owned by: netweb
Milestone: 4.3 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description (last modified by jorbin)

PHP7 is scheduled for release in October. In order to help facilitate that, we should get our PHPUnit tests in working order on PHP7.

Right now, there are a number of deprecated warnings and the tests don't run completely. We need to fix up the deprecated warnings and also ensure all tests run error free (while maintaining backwards compatability).

This ticket should be used for the small pieces. Larger changes that require there own discussion should be spun off into there own ticket.

This is a followup to #31454.

Attachments (9)

31982.diff (27.5 KB) - added by jdgrimes 2 years ago.
PHP4ConstructorSniff.php (3.1 KB) - added by jdgrimes 2 years ago.
PHP_CodeSniffer sniff used to generate the patch
31982.1.diff (28.2 KB) - added by netweb 2 years ago.
wp-import.diff (876 bytes) - added by jdgrimes 2 years ago.
Fix deprecated warnings for WP_Import
31982.2.diff (2.0 KB) - added by jorbin 2 years ago.
31982.3.diff (2.0 KB) - added by DrewAPicture 2 years ago.
Docs tweaks
31982.4.diff (27.9 KB) - added by jorbin 2 years ago.
31982.5.diff (1.1 KB) - added by johnbillion 2 years ago.
31982.6.diff (855 bytes) - added by ocean90 2 years ago.
See https://wiki.php.net/rfc/uniform_variable_syntax

Download all attachments as: .zip

Change History (48)

#1 @jorbin
2 years ago

  • Description modified (diff)
  • Summary changed from WordPress Unit Tests should run error free on PHP7 - 4.3 tracking ticket to WordPress PHPUnit Tests should run error free on PHP7 - 4.3 tracking ticket

#2 @johnbillion
2 years ago

  • Type changed from defect (bug) to task (blessed)

Here's a recent failing test for reference: https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/58632984 (not sure what's up with the WP_Import fatal).

#3 @jdgrimes
2 years ago

It looks like all of the deprecated warnings are caused by lingering PHP4 constructors:

Deprecated: Methods with the same name as their class will not be constructors in a future version of PHP

Are we OK with removing PHP4 constructors? I don't know if there is any other way to avoid the notice, but we've been keeping these for backward compatibility.

#4 @SergeyBiryukov
2 years ago

The WP_Widget PHP4 constructor was kept for compatibility with plugins, see ticket:16768:9, #20801, #30482.

The others can probably be removed or changed to PHP5 constructors.

#5 @jdgrimes
2 years ago

Original RFC for this change in PHP: https://wiki.php.net/rfc/remove_php4_constructors

Of particular note:

// function filter is not used as constructor in PHP 5+
class Filter {
    function __construct() {}
 
    // PHP 5.0.0 - 5.2.13, 5.3.0 - 5.3.2: E_STRICT "Redefining already defined constructor"
    // PHP 5.2.14 - 5.2.17, 5.3.3 - 5.6: No error is raised
    // PHP 7: No error is raised
    // PHP 8: No error is raised
    function filter($a) {}
}

What this means is that we don't need to remove any of our PHP4 constructors (yay!). But we do need to make sure that all of the external libraries' classes have a __construct() defined before a PHP4 constructor. Some of them do not have a __construct() method in addition to their PHP4 constructor, and that is what is causing the errors. Note that the order in which the constructors appear is important, the PHP5 one must come before the PHP4 one.

#6 follow-up: @jdgrimes
2 years ago

I'm compiling a list of classes which need PHP5 constructors added. Before I make up a patch, I'd like to know exactly how we want to handle this. We'll probably want to keep the PHP4 constructors for back-compat, but do we want to formally deprecate them? If not, should the new __construct() method call the PHP4 constructor, or should the logic be moved into __construct()?

#7 in reply to: ↑ 6 ; follow-up: @SergeyBiryukov
2 years ago

Replying to jdgrimes:

If not, should the new __construct() method call the PHP4 constructor, or should the logic be moved into __construct()?

I'd suggest following the current WP_Widget approach:

  • Move the logic to the new __construct() method.
  • Call the new __construct() method in the PHP4 constructor.

#8 in reply to: ↑ 7 ; follow-up: @jdgrimes
2 years ago

Replying to SergeyBiryukov:

I'd suggest following the current WP_Widget approach

OK, so the PHP4 constructors will just act as wrappers for the __construct() methods, and won't give any deprecation notices or anything.

I'll work on a patch.

#9 in reply to: ↑ 8 @jdgrimes
2 years ago

Replying to jdgrimes:

I'll work on a patch.

Now that trunk is 4.3, I'll try to get a patch up ASAP.

#10 @netweb
2 years ago

  • Component changed from General to Build/Test Tools
  • Milestone changed from Future Release to 4.3

Now that we have a 4.3 milestone...

@jdgrimes
2 years ago

@jdgrimes
2 years ago

PHP_CodeSniffer sniff used to generate the patch

#11 @jdgrimes
2 years ago

  • Keywords has-patch added

31982.diff is the first pass at a patch for the PHP5 constructors.

Notes:

  • The ftp classes already had PHP5 constructors, but they were below the PHP4 constructors. This will give PHP Strict Standards: Redefining already defined constructor for class ftp_base, so I've moved the PHP5 constructors to above the PHP4 ones.
  • I've added basic docblocks to most of the constructors, with just a short description ("PHP5 constructor"/"PHP4 constructor").
  • I've update whitespace on most of the modified lines.
  • I've mostly used tabs to indent things, even though a few of the files are space-indented. If anyone wants to clean this up, they are welcome to it.
  • I've added the public keyword for many of the PHP4 constructors, some of which didn't have this before.
  • FWIW PHP4ConstructorSniff.php is the PHPCS sniff I used to generate the initial patch (though I've modified both it and the patch since).

@netweb
2 years ago

#12 @netweb
2 years ago

Patch 31982.1.diff is a refresh only of 31982.diff as it wouldn't apply /src/wp-includes/pomo/streams.php cleanly, tweaked the patch to include a new line at the end of file and appears to apply cleanly for me now.

#13 @netweb
2 years ago

I ran 31982.1.diff through Travis CI, the resulting job is -> https://travis-ci.org/ntwb/wordpress/jobs/60068715

All the deprecated warnings are gone, nice one @jdgrimes.

Just left with the WP_Import class not found error thus far.

...........................................SSSSSSSSSSS....... 1769 / 3348 ( 52%)
......EPHP Fatal error:  Class 'WP_Import' not found in /home/travis/build/ntwb/wordpress/tests/phpunit/tests/import/base.php on line 24
Fatal error: Class 'WP_Import' not found in /home/travis/build/ntwb/wordpress/tests/phpunit/tests/import/base.php on line 24
Warning:  Use --force to continue.
Aborted due to warnings.
The command "grunt $WP_TRAVISCI" exited with 3.
Done. Your build exited with 1.

@jdgrimes
2 years ago

Fix deprecated warnings for WP_Import

#14 @jdgrimes
2 years ago

Thanks for testing it @netweb. WP_Import itself actually needs to be patched for deprecation errors (see wp-import.diff). I'm not sure who needs to do that. Maybe that is causing the class not to be loaded for some reason? Probably not, but let's patch it and see if the error goes away.

#15 follow-up: @netweb
2 years ago

The wp-import.diff patch should be added to #24373

Following that two discussions should be had:

  1. In #24373 should this update create a new v0.6.2 release, with previous commits in that ticket still not merged into the current v0.6.1 release, or merged only into /trunk
  1. Here on this ticket, should we change to using /trunk version of WordPress-Importer regardless of a tagged release in the plugin repo, the 1.0 beta release in /trunk is significantly improved, these improvements came from Frederick Ding's 2013 GSoC project but have never been deployed in a tagged release, only in /trunk

Edit: My bad, Frederick's enhancements are in the GSoC repo, none of these are in the plugins repo:

Last edited 2 years ago by netweb (previous) (diff)

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


2 years ago

#17 in reply to: ↑ 15 ; follow-up: @jdgrimes
2 years ago

Replying to netweb:

The wp-import.diff patch should be added to #24373

Done.

Here on this ticket, should we change to using /trunk version of WordPress-Importer regardless of a tagged release in the plugin repo...

I think we already do. At least the previous changes from that ticket are in the version checked out with my core checkout.

#18 in reply to: ↑ 17 @netweb
2 years ago

Replying to jdgrimes:

I think we already do. At least the previous changes from that ticket are in the version checked out with my core checkout.

My bad, you are correct, we already do use /trunk :)

This ticket was mentioned in Slack in #meta by netweb. View the logs.


2 years ago

#20 @obenland
2 years ago

  • Owner set to netweb
  • Status changed from new to assigned

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


2 years ago

#22 @netweb
2 years ago

Related: #32480

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


2 years ago

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


2 years ago

@jorbin
2 years ago

@DrewAPicture
2 years ago

Docs tweaks

#25 @jorbin
2 years ago

In 32989:

Add Deprecated Constructor Function

This function is one that can be called in core to indicate that a PHP4 style constructor is used. PHP4 style constructors are deprecated in PHP7.

Props jorbin, DrewAPicture for docs
See #31982

@jorbin
2 years ago

#26 @jorbin
2 years ago

In 32990:

Deprecate php4 style constructors

PHP7 is deprecating PHP4 style constructors, so we need to modify our code to have _construct methods that fire before the named PHP4 style constructors. The PHP4 style constructors will call the PHP5 style constructor in case it is being called directly (usually via parent::METHOD).

This modifies external libraries to add PHP5 style constructors, but doesn't add a notice for when they are used. In WordPress core code, PHP4 style constructors are being given a call to _deprecated_constructor. To the PHP4 style constructor I say "I know that I can't take no more | It ain't no lie | I wanna see you out that door | Baby, bye, bye, bye..."

Upstream: https://wiki.php.net/rfc/remove_php4_constructors

Props jdgrimes, netweb, jorbin
See #31982

#27 @obenland
2 years ago

In 33085:

Deprecate PHP4 style contructor in Twenty Eleven's Ephemera widget.

See #31982.

Props Frank Klein.
Fixes #32881.

#28 @jorbin
2 years ago

#32480 was marked as a duplicate.

@johnbillion
2 years ago

#29 @johnbillion
2 years ago

There's no reason to retain the PHP4 constructors in our test suite. 31982.5.diff removes these.

#30 @jorbin
2 years ago

In 33123:

Remove PHP4 constructors from Unit Tests

If you are subclassing these classes in your own tests, you'll need to update your code.

Props johnbillion
See #31982

#31 @jorbin
2 years ago

I am going to mark this as completed for 4.3.

Our unit tests and code are in much better shape to support PHP7 (which just went to beta this week). There are some errors when running the tests that we can focus on fixing in 4.4 as we work to have better PHP7 support in time for it's official release.

If there are any specific regressions, please create a new ticket with as much detail as possible.

#32 @jorbin
2 years ago

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

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


2 years ago

#34 @ocean90
2 years ago

Do we know the cause for the remaining failures related to nav menus?
https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/72667233#L353-L414 is related to this line trunk/src/wp-includes/nav-menu.php@33256#L641. Is it https://wiki.php.net/rfc/uniform_variable_syntax?

Edit:
The answer is yes, see the second example from the RFC.

                        // old meaning            // new meaning
$foo->$bar['baz']       $foo->{$bar['baz']}       ($foo->$bar)['baz']
Last edited 2 years ago by ocean90 (previous) (diff)

#35 @ocean90
2 years ago

31982.6.diff fixes the variable issue and tests are passing: https://travis-ci.org/ocean90/develop.wordpress/builds/72671062 \o/

@jorbin What do you think?

#36 @jorbin
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I hadn't looked into it much, but that is the uniform variable syntax backwards incompatable change biting us. More info: https://wiki.php.net/rfc/uniform_variable_syntax#semantic_differences_in_existing_syntax. I'm going to commit the change so that we get fully green in php7.

#37 @jorbin
2 years ago

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

In 33427:

Use explicit variable variable syntax

PHP7 introduces a backwards compatable change to variable varibale syntax that requires us to use curly brackets to maintain the syntax between php5 and php7. For more info, see https://wiki.php.net/rfc/uniform_variable_syntax#semantic_differences_in_existing_syntax for the semantic differences.

Props ocean90
Fixes #31982

#38 @netweb
2 years ago

Awesome

#39 @ocean90
19 months ago

In 36599:

i18n-tools: Remove PHP4 constructor from add-textdomain.php.

_deprecated_constructor() isn't available in non-WordPress context.

See #31982.

Note: See TracTickets for help on using tickets.