WPTT / theme-sniffer

Theme Sniffer plugin using sniffs.
MIT License
268 stars 3 forks source link

Issue with running on PHP 8 #232

Open TeBenachi opened 2 years ago

TeBenachi commented 2 years ago

When I tested a theme that requires PHP 8, the sniffer kept running without any results. The sniffer also kept running when testing with Twenty Twenty-Two.

It did not seem to matter what minimum PHP version I picked (Screenshot).

The sniffer ran without any issues on PHP 7.4.1.

I have the Local by Flywheel on Mac. I tested Twenty Twenty-Two on WordPress 5.9.1 on Firefox and Chrome.

sniffer-options

dingo-d commented 2 years ago

Do you have any errors in the debug.log? And in the inspector console?

The plugin wasn't tested with PHP8, and I'm not sure if anybody stepped up to maintain it.

TeBenachi commented 2 years ago

I did not see errors in the console but the debug log shows this error.

Array and string offset access syntax with curly braces is no longer supported
Line : 420
/wp-content/plugins/theme-sniffer/vendor/squizlabs/php_codesniffer/src/Config.php
dingo-d commented 2 years ago

This is due to the fact that the packages haven't been updated in a long while. I've pushed the branch with the updates: https://github.com/WPTT/theme-sniffer/tree/update-packages you can try this.

I've tried on PHP 8.1 and it seems to be working (the whole admin is completely broken because WP core is not compatible with 8.1 yet).

TeBenachi commented 2 years ago

Thank you for working on this so quickly! When I tried to install it via git, I get the error message below. If it isn't too much trouble, would you mind sharing in zip?

Problem 1
    - Root composer.json requires dealerdirect/phpcodesniffer-composer-installer ^0.5.0 -> satisfiable by dealerdirect/phpcodesniffer-composer-installer[v0.5.0].
    - dealerdirect/phpcodesniffer-composer-installer v0.5.0 requires php ^5.3|^7 -> your php version (8.1.3) does not satisfy that requirement.
kafleg commented 2 years ago

I just cloned the repo, but I am not able to check it.

Fatal error: Uncaught Error: Class 'Theme_Sniffer\Core\Plugin_Factory' not found in C:\laragon\www\review\wp-content\plugins\theme-sniffer\theme-sniffer.php:58 Stack trace: #0 C:\laragon\www\review\wp-settings.php(418): include_once() #1 C:\laragon\www\review\wp-config.php(103): require_once('C:\laragon\www\...') #2 C:\laragon\www\review\wp-load.php(50): require_once('C:\laragon\www\...') #3 C:\laragon\www\review\wp-admin\admin.php(34): require_once('C:\laragon\www\...') #4 C:\laragon\www\review\wp-admin\themes.php(10): require_once('C:\laragon\www\...') #5 {main} thrown in C:\laragon\www\review\wp-content\plugins\theme-sniffer\theme-sniffer.php on line 58

@dingo-d how can i check the GitHub version instead of using the ZIP file?

kafleg commented 2 years ago

Sorry, I mistakenly closed it. But i reopened now.

dingo-d commented 2 years ago

Thank you for working on this so quickly! When I tried to install it via git, I get the error message below. If it isn't too much trouble, would you mind sharing in zip?

Problem 1
    - Root composer.json requires dealerdirect/phpcodesniffer-composer-installer ^0.5.0 -> satisfiable by dealerdirect/phpcodesniffer-composer-installer[v0.5.0].
    - dealerdirect/phpcodesniffer-composer-installer v0.5.0 requires php ^5.3|^7 -> your php version (8.1.3) does not satisfy that requirement.

Try removing the composer.lock file and installing it from the scratch. Should be working then.

@kafleg For GH version, you'll need to clone the repo, checkout the branch I've pushed, delete composer.lock, and then run composer install and npm install && npm run build.

That should make it work.

TeBenachi commented 2 years ago

I’ve been trying for a few days but I still can’t seem to build a zip. I’m not entirely sure but node-gyp might be causing the build issue.

4350 error gyp ERR! node -v v16.14.0
4350 error gyp ERR! node-gyp -v v3.8.0
4350 error gyp ERR! not ok 
4350 error Build failed with error code: 1
dingo-d commented 2 years ago

On what command does this happen? I think I manually bundled the zip file 🤔

TeBenachi commented 2 years ago

Ha! I didn’t think about bundling manually 🙂 I will try that next time.

It fails at npm install.

dingo-d commented 2 years ago

Weird, should work on node 16 (it worked for me 😅)