Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Prepare Woo Blocks for PHP8 Compatibility #3220

Closed
nerrad opened this issue Sep 30, 2020 · 8 comments
Closed

Prepare Woo Blocks for PHP8 Compatibility #3220

nerrad opened this issue Sep 30, 2020 · 8 comments
Assignees
Labels
tools Used for work on build or release tools. type: cooldown Things that are queued for a cooldown period (assists with planning). type: enhancement The issue is a request for an enhancement. type: epic A label used by Zenhub for indicating issues functioning as an Epic.
Milestone

Comments

@nerrad
Copy link
Contributor

nerrad commented Sep 30, 2020

PHP8 is on the horizon and there are multiple efforts happening across the WordPress and WooCommerce projects to ensure compatibility with it. Our PHP surface area is fairly small but nevertheless we should make sure we're tracking these efforts and that there is nothing in our code that is incompatible with PHP8. A brief itemized list of todos (not exhaustive):

  • Verify codebase is compatible.
  • Configure PHPUnit for testing against PHP8 in travis builds (*Note, this will require taking a look at WP and Woo Core test suites are doing for this since we inherit those via docker).
  • Are there any changes needed to our environment setups?
@nerrad nerrad added tools Used for work on build or release tools. type: cooldown Things that are queued for a cooldown period (assists with planning). type: enhancement The issue is a request for an enhancement. labels Sep 30, 2020
@nerrad
Copy link
Contributor Author

nerrad commented Oct 5, 2020

See main Woo Core issue for PHP8 support in Woo Core: woocommerce/woocommerce#27674

@Aljullu Aljullu changed the title Prepare Woo Blocks for PHP8 Compatability. Prepare Woo Blocks for PHP8 Compatibility Oct 5, 2020
@nerrad
Copy link
Contributor Author

nerrad commented Oct 22, 2020

Converting this to an epic so we can link all issues uncovered to this.

@nerrad nerrad added the type: epic A label used by Zenhub for indicating issues functioning as an Epic. label Oct 22, 2020
@nerrad
Copy link
Contributor Author

nerrad commented Nov 1, 2020

I ran exakat against WooCommerce Blocks and there were no compatibility issues reported via the analyser.

@haszari
Copy link
Member

haszari commented Nov 6, 2020

😬 I re-ran the analysis and found issues! Maybe new issues merged to trunk since @nerrad analysis?

Screen Shot 2020-11-06 at 3 00 08 PM

PHP8 Mismatch parameter name

PHP8 Unsupported Types With Operators

Screen Shot 2020-11-06 at 3 01 06 PM

PHP7.4 Detect current class

Can't find the specific files in the Issues view for some reason.

We do use __CLASS__ in src/Library.php and src/Assets.php.

AHA I was looking at the wrong page in the report, it is __CLASS__.

Screen Shot 2020-11-06 at 3 26 36 PM

PHP7.4 array_key_exists() Works On Arrays

@nerrad
Copy link
Contributor Author

nerrad commented Nov 6, 2020

🤔 I'm puzzled why the php8 compat issues didn't show up for me initially, but I can confirm they show up now (maybe I was mixed up and viewing the results from another repo (which is likely because the only other project I have checked out currently is Product Addons and that has a clean bill for PHP 8). Thanks for catching this Rua.

PHP8 Mismatch parameter name

This is only potentially issue if we used named arguments. Since that syntax is only available for PHP 8 and requiring PHP 8 as a minimum version is a long ways off, I don't see this as a high priority/necessary thing to fix (a lot can change between now and then). It doesn't cause php warnings or errors, it's mostly a preventative measure so someone using named arguments syntax has no ambiguity around what arguments to use.

PHP8 Unsupported Types With Operators

Ya all of these should be addressed. However, this may be a bit of a false positive (especially for ! $value derivatives) because I'm not getting TypeErrors for those implementations. Still, given how PHP is continuing to become more strict with each release, I definitely think we need to update these to be typesafe.

PHP7.4 Detect current class

These are all false positives. __CLASS__ was set to be deprecated in PHP 7.4 and removed in PHP 8, but that was removed

PHP7.4 array_key_exists() Works On Arrays

This is a false positive. $this->data is an array.

@nerrad nerrad self-assigned this Nov 6, 2020
@haszari
Copy link
Member

haszari commented Nov 9, 2020

Thanks for investigating those @nerrad . Looks like there are common patterns for some of these across our code bases, so we should aim to consistently address (or ignore as appropriate) these across the different repos.

For example, we're also getting Detect Current Class in Product Add-Ons. Is it possible to switch these to a more recent syntax to make Exakat happy - e.g. self::class? Related – if we do switch the syntax, does that have implications for our minimum supported PHP version (if so, maybe it's better to stick with an "old" syntax).

It would be great if we can treat these all as valid warnings and work towards a clean Exakat pass in all repos, though I'm not sure if that's practical/possible - keen to hear your thoughts on that as a goal.

@nerrad
Copy link
Contributor Author

nerrad commented Nov 9, 2020

It would be great if we can treat these all as valid warnings

They aren't valid though, I consider it a bug with the sniff because __CLASS__ is not deprecated. Remember PHP 8 hasn't been released yet and the sniffs are still being improved for both Exakat and PHPCS Compatibility (and PHP 7.4 sniffs included with that apparently). I'm not concerned with getting all green checks for false positives.

@nerrad
Copy link
Contributor Author

nerrad commented Nov 9, 2020

I've been testing with WooCommerce Blocks on PHP 8 for a couple weeks now including various types of purchases and blocks. No more critical errors have popped up for me so I'm happy to consider Blocks "PHP 8 ready" in terms of no critical errors present.

@nerrad nerrad closed this as completed Nov 9, 2020
@nerrad nerrad added this to the 3.9.0 milestone Nov 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tools Used for work on build or release tools. type: cooldown Things that are queued for a cooldown period (assists with planning). type: enhancement The issue is a request for an enhancement. type: epic A label used by Zenhub for indicating issues functioning as an Epic.
Projects
None yet
Development

No branches or pull requests

2 participants