CS/QA update for WPCS 1.0.0, PHPCS 3.3.1 & PHPCompatibility by jrfnl · Pull Request #1311 · Automattic/_s

jrfnl · 2018-08-09T05:17:32Z

Changes proposed in this Pull Request:

This PR updates the CS/QA related toolset to use the latest versions of these tools and updates the code-base to comply.

Commit details:

QA: Break stylesheets out of browser cache on each new release of a theme

WPCS 1.0.0 contains a new sniff which identifies this issue, which is why it has to be solved for the update to be possible

Type: Functional change

The wp_enqueue_style() function allows for passing a $version parameter which is used to force browsers to load a fresh copy of a stylesheet when it has been changed.

If this parameter is not passed, the version number will be set to the version of the WP install which doesn't make sense at all for a theme which is not shipped with WP itself.

This commit adds a _S_VERSION constant which should be changed to the current version number of the actual finished theme on each new release.

This constant is subsequently used in all wp_enqueue_style() function calls to ensure that on each new release of the theme, the browser cache is cleared for these stylesheets.

Ref: https://developer.wordpress.org/reference/functions/wp_enqueue_style/

CS: Minor fixes to comply with WPCS 1.0.0 / PHPCS 3.3.1

Type: CS update

Nothing very interesting, just some minor fixes to comply with the latest versions of all sniffs being tested against.

Use PHPCompatibilityWP

Type: Build tools

Since mid July, the PHPCompatibility project offers dedicated CMS rulesets which already account for all back-fills/poly-fills.

This commit removes the _s native version of that ruleset in favour of using this dedicated WP-based ruleset.

Additionally, the PHPCompatibility repo has moved to a dedicated GitHub organisation, so that change is included here as well.

Refs:

Note: I would strongly recommend installing these dependencies via Composer instead. @grappler has created a good setup for this in PR #1291 which I would recommend for merging, though it will need to be updated to be in line with the changes made in this PR.

Use WPCS 1.0.0 and sniff against a high PHP version for the fastest and most reliable results.

Type: Build tools

No ruleset changes are needed.

Refs: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases/tag/1.0.0

…heme The `wp_enqueue_style()` function allows for passing a `$version` parameter which is used to force browsers to load a fresh copy of a stylesheet when it has been changed. If this parameter is not passed, the version number will be set to the version of the WP install which doesn't make sense at all for a theme which is not shipped with WP itself. This commit adds a `_S_VERSION` constant which should be changed to the current version number of the actual finished theme on each new release. This constant is subsequently used in all `wp_enqueue_style()` function calls to ensure that on each new release of the theme, the browser cache is cleared for these stylesheets. Ref: https://developer.wordpress.org/reference/functions/wp_enqueue_style/

Since mid July, the PHPCompatibility project offers dedicated CMS rulesets which already account for all back-fills/poly-fills. This commit removes the `_s` native version of that ruleset in favour of using this dedicated WP-based ruleset. Additionally, the PHPCompatibility repo has moved to a dedicated GitHub organisation, so that change is included here as well. Refs: * https://github.com/PHPCompatibility/PHPCompatibility/releases/tag/8.2.0 * https://github.com/phpcompatibility/phpcompatibilitywp Note: I would strongly recommend installing these dependencies via Composer instead. grappler has created a good setup for this in PR 1291 which I would recommend for merging, though it will need to be updated to be in line with the changes made in this PR.

... and sniff against a high PHP version for the fastest and most reliable results. No ruleset changes are needed. Refs: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases/tag/1.0.0

jrfnl · 2018-09-12T09:20:11Z

Hmm... no response at all and in the mean time WPCS 1.1.0 has come out... makes it difficult to keep up.

samikeijonen · 2018-09-12T12:36:06Z

If I was in charge I'd say go for it! Looks good.

jrfnl · 2018-10-08T01:27:45Z

I'm willing to update this PR to account for WPCS 1.1.0 as well as PHPCompatibilityWP 2.0.0, however, I can't really be bothered if this PR is left open to rot....

philiparthurmoore · 2018-10-08T01:50:21Z

This looks good.

jrfnl · 2018-10-08T02:03:31Z

This looks good.

Except the build won't pass anymore now WPCS 1.1.0 has come out...

philiparthurmoore · 2018-10-08T02:04:34Z

Then we should update it. 😝

jrfnl · 2018-11-12T21:24:27Z

Just checking in - WPCS 1.2.0 has come out today. If I would update this PR to fix the issues and make the theme comply with WPCS 1.2.0, is there any chance of the PR being merged within a reasonable time-span (max 2 weeks) so it will not go stale again ?

limestreet · 2018-11-19T16:10:11Z

Hi @jrfnl
I see PHPCompatibilityWP uses PHPCompatibilityParagonieRandomCompat:
https://github.com/PHPCompatibility/PHPCompatibilityWP/blob/master/PHPCompatibilityWP/ruleset.xml#L34
It looks like it should be added here as well (as you added it in PHPCompatibilityWP 2.0):
https://github.com/Automattic/_s/pull/1311/files#diff-354f30a63fb0907d4ad57269548329e3

jrfnl · 2018-11-19T16:16:14Z

@limestreet I'm aware - see: #1311 (comment)

limestreet · 2018-11-19T16:28:09Z

I missed that one :)
BTW - it's really epic ticket... but great work!

grappler · 2019-03-07T12:13:57Z

functions.php

+	wp_enqueue_style( '_s-style', get_stylesheet_uri(), array(), _S_VERSION );

-	wp_enqueue_script( '_s-navigation', get_template_directory_uri() . '/js/navigation.js', array(), '20151215', true );
+	wp_enqueue_script( '_s-navigation', get_template_directory_uri() . '/js/navigation.js', array(), _S_VERSION, true );


Can we leave the version as is? The script does not get changed with every theme version. This way we would not need the constant.

Except that the scripts have been changed a couple of times already and people keep forgetting to update this cache-busting number, so I would strongly recommend leaving it as is (with the constant), as refreshing the script once on every new release is a minor slow-down, while serving an out-of-date script can break a site.

I don't agree but I don't want to stand in the way of this PR being merged. We might need a PR for https://github.com/Automattic/underscores.me/ too to search and replace _S.

enkrates · 2019-03-19T13:30:08Z

The build is failing on a missing dependency, the Paragonie compat sniff. It seems like it'd be pretty simple to also install that along the other sniffs, I'd be happy to help if that'd keep this PR moving forward.

jrfnl · 2019-03-19T15:32:49Z

@enkrates The build still wouldn't pass, but that is because no-one of the maintainers bothers to merge in a timely manner.
See #1311 (comment) and #1311 (comment)

ghost · 2019-03-19T15:59:57Z

Where are the maintainers of this build?

…

On Tue, Mar 19, 2019, 08:33 Juliette ***@***.***> wrote: @enkrates <https://github.com/enkrates> The build still wouldn't pass, but that is because no-one of the maintainers bothers to merge in a timely manner. See #1311 (comment) <#1311 (comment)> and #1311 (comment) <#1311 (comment)> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1311 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ae6ilwxZ1WbjCg8fXIydnUuDr323swv5ks5vYQM7gaJpZM4V1DuI> .

crunnells · 2019-03-27T20:22:10Z

@jrfnl If you'd like to update this PR to pass the build and update the version of WPCS I'll get it merged in a timely manner.

philiparthurmoore · 2019-12-22T04:45:44Z

@jrfnl I haven't kept up with this much. Are you done with it and just waiting for it to be merged? Trying to get a sense of current status without reading the entire thread. It seems like it was ready a while back and then nothing happened.

jrfnl · 2019-12-22T04:56:25Z

@philiparthurmoore TL:DR: This PR needs a big update as it was left to rot for a year and a half and all the standards involved have moved on since then.

I'd be willing to do the update, but only if I have a solid commitment that the PR will be reviewed & merged within two weeks of doing the update as otherwise it will quickly be out of date again.

Note: By now, I have very little faith in anything being merged within a reasonable timespan in this project based on (the lack of) progress on any PR in the past two years.

Also: it would be so much better if this project would use Composer to manage the (dev) dependencies as that way, it won't be bound to dev-master versions of the CS dependencies which will break builds unexpectedly.

From the PR description above:

Note: I would strongly recommend installing these dependencies via Composer instead. @grappler has created a good setup for this in PR #1291 which I would recommend for merging, though it will need to be updated to be in line with the changes made in this PR.

philiparthurmoore · 2019-12-22T05:20:58Z

@jrfnl, To be honest, I don't see the point of submitting another PR unless there is a firm commitment from the Automattic theme team to revive _s. Otherwise, I consider it in maintenance mode until I see activity that proves otherwise. I have the rights to merge pretty much anything and everything regarding _s but it doesn't make any sense for me to be the one doing the work if Automattic is using _s (or not) for themes now. Dave mentioned that they were using another starter theme a while back so I'm still a little confused about who leads this now.

szepeviktor · 2020-03-23T14:24:22Z

@jrfnl Begetting a child and being pregnant takes ~50% of time this PR gets merged.

👶 👶

jrfnl · 2020-03-23T14:34:46Z

@szepeviktor I'd still rather send in a PR though ;-)

Ismail-elkorchi · 2020-04-05T22:38:21Z

In #1386, I updated the CS tools and made Travis use these, then I created #1388 to make the theme comply with WPCS.

Ismail-elkorchi · 2020-04-09T10:48:17Z

@jrfnl Thank you for the Pull Request. The issues regarding coding standards and Travis were resolved in #1388 and #1386.

jrfnl added 4 commits August 9, 2018 04:08

CS: Minor fixes to comply with WPCS 1.0.0 / PHPCS 3.3.1

9f9cb4a

🚀 Build: use WPCS 1.0.0

30447f1

... and sniff against a high PHP version for the fastest and most reliable results. No ruleset changes are needed. Refs: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases/tag/1.0.0

jrfnl mentioned this pull request Aug 9, 2018

Use composer to install PHPCS and friends on Travis #1291

Closed

philiparthurmoore added Suggest Merge Second Opinion Has Patch labels Oct 8, 2018

philiparthurmoore removed the Suggest Merge label Oct 8, 2018

jrfnl mentioned this pull request Nov 19, 2018

ERROR: Referenced sniff "PHPCompatibility.PHP.RemovedExtensions" does not exist #1331

Closed

This was referenced Jan 4, 2019

[ UPDATE ] Opening parenthesis of a multi-line function call must be … #1338

Closed

[ UPDATE ] wpcs error - Opening parenthesis of a multi-line function … #1339

Closed

grappler reviewed Mar 7, 2019

View reviewed changes

mirucon mentioned this pull request Mar 19, 2019

chore(phpcs): Fix compatibility with PHPCompatibility 9.0 #1324

Closed

jrfnl mentioned this pull request Mar 26, 2019

Change WordPress.WP.EnqueuedResourceParameters.MissingVersion to warning WordPress/WordPress-Coding-Standards#1472

Closed

jrfnl mentioned this pull request Sep 17, 2019

Remove removed PHPCS rule #1374

Closed

tomjn mentioned this pull request Nov 12, 2019

Clarify the development status of Underscores #1369

Closed

jrfnl mentioned this pull request Dec 22, 2019

Close small menu when user clicks outside #1375 #1379

Merged

jrfnl referenced this pull request Mar 12, 2020

Merge 9dc37fa into 28f0a5b

da55a96

Ismail-elkorchi added Needs Refresh and removed Has Patch labels Mar 20, 2020

Ismail-elkorchi mentioned this pull request Mar 28, 2020

Update link & Fix typo. #1385

Merged

Ismail-elkorchi mentioned this pull request Apr 5, 2020

PHP, SCSS and JS linting tools for use in local and Travis #1386

Merged

Ismail-elkorchi closed this Apr 9, 2020

jrfnl deleted the feature/comply-with-wpcs-1.0.0 branch October 25, 2024 18:09

Comments

Conversation

jrfnl commented Aug 9, 2018

Changes proposed in this Pull Request:

Commit details:

QA: Break stylesheets out of browser cache on each new release of a theme

CS: Minor fixes to comply with WPCS 1.0.0 / PHPCS 3.3.1

Use PHPCompatibilityWP

Use WPCS 1.0.0 and sniff against a high PHP version for the fastest and most reliable results.

Uh oh!

jrfnl commented Sep 12, 2018

Uh oh!

samikeijonen commented Sep 12, 2018

Uh oh!

jrfnl commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philiparthurmoore commented Oct 8, 2018

Uh oh!

jrfnl commented Oct 8, 2018

Uh oh!

philiparthurmoore commented Oct 8, 2018

Uh oh!

jrfnl commented Nov 12, 2018

Uh oh!

limestreet commented Nov 19, 2018

Uh oh!

jrfnl commented Nov 19, 2018

Uh oh!

limestreet commented Nov 19, 2018

Uh oh!

grappler Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

jrfnl Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

grappler Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

enkrates commented Mar 19, 2019

Uh oh!

jrfnl commented Mar 19, 2019

Uh oh!

ghost commented Mar 19, 2019 via email

Uh oh!

crunnells commented Mar 27, 2019

Uh oh!

philiparthurmoore commented Dec 22, 2019

Uh oh!

jrfnl commented Dec 22, 2019

Uh oh!

philiparthurmoore commented Dec 22, 2019

Uh oh!

szepeviktor commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrfnl commented Mar 23, 2020

Uh oh!

Ismail-elkorchi commented Apr 5, 2020

Uh oh!

Ismail-elkorchi commented Apr 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jrfnl commented Oct 8, 2018 •

edited

Loading

szepeviktor commented Mar 23, 2020 •

edited

Loading