Page MenuHomePhabricator

MagicWordArray::parseMatch() uses each() function deprecated in PHP 7.2
Closed, ResolvedPublic

Description

I am using git branch REL1_29. After switching to PHP 7.2beta3, I saw the following error on most pages:

Deprecated: The each() function is deprecated. This message will be suppressed on further calls in /home/ibwiki/w/includes/MagicWordArray.php on line 197

I have the following settings:

error_reporting( E_ALL );
ini_set( 'display_errors', 1 );

The code in question starts:

public function parseMatch( $m ) {
        reset( $m );
        while ( list( $key, $value ) = @each( $m ) ) {

I removed the reset and changed the while/each to a foreach as so:

public function parseMatch( $m ) {
        foreach ( $m as $key => $value ) {

This appears to work [update: it doesn't] (and in theory foreach should be faster/easier to optimize), however I don't know if any other code in this function or elsewhere relies on the differences between each() and foreach(), such as the side-effect of array iteration or the ability to edit the array, rather than work on a copy of the array.

--- a/includes/MagicWordArray.php
+++ b/includes/MagicWordArray.php
@@ -193,8 +193,7 @@ class MagicWordArray {
         * @return array
         */
        public function parseMatch( $m ) {
-               reset( $m );
-               while ( list( $key, $value ) = each( $m ) ) {
+               foreach ( $m as $key => $value ) {
                        if ( $key === 0 || $value === '' ) {
                                continue;
                        }

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 28 2017, 4:07 PM
GreenReaper updated the task description. (Show Details)Aug 28 2017, 4:09 PM
Reedy added a subscriber: Reedy.Aug 28 2017, 4:12 PM

Do unit tests etc still pass?

Feel free to submit a patch on gerrit and find out :)

Reedy added a comment.Aug 28 2017, 4:30 PM

A quick google suggests it should be the same! https://stackoverflow.com/questions/3304885/whilelistkey-value-eacharray-vs-foreacharray-as-key-value

Also, it removes an assignment in conditional too, so yay

Change 374348 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Swap while list and each for foreach loop

https://gerrit.wikimedia.org/r/374348

I've not used that feature before. I tried running it via the 'tap' method.
Many ParserIntegrationTest tests do not appear to pass with the above change, while they do with the original. So it's possibly not a great patch...

I'm kind of in the middle of a server migration so I can't spend a huge amount of time on this right now, but I thought I should report it. >_>

Incidentally, the deprecation warning is also present for line 38 of vendor/phpunit/phpunit/src/Util/Getopt.php

Reedy added a comment.Aug 28 2017, 4:37 PM

I'm kind of in the middle of a server migration so I can't spend a huge amount of time on this right now, but I thought I should report it. >_>

But why use a beta version of PHP 7.2? lol

Incidentally, the deprecation warning is also present for line 38 of vendor/phpunit/phpunit/src/Util/Getopt.php

That one counts as Upstream, but a look at master suggests they've replicated it already https://github.com/sebastianbergmann/phpunit/blob/master/src/Util/Getopt.php#L40 vs what they had in 3.6 https://github.com/sebastianbergmann/phpunit/blob/3.6/PHPUnit/Util/Getopt.php#L81

Mostly because PHP 7.2 is more efficient, and I don't want to do another PHP version migration in a few months!
I have several days to do it, which is why I'm reporting it at all and didn't just put an @ on for now. ;-)

GreenReaper updated the task description. (Show Details)Aug 28 2017, 4:45 PM
Reedy added a comment.Aug 28 2017, 4:47 PM

Mostly because PHP 7.2 is more efficient, and I don't want to do another PHP version migration in a few months!
I have several days to do it, which is why I'm reporting it at all and didn't just put an @ on for now. ;-)

Surely you're gonna have to migrate soon to the next beta/release versions?

I'm not sure how that's gonna be much different to just using 7.1 and upgrading to 7.2 later...

GreenReaper added a comment.EditedAug 28 2017, 4:50 PM

Surely you're gonna have to migrate soon to the next beta/release versions?
I'm not sure how that's gonna be much different to just using 7.1 and upgrading to 7.2 later...

Yes, but the point is to find issues like this (which I incidentally wouldn't have noticed if I went to 7.1) once rather than twice. 7.2 is feature-frozen and the next in a couple of days will be RC1.
I'm upgrading from 5.6 so I figured I'd try to get them all out of the way at once. Debian stretch came with 7.0, so I was already doing the work to use another repo.

Change 374348 abandoned by Reedy:
Swap while list and each for for loop

https://gerrit.wikimedia.org/r/374348

Change 379047 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Replace uses of each()

https://gerrit.wikimedia.org/r/379047

Change 379048 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/extensions/Scribunto@master] Replace uses of each()

https://gerrit.wikimedia.org/r/379048

Change 379048 merged by jenkins-bot:
[mediawiki/extensions/Scribunto@master] Replace uses of each()

https://gerrit.wikimedia.org/r/379048

Change 379047 merged by jenkins-bot:
[mediawiki/core@master] Replace uses of each()

https://gerrit.wikimedia.org/r/379047

Anomie closed this task as Resolved.Sep 20 2017, 5:19 PM
Anomie claimed this task.
Anomie added a subscriber: Anomie.

Core and WMF-deployed extensions are updated.

Someone might file separate tasks for non-WMF-deployed extensions and skins. Currently those are:

  • BlueSpiceExtensions
  • BlueSpiceFoundation
  • Foxway
  • FundraisingEmailUnsubscribe
  • LiveTranslate
  • MediaWikiAuth
  • Mpdf
  • PhpTags
  • PhpTagsFunctions
  • ReaderFeedback
  • Reflect
  • SecureSessions
  • Widgets

Change 401045 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/tools/codesniffer@master] Warn on usage of each()

https://gerrit.wikimedia.org/r/401045

Change 401045 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Warn on usage of each()

https://gerrit.wikimedia.org/r/401045