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

Fix i18n violations #2271

Merged
merged 3 commits into from Oct 29, 2018

Conversation

Projects
None yet
2 participants
@GaryJones
Member

GaryJones commented Oct 16, 2018

Fixes a small mistake I introduced in 64e86d7 for the PHPS config file - should be <element value="..."/> and not <element name="..."/>.

Also, adds an XSD reference to the phpcs.xml.dist so that issues like this are more easily spotted via IDEs and any other XML validation tools.

Requires PHPCS 3.3.2, which is when the new array element attribute was added to the XSD file.

Note, that this now does enable the prefix and i18n checks that the previous effort was meant to!

GaryJones added some commits Oct 16, 2018

PHPCS: Use `value`, not `name` attribute, with `element`
Although the `name` didn't cause any error, it also meant that prefixes and i18n checks weren't actually activated.

@GaryJones GaryJones self-assigned this Oct 16, 2018

@GaryJones GaryJones requested a review from donnapep Oct 16, 2018

@donnapep

This comment has been minimized.

Contributor

donnapep commented Oct 17, 2018

@GaryJones This PR fails on PHP 7.2 on Travis CI:

screen shot 2018-10-17 at 7 20 30 am

@GaryJones

This comment has been minimized.

Member

GaryJones commented Oct 17, 2018

This PR fails on PHP 7.2 on Travis CI:

So, this proves that the extra checks are now working :-)

The errors are related to text domains. Ideally, each plugin should have exactly one text domain, and use it for all its strings. It should not rely on strings from other plugins or WP core, which is what it seems to be doing here.

See the Multiple Text Domains section in @swissspidy's article for background info.

If you're in agreement with this, then I can add a commit which fixes the violations, and adds/changes them all to woothemes-sensei.

This will require a new .pot file to be generated (if one is distributed), and new strings to be translated on translate.wordpress.org, for the next release.

@donnapep

This comment has been minimized.

Contributor

donnapep commented Oct 17, 2018

If you're in agreement with this, then I can add a commit which fixes the violations, and adds/changes them all to woothemes-sensei.

Sure, that would be great. 🙂

@donnapep

This comment has been minimized.

Contributor

donnapep commented Oct 17, 2018

Also, curious as to why it would fail on the text domain check but not on any of the other ones. Do you know if it's based on severity, or something else?

@GaryJones

This comment has been minimized.

Member

GaryJones commented Oct 17, 2018

Also, curious as to why it would fail on the text domain check but not on any of the other ones. Do you know if it's based on severity, or something else?

Because the Travis setup doesn't run a full PHPCS check (which makes sense, since there's a lot of violations to address). Instead, it just runs the checks from just two sniffs:

- ./vendor/bin/phpcs -s --sniffs=WordPress.WP.I18n,Generic.PHP.Syntax .

It's only flagged now, because the lack of text domain in the PHPCS config basically meant the I18n sniff wasn't doing much before, so it was silently doing nothing. Now the text domain is set, the check is kicking in.

Obviously, thete's lots more that could and should be checked:

------------------------------------------------------------------------------------------
A TOTAL OF 37175 SNIFF VIOLATIONS WERE FOUND IN 220 SOURCES
------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 121 MARKED SOURCES AUTOMATICALLY (29018 VIOLATIONS IN TOTAL)
------------------------------------------------------------------------------------------

There are about 6 times more violations in Sensei than in WP Core :-) Luckily, about 78% of these can be auto-fixed, and another 8% are labouriously manual but not mind-taxing to fix, which would bring the remaining fixes down to about 5000, some easy to fix, some tricky.

@donnapep

This comment has been minimized.

Contributor

donnapep commented Oct 17, 2018

Yes, we had a hack week not too long ago to address some of these on a file-by-file basis. Adding comments to functions was particularly tedious. Now, our plan is to leave every file we touch in a better state than it was before. We'll take all the help we can get with fixing violations. 🙂

@GaryJones

This comment has been minimized.

Member

GaryJones commented Oct 17, 2018

Violation fixes have been added, so they should now pass in Travis.

screenshot 2018-10-17 13 33 47

@donnapep donnapep added this to the 1.12.1 milestone Oct 17, 2018

@GaryJones

This comment has been minimized.

Member

GaryJones commented Oct 17, 2018

Noting that Sensei_Usage_Tracking_Base has an I18n sniff check disabled, and uses a non-string literal for the text domain - not addressed in this PR.

@donnapep

This comment has been minimized.

Contributor

donnapep commented Oct 23, 2018

@GaryJones Curious about something. Would it make more sense for us to adhere to the WordPress-VIP-Go PHPCS standard so that we can be assured Sensei will always work on VIP sites?

@GaryJones

This comment has been minimized.

Member

GaryJones commented Oct 24, 2018

Would it make more sense for us to adhere to the WordPress-VIP-Go PHPCS standard so that we can be assured Sensei will always work on VIP sites?

Absolutely :-) The WordPress-VIP-Go standard could be added in on top of the WPCS and PHPCompatibilityWP standards - it's not a case of needing to choose one over the other.

@donnapep donnapep modified the milestones: 1.12.2, 1.12.1 Oct 27, 2018

@donnapep donnapep removed the [Type] Bug label Oct 27, 2018

@donnapep donnapep changed the title from PHPCS: Fix element attribute + add XSD to Fix i18n violations Oct 29, 2018

@donnapep donnapep merged commit f314f43 into master Oct 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@donnapep donnapep deleted the feature/phpcs-elements-attribute branch Oct 29, 2018

@alexsanford alexsanford referenced this pull request Oct 29, 2018

Merged

Release 1.12.1 Beta 1 #2290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment