Optimize PHPCS config and bump minimum PHP requirement

swissspidy commented 5 years ago

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

Write some bad code in a PHP file within tests
Run npm run lint:php
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 `*/tests/*` line from `phpcs.xml` - we want to lint these too! * Remove `*/phpunit.xml*` line - there is no such folder and there would be no PHP in that anyway. * Remove `*/phpunit.xml*` - 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 commented 5 years ago

@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 commented 5 years ago

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

aaemnnosttv commented 5 years ago

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

Yep, that's what I meant 👍

swissspidy commented 4 years ago

Updated the IB accordingly.

ThierryA commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Sure, checking now 👍

swissspidy commented 4 years ago

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 commented 4 years ago

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

felixarntz commented 4 years ago

@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 commented 4 years ago

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

felixarntz commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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!

google / site-kit-wp