Skip to content
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

ETK: make PHP linting PHP 8 compatible #71062

Merged
merged 2 commits into from
Dec 12, 2022
Merged

ETK: make PHP linting PHP 8 compatible #71062

merged 2 commits into from
Dec 12, 2022

Conversation

simison
Copy link
Member

@simison simison commented Dec 12, 2022

Proposed Changes

  • Add config to phpcs.xml suppressing PHP run-time notices (E_DEPRECATED) which allows running the linter with PHP8.

The issue is fixed in WordPress-Coding-Standards develop branch but hasn't been released yet. Their latest release 2.3.0 is from May 2020. The slution was suggested in a comment WordPress/WordPress-Coding-Standards#2035 (comment)

Testing Instructions

  • Open apps/editing-toolkit
  • In PHP8, run yarn lint:php

Before the change, you see errors like:

FILE: editing-toolkit/editing-toolkit-plugin/newspack-blocks/synced-newspack-blocks/class-newspack-blocks-api.php
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 1 | ERROR | An error occurred during processing; checking has been aborted. The error message was: trim(): Passing null to parameter #1 ($string) of type string is deprecated in
   |       | /wp-calypso/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php on line 280
   |       | (Internal.Exception)
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

After the change you see only regular linting errors/warnings.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

Related to #

@simison simison added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 12, 2022
@simison simison requested review from a team December 12, 2022 12:29
@github-actions
Copy link

github-actions bot commented Dec 12, 2022

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Prevent errors caused by WordPress Coding Standards not supporting PHP 8.0+.
See https://github.com/WordPress/WordPress-Coding-Standards/issues/2035
-->
<ini name="error_reporting" value="E_ALL &#38; ~E_DEPRECATED" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting this will disable all deprecated syntax and not just the syntax that was deprecated in PHP 8. Is that intentional? If yes, then we might want to be explicit about it in the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to ideas how to limit this just to specific errors or even just when running in PHP, but keep runtime errors for other versions, or even just for CI runs.

I'll update the comment tho!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a better way to do it TBH. I'm fine with this temporary approach, as long as we're explicit about it in the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check the new one out, 3980d3a

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you 🙌

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go once comment is updated 🚀

@simison simison merged commit b17ee1e into trunk Dec 12, 2022
@simison simison deleted the update/etk-php8-lint branch December 12, 2022 14:55
@simison simison added the Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin label Dec 12, 2022
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 12, 2022
micahmills added a commit to micahmills/disciple-tools-theme that referenced this pull request Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants