Page MenuHomePhabricator

Possible PHP7 compatibility issues in WMF-deployed extensions
Closed, ResolvedPublic

Description

Similar to T173848, output from From https://github.com/sstalle/php7cc

Done for extensions in 1.31.0-wmf.2

File: /home/reedy/git/mediawiki/core/extensions/EventLogging/includes/JsonSchema.php
> Line 63: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();


File: /home/reedy/git/mediawiki/core/extensions/EventLogging/includes/ApiJsonSchema.php
> Line 133: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($v as &$properties) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Flow/includes/Data/Storage/RevisionStorage.php
> Line 324: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($source as &$row) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Flow/includes/Repository/TreeRepository.php
> Line 365: [Warning] Possible array element creation during by-reference assignment
    $identityMap[$parent->getAlphadecimal()]['children'][$child] =& $identityMap[$child];


File: /home/reedy/git/mediawiki/core/extensions/Gadgets/api/ApiQueryGadgets.php
> Line 178: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($data as $key => &$value) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Cognate/src/StringHasher.php
> Line 61: [Error] Bitwise shift by 32 bits
    hexdec($hexhi) << 32;


File: /home/reedy/git/mediawiki/core/extensions/DonationInterface/gateway_common/GatewayPage.php
> Line 578: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($fieldRules as &$rule) {
    }


File: /home/reedy/git/mediawiki/core/extensions/DonationInterface/gateway_common/gateway.adapter.php
> Line 3312: [Warning] Indirect variable, property or method access
    ${$var[$key]};
> Line 3316: [Warning] Indirect variable, property or method access
    ${$var[$subkey]};
> Line 3317: [Warning] Indirect variable, property or method access
    ${$var[$subkey][$subvalue]};


File: /home/reedy/git/mediawiki/core/extensions/Kartographer/includes/DataModuleLinks.php
> Line 44: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($service->links as &$link) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikibase/lib/includes/Store/GenericEntityInfoBuilder.php
> Line 158: [Warning] Possible array element creation during by-reference assignment
    $this->entityInfo[$idString] =& $this->entityInfo[$targetKey];


File: /home/reedy/git/mediawiki/core/extensions/Wikibase/lib/includes/Serialization/CallbackFactory.php
> Line 55: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($snakGroup as &$snak) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikibase/repo/tests/phpunit/includes/Api/SetSiteLinkTest.php
> Line 398: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($site['badges'] as &$dummy) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikibase/repo/tests/phpunit/includes/Api/GetClaimsTest.php
> Line 170: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($claimsByProperty as &$claimArray) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikibase/repo/tests/phpunit/includes/Api/EntityTestHelper.php
> Line 428: [Warning] Possible adding to array on the last iteration of a by-reference foreach loop
    $data[$newKey] = $value;


File: /home/reedy/git/mediawiki/core/extensions/Wikibase/repo/tests/phpunit/includes/Api/SetReferenceTest.php
> Line 214: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($propertyGroup as &$snak) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikibase/repo/tests/phpunit/includes/Parsers/MediaWikiNumberUnlocalizerTest.php
> Line 158: [Warning] String containing number in hexadecimal notation
    '0x20';


File: /home/reedy/git/mediawiki/core/extensions/Scribunto/tests/phpunit/engines/LuaCommon/UstringLibraryTest.php
> Line 56: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($chars as &$c) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/vendor/wikibase/data-model-services/tests/unit/DataValue/ValuesFinderTest.php
> Line 146: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/vendor/wikimedia/purtle/src/RdfWriterBase.php
> Line 206: [Warning] Possible object property creation during by-reference assignment
    $writer->prefixes =& $this->prefixes;


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/vendor/data-values/common/tests/ValueParsers/FloatParserTest.php
> Line 80: [Warning] String containing number in hexadecimal notation
    '0x20';


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/vendor/data-values/common/tests/ValueParsers/IntParserTest.php
> Line 69: [Warning] String containing number in hexadecimal notation
    '0x20';


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/vendor/data-values/data-values/tests/phpunit/NumberValueTest.php
> Line 52: [Warning] String containing number in hexadecimal notation
    '0x20';


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/extensions/Wikibase/lib/includes/Store/GenericEntityInfoBuilder.php
> Line 158: [Warning] Possible array element creation during by-reference assignment
    $this->entityInfo[$idString] =& $this->entityInfo[$targetKey];


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/extensions/Wikibase/lib/includes/Serialization/CallbackFactory.php
> Line 55: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($snakGroup as &$snak) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/extensions/Wikibase/repo/tests/phpunit/includes/Api/SetSiteLinkTest.php
> Line 398: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($site['badges'] as &$dummy) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/extensions/Wikibase/repo/tests/phpunit/includes/Api/GetClaimsTest.php
> Line 170: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($claimsByProperty as &$claimArray) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/extensions/Wikibase/repo/tests/phpunit/includes/Api/EntityTestHelper.php
> Line 428: [Warning] Possible adding to array on the last iteration of a by-reference foreach loop
    $data[$newKey] = $value;


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/extensions/Wikibase/repo/tests/phpunit/includes/Api/SetReferenceTest.php
> Line 214: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($propertyGroup as &$snak) {
    }


File: /home/reedy/git/mediawiki/core/extensions/Wikidata/extensions/Wikibase/repo/tests/phpunit/includes/Parsers/MediaWikiNumberUnlocalizerTest.php
> Line 158: [Warning] String containing number in hexadecimal notation
    '0x20';


File: /home/reedy/git/mediawiki/core/extensions/CirrusSearch/includes/SearchConfig.php
> Line 214: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();


File: /home/reedy/git/mediawiki/core/extensions/CentralNotice/tests/CentralNoticeTestFixtures.php
> Line 108: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($campaign['banners'] as &$banner) {
    }
> Line 143: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($context_and_output['choices'] as &$choice) {
    }
> Line 192: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($campaign['banners'] as &$banner) {
    }


File: /home/reedy/git/mediawiki/core/extensions/CentralNotice/includes/AllocationCalculator.php
> Line 133: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($campaignsAtThisPriority as &$campaign) {
    }
> Line 166: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($campaignsAtThisPriority as $i => &$campaign) {
    }

Event Timeline

Reedy created this task.Aug 22 2017, 5:05 PM
Reedy updated the task description. (Show Details)
Reedy moved this task from Unsorted to PHP 7 on the [DO NOT USE] NewPHP board.
Reedy updated the task description. (Show Details)Aug 22 2017, 6:11 PM
Anomie added a subscriber: Anomie.Aug 22 2017, 6:56 PM

I'm not impressed with the false positive rate for this tool. The warnings in OAuth, CentralAuth, and Scribunto all appear to be bogus.

Thanks for running this check. Flow looks good.

File: /home/reedy/git/mediawiki/core/extensions/Flow/includes/Data/Storage/RevisionStorage.php
> Line 324: [Warning] Nested by-reference foreach loop, make sure that array modifications (if any) do what you expect
    foreach ($source as &$row) {
    }

I reviewed php7_foreach and the above looks fine.

It is not doing anything edge-casey on either of the arrays being iterated ($cacheResult and $source) (no additions or removals to these arrays, no replacements of the whole array).

File: /home/reedy/git/mediawiki/core/extensions/Flow/includes/Import/LiquidThreadsApi/Source.php
> Line 195: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

Turns out they're not.

File: /home/reedy/git/mediawiki/core/extensions/Flow/includes/Repository/TreeRepository.php
> Line 365: [Warning] Possible array element creation during by-reference assignment
    $identityMap[$parent->getAlphadecimal()]['children'][$child] =& $identityMap[$child];

This line is a bit tricky.

However, $identityMap[$child] is certainly created before this line, so there's no way two new keys of $identityMap itself are created in this line (and thus no ordering issue between these two).

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptAug 29 2017, 7:17 PM
Reedy added a comment.Aug 29 2017, 7:21 PM

func_get_args is very false positive/naive check.

Upstream issue filed, with various examples provided at https://github.com/sstalle/php7cc/issues/127

Change 374816 had a related patch set uploaded (by Ottomata; owner: Ottomata):
[mediawiki/extensions/EventLogging@master] Fix PHP7 warnings

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

I fixed the two warnings from the EventLogging extension, but I have not been able to test them. If anyone knows how, I would be much obliged.

https://gerrit.wikimedia.org/r/#/c/374816/

Krinkle removed a subscriber: Krinkle.Aug 30 2017, 3:38 PM

Regarding Cognate:

File: /home/reedy/git/mediawiki/core/extensions/Cognate/src/CognateHooks.php
> Line 25: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();
> Line 43: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();
> Line 54: [Warning] Function argument(s) returned by "func_get_args" might have been modified
    func_get_args();

These all look like false positives as long as I understand what this is trying to hint at correctly.

File: /home/reedy/git/mediawiki/core/extensions/Cognate/src/StringHasher.php
> Line 61: [Error] Bitwise shift by 32 bits
    hexdec($hexhi) \ 32;

Doesn't look like an issue?
https://3v4l.org/54iU5
Maybe it would be an issue on 32 bit systems?
Cognate will already refuse to run on 32 bit systems:
https://github.com/wikimedia/mediawiki-extensions-Cognate/blob/d5c97af4ece85304f27cea60a89d9d59d664e0b0/src/StringHasher.php#L28

Change 374816 abandoned by Ottomata:
Fix PHP7 warnings

Reason:
Ha! Psshhh ok then, abandoning! :)

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

Ottomata moved this task from In Code Review to Done on the Analytics-Kanban board.Sep 5 2017, 3:04 PM
Nuria edited projects, added Analytics; removed Analytics-Kanban.Sep 11 2017, 3:49 AM
Nuria moved this task from Incoming to Radar on the Analytics board.Sep 14 2017, 6:16 PM
Reedy added a comment.Sep 18 2017, 2:06 PM

I'm not impressed with the false positive rate for this tool. The warnings in OAuth, CentralAuth, and Scribunto all appear to be bogus.

https://github.com/sstalle/php7cc/issues/127#issuecomment-330169791

https://github.com/sstalle/php7cc/commit/7bd1d16d40941070359e6f705a6fa00c801576ed

I think they fixed most of the false positives now. Could someone re run the linter and update the description please?

Krinkle removed a project: HHVM.Oct 9 2017, 10:32 PM
Reedy updated the task description. (Show Details)Oct 9 2017, 11:07 PM

Looks like we've gone down from 30 plus func_get_args, to 3...

Change 383289 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Gadgets@master] Remove nested foreach references

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

Change 383289 merged by jenkins-bot:
[mediawiki/extensions/Gadgets@master] Remove nested foreach references

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

Smalyshev removed a project: Discovery.
Smalyshev added a subscriber: Smalyshev.

CirrusSearch one seems to be false positive.

Reedy added a comment.Oct 11 2017, 5:40 PM

Many are likely to be false positives, and that's not too much of an issue as long as people have looked over them and deemed them as such :)

Krinkle renamed this task from Possible WMF deployed extension PHP 7 issues to Possible PHP7 compatibility issues in WMF-deployed extensions.Feb 9 2018, 12:09 AM
Krinkle closed this task as Resolved.Apr 18 2018, 7:34 PM