Site-kit-wp: Optimize PHPCS config and bump minimum PHP requirement

[swissspidy]

Bug Description

As noticed in #541, the tests folder is excluded from the PHPCS config, which I think is a bad practice. We shouldn't be making such big exceptions to code style adherence. Especially when this let's us potentially slip through errors that go unnoticed.

Steps to reproduce

1. Write some bad code in a PHP file within tests
2. Run npm run lint:php
3. Notice that nothing is flagged

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• Modify minimum required PHP version from 5.4 to 5.6 wherever applicable (e.g. readme.txt)
• Site Kit uses WPCS version 2.x
• PHP dependencies used in production remain fully compatible with PHP 5.6
• Linting with PHPCS+WPCS works in PHP 7.4
• Ensure PHPCS considers our oldest supported WP version 4.7
• Files in tests are handled by PHPCS
• No PHPCS errors exist

Implementation Brief

• Update required version of PHP in:
  
  
  
  • composer.json
  
  
  • readme.txt
  
  
  • googlesitekit_activate_plugin()
  
  
• Update phpunit/phpunit version constraint to ^5
• Run composer update and update lock file
• Remove \Google\Site_Kit\Core\REST_API\REST_Routes::parse_google_response_data (PHP 5.4 compatibility method)
• Remove gulp-tasks/phpcs.js
• Remove lint:php - no point in maintaining this in multiple places.
  Linting should be run with composer lint and composer lint-fix
• Remove <exclude-pattern>*/tests/*</exclude-pattern> line from phpcs.xml - we want to lint these too!
• Remove <exclude-pattern>*/phpunit.xml*</exclude-pattern> line - there is no such folder and there would be no PHP in that anyway.
• Remove <exclude-pattern>*/phpunit.xml*</exclude-pattern> - PHPCS doesn't lint XML files, only PHP files.
• Set <config name="minimum_supported_wp_version" value="4.7"/> in phpcs.xml
• Manually address remaining issues
• Add exception for WordPress.PHP.DisallowShortTernary
• Add additional exceptions to make some rules less-strict in TestCases

After autofixes, there is still a substantial number of code style fixes that need to be made manually

PHPCBF RESULT SUMMARY

FILE                                                  FIXED  REMAINING
----------------------------------------------------------------------
.../phpunit/integration/Core/Util/ActivationTest.php  1      15
...hpunit/integration/Core/Util/Exit_HandlerTest.php  3      2
...nit/integration/Core/Util/Migration_1_0_0Test.php  4      12
...unit/integration/Core/Util/Beta_MigrationTest.php  4      7
...ts/phpunit/integration/Core/Admin/ScreensTest.php  3      10
.../phpunit/integration/Core/Admin/DashboardTest.php  3      11
...sts/phpunit/integration/Core/Admin/ScreenTest.php  15     14
...sts/phpunit/integration/Core/Admin/NoticeTest.php  2      7
...ts/phpunit/integration/Core/Storage/CacheTest.php  1      7
.../phpunit/integration/Core/Storage/SettingTest.php  2      11
.../phpunit/integration/Core/Modules/ModulesTest.php  7      22
...s/phpunit/integration/Core/Modules/ModuleTest.php  9      22
...ts/phpunit/integration/Core/Assets/ScriptTest.php  14     16
...hpunit/integration/Core/Assets/StylesheetTest.php  13     14
...ts/phpunit/integration/Core/Assets/AssetsTest.php  4      10
.../Core/Authentication/Clients/OAuth_ClientTest.php  27     44
...t/integration/Core/Authentication/ProfileTest.php  6      6
...egration/Core/Authentication/Google_ProxyTest.php  1      4
...ration/Core/Authentication/AuthenticationTest.php  5      23
...tegration/Core/Authentication/CredentialsTest.php  15     12
...unit/integration/Core/REST_API/REST_RouteTest.php  11     10
...nit/integration/Core/REST_API/REST_RoutesTest.php  1      6
...ite-kit/tests/phpunit/integration/ContextTest.php  2      15
...site-kit/tests/phpunit/integration/PluginTest.php  1      10
...nit/integration/Modules/Optimize/SettingsTest.php  2      5
...sts/phpunit/integration/Modules/AnalyticsTest.php  2      18
...nit/integration/Modules/Site_VerificationTest.php  2      15
...unit/integration/Modules/AdSense/SettingsTest.php  6      6
...hpunit/integration/Modules/Search_ConsoleTest.php  3      9
...it/integration/Modules/Analytics/SettingsTest.php  12     10
...gle-site-kit/tests/phpunit/includes/functions.php  1      4
...ests/phpunit/includes/Core/Modules/FakeModule.php  4      7
...gle-site-kit/tests/phpunit/includes/MethodSpy.php  1      4
...ogle-site-kit/tests/phpunit/includes/TestCase.php  10     21
...-kit/tests/e2e/plugins/analytics-existing-tag.php  9      6
...gle-site-kit/tests/e2e/plugins/oauth-callback.php  5      14
.../tests/e2e/plugins/site-verification-api-mock.php  16     1
...lugins/google-site-kit/tests/e2e/plugins/auth.php  6      2
...ugins/google-site-kit/tests/e2e/plugins/reset.php  3      1
...2e/plugins/module-setup-tagmanager-no-account.php  8      1
...-site-kit/tests/e2e/plugins/site-verification.php  3      1
...e2e/plugins/module-setup-analytics-no-account.php  7      1
...-kit/tests/e2e/plugins/module-setup-analytics.php  20     15
...kit/tests/e2e/plugins/module-setup-tagmanager.php  6      3
...kit/tests/e2e/mu-plugins/e2e-rest-credentials.php  5      2
...it/tests/e2e/mu-plugins/e2e-rest-access-token.php  6      1
...ugins/e2e-rest-analytics-existing-property-id.php  6      1
...ite-kit/tests/e2e/mu-plugins/e2e-wp-api-fetch.php  4      1
...sts/e2e/mu-plugins/e2e-rest-site-verification.php  5      1
...e/mu-plugins/e2e-rest-search-console-property.php  5      1
...udes/Core/Authentication/Clients/OAuth_Client.php  1      7
.../google-site-kit/includes/Modules/Tag_Manager.php  1      6
----------------------------------------------------------------------
A TOTAL OF 313 ERRORS WERE FIXED IN 52 FILES
----------------------------------------------------------------------

Original IB

When writing new IB, please reuse what you can from below. Some things might no longer be applicable.

• Remove gulp-tasks/phpcs.js
• Change lint:php entry to use composer lint directly
• Remove <exclude-pattern>*/tests/*</exclude-pattern> line from phpcs.xml - we want to lint these too!
• Remove <exclude-pattern>*/phpunit.xml*</exclude-pattern> line - there is no such folder and there would be no PHP in that anyway.
• Remove <exclude-pattern>*/phpunit.xml*</exclude-pattern> - PHPCS doesn't lint XML files, only PHP files.
• Add lint:php:fix entry to use composer lint-fix to auto-fix issues reported by PHPCS
• Run npm run lint:php:fix to fix issues
• Manually address remaining issues
• Update composer lint and composer lint-fix scripts to remove vendor/bin prefix
  This should work as per https://getcomposer.org/doc/articles/scripts.md#writing-custom-commands

Exceptions could potentially bee added for files within tests to make PHPCS less strict for those.

Changelog entry

• Bump minimum required PHP version to 5.6, rely on up-to-date linting tools and fix various PHPCS issues.

[aaemnnosttv]

@swissspidy IMO PHP linting should be called from Composer using composer lint - the location of the installed binary shouldn't leak out of the composer script if we can avoid it. This relates to the changes suggested in #113 .

If we want want to make everything available from an npm script, then we could have it run the same command from npm but I think it makes more sense to use Composer directly for php-related scripts.

[swissspidy]

npm run lint:php could just run composer lint then? Easier to remember when everything can be easily accessed via one place.

[aaemnnosttv]

npm run lint:php could just run composer lint then?

Yep, that's what I meant 👍

[swissspidy]

Updated the IB accordingly.

[ThierryA]

To better understand the level of effort on this one (doing code formatting can be time consuming), @aaemnnosttv could you run PHPCS and find out the number of errors/warnings? And then run PHPCBF to find out how much can be automated.

[swissspidy]

IIRC last I checked there were lots of PHPCS warnings about missing doc blocks for tests. Since those are not always super helpful/relevant for tests anyway we could disable that rule specifically for tests. Then there shouldn‘t be too many non-autofixable issues I think.

[ThierryA]

IIRC last I checked there were lots of PHPCS warnings about missing doc blocks for tests.

Sure we could exclude the doc rule for the tests directory.

If one of you could estimate the level of effort, it would be really useful in order to decide whether this should be pre or post GA.

Thanks folks,

[swissspidy]

Sure, checking now 👍

[swissspidy]

Turns out I basically just resolved this while testing.

I excluded the doc rule for tests and then phpcbf fixed the vast majority of issues. The remaining ones were super minor and easy to fix. For example, changing parse_url to wp_parse_url or using yoda conditions.

While doing that I noticed that the sniffs were not fully accurate, realizing that the project currently uses the old 1.x version of WPCS instead of the more current 2.x version. So in my testing I updated that package to improve results. The upgrade path was really smooth.

[ThierryA]

Great, @marrrmarrr I think we can get this one move forward then since it is an XS and almost done.

[felixarntz]

@aaemnnosttv Let's wait with this until we've discussed our PHP version requirement further.

I also added one new AC I noticed we should have:

Ensure PHPCS considers our oldest supported WP version 4.7

We can set <config name="minimum_supported_wp_version" value="4.7"/> in phpcs.xml.

One other thing:
Regarding

Add exceptions for a few rules

we shouldn't exclude the sniff that disallows short-array syntax. Even if we raise the minimum PHP version to 5.6, this is a separate discussion to have later.

[aaemnnosttv]

@felixarntz I created a new issue for discussing the PHP requirement with a potential solution in #1050

[felixarntz]

@aaemnnosttv Let's bump the minimum PHP requirement to PHP 5.6 - good to move forward!

Can you update the IB accordingly? Let's not split out prod dependencies / dev dependencies for now.

[felixarntz]

IB ✅

@aaemnnosttv One additional thing regarding also linting our PHPUnit tests: I think we should still exclude tests for the WordPress-Docs ruleset specifically. We don't need to write proper doc blocks for all our test methods etc.

[aaemnnosttv]

I think we should still exclude tests for the WordPress-Docs ruleset specifically. We don't need to write proper doc blocks for all our test methods etc.

💯 👍

[tofumatt]

QA ✅

Wrote some code that would fail the linter in the tests/ directly and it failed, compared to previous builds where it wouldn't.

Also saw npm run lint:php is removed, which makes sense 👍🏻

All good!