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

Exception thrown by I18nSniff in PHP 8.1 #136

Closed
jjgrainger opened this issue Apr 5, 2023 · 6 comments · Fixed by #163
Closed

Exception thrown by I18nSniff in PHP 8.1 #136

jjgrainger opened this issue Apr 5, 2023 · 6 comments · Fixed by #163
Assignees
Labels
QA Testing Issues identified from QA testing [Type] Bug An existing feature is broken

Comments

@jjgrainger
Copy link
Contributor

While testing in PHP 8.1 all warnings from the I18n_Usage_Check are replaced by the following Exception thrown. It looks as though the issue is to do with WPCS dependency rather than the plugin check itself but we may want to consider how this is handled?

image

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 /var/www/html/wp-content/plugins/plugin-check/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/WP/I18nSniff.php on line 194
@jjgrainger jjgrainger added [Type] Bug An existing feature is broken QA Testing Issues identified from QA testing labels Apr 5, 2023
@eclarke1 eclarke1 moved this to Backlog in Plugin Check Apr 25, 2023
@spacedmonkey
Copy link
Member

This seems like a P0 and should be fixed ASAP.

@jjgrainger jjgrainger moved this from Backlog to To Do in Plugin Check Apr 26, 2023
@mukeshpanchal27 mukeshpanchal27 moved this from To Do to In Progress in Plugin Check Apr 28, 2023
@mukeshpanchal27
Copy link
Member

mukeshpanchal27 commented May 10, 2023

@jjgrainger I check the issue in deeply and found that the issue is in wp-coding-standards/wpcs and it is not support in 8.1 and we can't override the composer package files or any have any quick fix for it. The wp-coding-standards/wpcs team is working in 3.0 that will solve this error. Mean time we can use any of below approach.

Approach 1: Update wp-coding-standards/wpcs version to develop branch or last commit in the composer.json.

Develop branch:

"require-dev": {
    "wp-coding-standards/wpcs": "dev-develop"
}

Last commit branch:

"require-dev": {
    "wp-coding-standards/wpcs": "dev-feature/abstractclassrestrictions-improvements"
}

Approach 2: We can add steps in our readme.txt/readme.md how to update wp-coding-standards/wpcs to development version if anyone want to use plugin in PHP 8.1.

Using Plugin check on PHP 8.1

If you plan on using this plugin on PHP 8.1, you will need to make some modifications to your Composer configuration.

Add the Composer repository to your project's composer.json file:

Switch to the develop Version

Add the following to your composer.json file autoload-dev section:

"require-dev": {
       "wp-coding-standards/wpcs": "dev-develop"
}

@felixarntz @joemcgill @spacedmonkey i did like to get you feedback on this so we can move forward.

@spacedmonkey
Copy link
Member

The related tickets. WordPress/WordPress-Coding-Standards#2070 & WordPress/WordPress-Coding-Standards#1877

Seems like their is plans or release plans for 3.0.0 which will have support for PHP 8.0+.

We have options.

  1. Remove all wpcs based checks, support PHP 8.0+
  2. Conditionally load wpcs on PHP 8.0-
  3. Load from dev-develop in composer
  4. Use composer-patches to patch this issue.

Load from dev-develop, I would recommend locking down to a commit like this. "sabberworm/php-css-parser": "dev-master#bfdd976", ( Example ).

Using composer patches might be a tidy solution, but might result in find other issues with PHP 8.0.

Using dev-develop feel like the only option, but I am not sure I like it.

@joemcgill
Copy link
Member

If we want to support PHP 8.1 with WPCS sniffs, I think the only real solution is to use the dev-develop branch until they've released version 3.0. Pinning our support to a specific commit hash is not a bad idea. Alternately, if we know there are specific tests that should be avoided, we can ship our own phpcs.xml file that excludes any rules that are problematic, like the I18n_Usage_Check sniff.

@spacedmonkey
Copy link
Member

If we are going to load from a dev-develop we HAVE to lock down to a commit. Otherwise, upstream change can break our plugin. I have had this problem on other projects.

@jjgrainger
Copy link
Contributor Author

Thanks for the input here everyone!

Just adding that we plan to use a number of WPCS sniffs in other checks (not just the I18n_Usage_Check). So it looks like the best path forward here, as others have already stated, is to use the dev-develop branch at a specific commit 👍

@mukeshpanchal27 can you move this one forward with this approach and let me know if you have any question. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA Testing Issues identified from QA testing [Type] Bug An existing feature is broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants