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

Fix PHP 5.2 notice by ensuring `$memo` is always an array #7120

Merged
merged 4 commits into from Jun 5, 2018

Conversation

Projects
None yet
2 participants
@danielbachhuber
Member

danielbachhuber commented Jun 4, 2018

array_reduce() only permits $initial to be a mixed variable as of PHP 5.3

Fixes #4689

danielbachhuber added some commits Jun 4, 2018

Fix PHP 5.2 notice by ensuring `$memo` is always an array
`array_reduce()` only permits `$initial` to be a mixed variable as of PHP 5.3

@danielbachhuber danielbachhuber added this to the 3.1 milestone Jun 4, 2018

@danielbachhuber danielbachhuber requested a review from WordPress/gutenberg-core Jun 4, 2018

@aduth

This comment has been minimized.

Member

aduth commented Jun 4, 2018

In what cases are we calling gutenberg_preload_api_request without the $memo value being an array?

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 4, 2018

@aduth:

array_reduce() only permits $initial to be a mixed variable as of PHP 5.3

See http://php.net/manual/en/function.array-reduce.php

@aduth

This comment has been minimized.

Member

aduth commented Jun 4, 2018

But that's not what I've asked. Specifically, where are we passing a non-array value to gutenberg_preload_api_request?

Edit: To expand on this, from what I can discern we should only be passing an array, and if we're not, that itself is a bug which ought to be addressed, not just being resilient from within the reduce callback to a non-array value (treating symptom, not cause).

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 4, 2018

But that's not what I've asked. Specifically, where are we passing a non-array value to gutenberg_preload_api_request?

$preload_data = array_reduce(
$preload_paths,
'gutenberg_preload_api_request',
array()
);

Edit: To expand on this, from what I can discern we should only be passing an array, and if we're not, that itself is a bug which ought to be addressed, not just being resilient from within the reduce callback to a non-array value (treating symptom, not cause).

gutenberg_preload_api_request() is designed to be called from array_reduce(). We can change its function signature, surely, but that's a different type of change. Ultimately, the bug is within array_reduce(). For the purposes of solving the immediate problem, I thought it appropriate to simply include a PHP 5.2 compatibility shim.

@aduth

This comment has been minimized.

Member

aduth commented Jun 5, 2018

I misinterpreted the issue to be that for some reason, non-array values were being passed as the initial value for the array_reduce, when in fact it's that we are passing an array, but this was not supported until 5.3.

It was a bit strange to me that the proposed fix would resolve the warning, and while I don't have immediate access to a 5.2.x environment, this sandbox site turned out to be useful for testing. I suppose prior to PHP 5.3 if an array was passed, it would be coerced to 0, but if the reduced return value was an array (i.e. by testing the non-array initial and converting it into one), it ends up being respected?

"Pre-Fix" "Post-Fix"
pre-fix post-fix

Above is all my thought process for following this. Not implying that I'm saying anything new. But do you think maybe we ought to leave a comment in here somewhere explaining why we're doing the ! is_array condition? It might not otherwise be obvious to someone reading. Also wondering why PHPCS can't catch this sort of issue by our configured PHP version support.

@aduth

aduth approved these changes Jun 5, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 5, 2018

I suppose prior to PHP 5.3 if an array was passed, it would be coerced to 0, but if the reduced return value was an array (i.e. by testing the non-array initial and converting it into one), it ends up being respected?

Correct.

Above is all my thought process for following this.

You're much more verbose and articulate than I 😄

But do you think maybe we ought to leave a comment in here somewhere explaining why we're doing the ! is_array condition?

Good suggestion. Updated in 4172d43

Also wondering why PHPCS can't catch this sort of issue by our configured PHP version support.

Doesn't seem like there's a sniff yet for it. I've filed a new issue: PHPCompatibility/PHPCompatibility#649

@danielbachhuber danielbachhuber merged commit f67ec84 into master Jun 5, 2018

2 checks passed

codecov/project 46.48% (+0.12%) compared to b763387
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the 4689-avoid-php-52-notice branch Jun 5, 2018

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