Fix known compatibility issues with PHP 7.4 #225

bahiirwa · 2020-02-25T06:41:14Z

All Submissions:

Have you followed the Classic Commerce Contributing guideline?
Does your code follow the repo standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Fix curly brackets fatal error [and other notices/warnings/errors] in PHP 7.4

PHP used to allow both square brackets and curly braces to be used interchangeably for accessing array elements and string offsets. The curly bracket syntax is only allowed in a limited set of cases and can be confusing for people not used to it.

PHP 7.4 will deprecate the curly brace syntax for accessing array elements and string offsets and it is expected that support will be completely removed in PHP 8.0.
Ref: https://wiki.php.net/rfc/deprecate_curly_braces_array_access

See https://core.trac.wordpress.org/ticket/47751.

Closes #194.

How to test the changes in this Pull Request:

Use a server with PHP 7.4
Turn on WP_DEBUG to true
Install CC (No errors, plugin installs well.)

Screenshot - before:

Screenshot - after:

codecov-io · 2020-02-25T06:54:18Z

Codecov Report

Merging #225 into master will increase coverage by <.01%.
The diff coverage is 80%.

@@             Coverage Diff              @@
##             master     #225      +/-   ##
============================================
+ Coverage     40.82%   40.83%   +<.01%     
+ Complexity    13447    13445       -2     
============================================
  Files           367      367              
  Lines         51277    51275       -2     
============================================
  Hits          20936    20936              
+ Misses        30341    30339       -2

Impacted Files	Coverage Δ	Complexity Δ
includes/class-wc-download-handler.php	`0% <ø> (ø)`	`102 <0> (-3)`	⬇️
includes/class-woocommerce.php	`10.36% <0%> (ø)`	`69 <0> (+1)`	⬆️
includes/wc-product-functions.php	`46.93% <100%> (ø)`	`0 <0> (ø)`	⬇️
includes/wc-formatting-functions.php	`75.55% <100%> (ø)`	`0 <0> (ø)`	⬇️
...udes/admin/settings/class-wc-settings-accounts.php	`94.4% <0%> (ø)`	`3% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 548ee48...cfd3164. Read the comment docs.

timbocode

Laurence, I know this is unlikely to cause any breaking changes but are you able to test the functionality of the wc_rgb_from_hex() function after changing the braces? The wiki (https://wiki.php.net/rfc/deprecate_curly_braces_array_access) is quite vague in terms of potential impact but does suggest that there are differences between the two.

Are there any other instances where { } are used instead of [ ]?

Would this script help https://gist.github.com/theodorejb/763b83a43522b0fc1755a537663b1863 ?

bahiirwa · 2020-02-25T07:45:24Z

Laurence, I know this is unlikely to cause any breaking changes but are you able to test the functionality of the wc_rgb_from_hex() function after changing the braces? The wiki (https://wiki.php.net/rfc/deprecate_curly_braces_array_access) is quite vague in terms of potential impact but does suggest that there are differences between the two.

I have done manual checks for the plugin as an ordinary user since this converts colors. I am open you to suggesting other ways of testing.

Are there any other instances where { } are used instead of [ ]?

I think this is an open door to #194 as a wider issue.

I see this a quick solution to the fatal error coming right now. We cannot guess what version a user will be seeing hosts are allowing PHP 7.4 to run on servers.

Would this script help https://gist.github.com/theodorejb/763b83a43522b0fc1755a537663b1863 ?

Bash is not my forte but I don't see any other breaking lines via the automated 7.4 travis build. This particular issue arises from []followed by {}. Again I see this PR as a small drop to #194 which intends to modernize the code.

timbocode · 2020-02-25T13:13:15Z

I have done manual checks for the plugin as an ordinary user since this converts colors.

That's fine. That's what I was looking for.

I think this is an open door to #194 as a wider issue.

This PR is dealing with a specific PHP7.4 issue as the title and description suggest. I agree it's part of #194 but that in itself is likely to get broken down into several PRs - like this one. So I think checks for other instances of { } would fit nicely in this PR.

Bash is not my forte but I don't see any other breaking lines via the automated 7.4 travis build.

It's actually a PHP script but if you're saying that Travis is already checking for this and not reporting any errors then we should already be covered.

Might also be worth trying to run that script on a backup copy of CC using PHP 7.4. Just for fun 🙂

Cherry-pick of woocommerce/woocommerce@7a7151bd44 to Classic Commerce.

This commit simply updates the PHP 7.4 version that is used in Travis now that PHP 7.4 has been officially released. Previously we were using RC versions to test the upcoming release. Cherry-pick of woocommerce/woocommerce@1ce283e to Classic Commerce. This commit also removes PHP 7.4 as an allowable failure on Travis.

…post object. Prevents Notices of Trying to access array offset on value of type bool because $src variables will be false. Cherry-pick of woocommerce/woocommerce@0d5c69712 to Classic Commerce.

Remove call to get_magic_quotes_runtime() as this function is deprecated as of PHP 7.4 (see https://wiki.php.net/rfc/deprecations_php_7_4). This part of the code was only needed for PHP <= 5.3 and this version is not supported anymore by WooCommerce. Cherry-pick of woocommerce/woocommerce@1f69fbcf3 to Classic Commerce.

bahiirwa · 2020-03-06T19:19:13Z

This PR still needs some checks. One error still raise when activating the plugin and setting up using the wizard.

nylen · 2020-03-06T19:19:50Z

@bahiirwa which error is that?

Note I pushed some commits, with the goal of addressing all PHP 7.4 changes in this PR. There aren't very many.

… PHP 7.4 Cherry-pick of woocommerce/woocommerce@d685964df to Classic Commerce.

bahiirwa · 2020-03-06T19:26:24Z

@nylen

Cherry-pick of woocommerce/woocommerce@1c47315b8 to Classic Commerce.

This allows us to check for PHP 7.4 compatibility even though ClassicPress hasn't released a fully compatible version yet.

bahiirwa · 2020-03-06T20:00:55Z

Thanks for the changes.

Tests: Now the setup is clean. No issues and the admin section does not scream with errors in 7.4 environment.

nylen · 2020-03-06T20:10:04Z

There's no difference between $array[0] and $array{0} except that the latter is now deprecated. From the RFC:

PHP allows both square brackets and curly braces to be used interchangeably for accessing array elements and string offsets.

I do not see any other instances of accessing an array like this:

git ls-files | grep '\.php$' | xargs -d'\n' grep -P '[a-z0-9_]{' --color=always | less -S

I looked for further PHP 7.4 compatibility commits using the following commands (your remote name may be different than woo):

git fetch woo
git log woo/master --grep '7\.4'

The error reported by @bahiirwa above did not appear in this list, but I found a relevant commit here: woocommerce/woocommerce@1c47315b8

Since there was at least 1 PHP notice-related commit where the Woo team did not write 7.4 in the commit message, it might also be a good idea to check through some of the following commits:

# notice: 141 matches (mostly unrelated)
git log LAST_WOO_COMMIT...woo/master --grep 'notice' -i
# warning: 31 matches
git log LAST_WOO_COMMIT...woo/master --grep 'warning' -i
# error: 133 matches (probably mostly unrelated)
git log LAST_WOO_COMMIT...woo/master --grep 'error' -i

LAST_WOO_COMMIT is a tag I just added to the main repository to mark the fork point from Woo, so you may need to run git fetch upstream --tags in order to be able to use it.

nylen · 2020-03-06T20:10:48Z

Short version of the above comment: I think this is ready for beta1 but there may still be more to be done regarding full PHP 7.4 compatibility.

timbocode

This is great work y'all. Just a couple of comments:

@bahiirwa could you please take a quick look at the commits @nylen mentions in Fix known compatibility issues with PHP 7.4 #225 (comment). As James says, there may be nothing here, but while we're looking for 7.4 compat issues, it would be good to get that bit boxed off.
Does commit e7d0541 do anything? It seems only to apply to file includes/tracks/events/class-wc-settings-tracking.php which does not exist in CC

Thanks again both. Good work.

nylen · 2020-03-08T23:05:03Z

@bahiirwa could you please take a quick look at the commits @nylen mentions in #225 (comment)

Not sure what you had in mind but I think this could probably wait for a separate issue/PR. Even though nothing else jumped out at me as "definitely a PHP 7.4 issue", there is potentially quite a bit to look through, and so it might be good to organize that a little better.

Does commit e7d0541 do anything? It seems only to apply to file includes/tracks/events/class-wc-settings-tracking.php which does not exist in CC

I did git cherry-pick and the result was an empty commit because, as you say, we removed this file. I included the commit just as a way of saying "yes we looked at this commit". This may or may not matter depending on where we end up with #226.

timbocode · 2020-03-09T10:55:25Z

OK. Title changed to "Fix known compatibility issues with PHP 7.4"
OK, that's fine.

bahiirwa · 2020-03-10T09:39:37Z

I think with James' comments, this is green to go if you agree with the changes as well.

Tests:

Travis still passes.
Manually, I have tested the plugin and fresh installations work, all the basic functions work well.

timbocode

All seems good.

Fix curly brackets for PHP 7.4

887685f

bahiirwa mentioned this pull request Feb 25, 2020

PHP 7.4 compatibility #194

Closed

bahiirwa mentioned this pull request Feb 25, 2020

Fatal Error: PHP 7.4 Curly brackets deprecated. Plugin doesn't Install #223

Closed

timbocode reviewed Feb 25, 2020

View reviewed changes

superdav42 and others added 3 commits March 6, 2020 19:07

Use brackets s instead of braces to avoid deprecation notice in PHP 7.4

a0f68c8

Cherry-pick of woocommerce/woocommerce@7a7151bd44 to Classic Commerce.

Check that $attachment is really an attachment and not the global $…

d613954

…post object. Prevents Notices of Trying to access array offset on value of type bool because $src variables will be false. Cherry-pick of woocommerce/woocommerce@0d5c69712 to Classic Commerce.

nylen changed the title ~~Fix Fatal error for curly brackets in PHP 7.4~~ Fix compatibility issues with PHP 7.4 Mar 6, 2020

superdav42 and others added 2 commits March 6, 2020 19:20

Use correct parameter order for implode. Solves deprecation notice in…

e7d0541

… PHP 7.4 Cherry-pick of woocommerce/woocommerce@d685964df to Classic Commerce.

Update comment in .travis.yml

2e77f4c

superdav42 and others added 3 commits March 6, 2020 19:45

Fix trying to access array offset on a boolean notice

2a346e8

Cherry-pick of woocommerce/woocommerce@1c47315b8 to Classic Commerce.

Run the tests against a development build of ClassicPress

80d667f

This allows us to check for PHP 7.4 compatibility even though ClassicPress hasn't released a fully compatible version yet.

Bugfix

cfd3164

bahiirwa requested a review from timbocode March 6, 2020 20:02

timbocode reviewed Mar 7, 2020

View reviewed changes

timbocode added this to the 1.0.0-beta1 milestone Mar 8, 2020

timbocode changed the title ~~Fix compatibility issues with PHP 7.4~~ Fix known compatibility issues with PHP 7.4 Mar 9, 2020

timbocode approved these changes Mar 10, 2020

View reviewed changes

timbocode merged commit 6ce6829 into ClassicPress:master Mar 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix known compatibility issues with PHP 7.4 #225

Fix known compatibility issues with PHP 7.4 #225

bahiirwa commented Feb 25, 2020 •

edited by nylen

Loading

codecov-io commented Feb 25, 2020 •

edited

Loading

timbocode left a comment

bahiirwa commented Feb 25, 2020 •

edited

Loading

timbocode commented Feb 25, 2020

bahiirwa commented Mar 6, 2020

nylen commented Mar 6, 2020

bahiirwa commented Mar 6, 2020

bahiirwa commented Mar 6, 2020

nylen commented Mar 6, 2020

nylen commented Mar 6, 2020

timbocode left a comment

nylen commented Mar 8, 2020 •

edited

Loading

timbocode commented Mar 9, 2020

bahiirwa commented Mar 10, 2020

timbocode left a comment

Fix known compatibility issues with PHP 7.4 #225

Fix known compatibility issues with PHP 7.4 #225

Conversation

bahiirwa commented Feb 25, 2020 • edited by nylen Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Screenshot - before:

Screenshot - after:

codecov-io commented Feb 25, 2020 • edited Loading

Codecov Report

timbocode left a comment

Choose a reason for hiding this comment

bahiirwa commented Feb 25, 2020 • edited Loading

timbocode commented Feb 25, 2020

bahiirwa commented Mar 6, 2020

nylen commented Mar 6, 2020

bahiirwa commented Mar 6, 2020

bahiirwa commented Mar 6, 2020

nylen commented Mar 6, 2020

nylen commented Mar 6, 2020

timbocode left a comment

Choose a reason for hiding this comment

nylen commented Mar 8, 2020 • edited Loading

timbocode commented Mar 9, 2020

bahiirwa commented Mar 10, 2020

timbocode left a comment

Choose a reason for hiding this comment

bahiirwa commented Feb 25, 2020 •

edited by nylen

Loading

codecov-io commented Feb 25, 2020 •

edited

Loading

bahiirwa commented Feb 25, 2020 •

edited

Loading

nylen commented Mar 8, 2020 •

edited

Loading