Build tools, WPCS: disable line ending check

[azaozz]

Running phpcs on Windows always throws a "line ending" error for all files. Happens because when Git and SVN "check-out" the source they convert line ending to "native", but phpcs expects \n.

[azaozz]

In 47411.diff: disable the line ending check when reporting errors, i.e. running phpcs, but fix the line ending to \n when fixing by running phpcfb.

This makes phpcs less "noisy" on Windows and syncs the behaviour on different platforms.

[netweb]

Nice, looks good to me 👍🏼

Also see #core chat ​https://wordpress.slack.com/conversation/C02RQBWTW/p1558980094159100

[azaozz]

In 45455:

Build tools, WPCS: disable line ending check when running phpcs but fix them to \n whit phpcfb. Fixes (erroneous) reports on Windows about improper line endings in all files. They are managed directly by Git or SVN.

Fixes #47411.

[azaozz]

Replying to netweb:

Nice, looks good to me

Thanks for testing :)

[johnbillion]

Similar to my comment on 7:ticket:46869, globally overriding one of core's coding standards in core signifies that something is wrong with the rule and needs to be fixed. Overriding it in core means core doesn't adhere to core's own coding standards.

This change prevents "real" line ending issues from being discovered during code sniffing.

• Does this rule need to be fixed upstream in WPCS?
• Is there a lower level issue with PHPCS?
• Is this an issue with the local Git/SVN configuration?
• How does every other project that uses PHPCS handle this problem?

[azaozz]

Replying to johnbillion:

...globally overriding one of core's coding standards

Line endings, i.e. \r\n vs. \n have never been part of the coding standards and as far as I see are not part of any (popular) coding standard. I don't mind enforcing either one or the other but that would mean forcing all people that want to contribute something to change their (global) git or svn configuration.

This change prevents "real" line ending issues from being discovered during code sniffing.

Not sure what "real" line ending issues are. The facts are that doing svn co or git clone on Windows changes all line endings in all files to \r\n. Then running phpcs triggers errors/warnings for each file in WordPress.

I know, I've taken it upon myself to still (try to) develop on Windows when pretty much "everybody" is on Mac or Linux. This is exactly to be able to spot and fix this kind of issues (and of course to be able to easily test in the Windows browsers). BTW there are some unit tests that fail because \n line endings are hard-coded in the source, see #46798. Going to fix that next :)

To answer your questions:

• Does this rule need to be fixed upstream in WPCS?

No, it is not part of WPCS.

• Is there a lower level issue with PHPCS?

It is not an issue. It is a (configurable) setting.

• Is this an issue with the local Git/SVN configuration?

No. The global (default) configuration handles line endings in both Git and SVN.

• How does every other project that uses PHPCS handle this problem?

Not sure. There are probably thousands of projects that use PHPCS :)

It would be great if you can have a look at building and testing WP on Windows. I may be missing something. Help is very much appreciated :)

[SergeyBiryukov]

Replying to azaozz:

I know, I've taken it upon myself to still (try to) develop on Windows when pretty much "everybody" is on Mac or Linux. This is exactly to be able to spot and fix this kind of issues (and of course to be able to easily test in the Windows browsers).

FWIW, Windows is my main platform too.

I've noticed that phpcs flags \r\n line endings as an issue, but it didn't seem like a big deal for me, as I generally run phpcbf on the files I'm going to commit, and it fixes line endings in the process.

So I don't have a strong preference here.

BTW there are some unit tests that fail because \n line endings are hard-coded in the source, see #46798. Going to fix that next :)

Related: #31432

[azaozz]

Replying to SergeyBiryukov:

I've noticed that phpcs flags \r\n line endings as an issue, but it didn't seem like a big deal

Yeah, it's just... annoying throwing all these incorrect errors. If you run phpcs in the checkout dir, there are about.... 2 meters of console output just from that :)

So I don't have a strong preference here.

Frankly, me neither. It's annoying but can "live with it" :) Just trying to make this work better for everybody.

[jrf]

I only just saw this ticket.

Please revert the committed patch as it is plain wrong.

I completely agree with what @johnbillion said above.

Overriding it in core means core doesn't adhere to core's own coding standards.

This is not an issue with WPCS or even the Core phpcs.xml.dist ruleset, this is an issue with the setup as used by an individual developer.

There are two solutions for this - either one of which should be applied by the individual developer -. Either solution will fix this, you only need to choose which one you prefer to use.

This is not a Core issue.

Solution 1: Set up git to check-out as-is.

How git checks out files line-ending wise is a configuration setting of the git install on the local machine.
By default, on Windows, it will check out files Windows-style and commit Unix-style.
However, this can easily be changed to check out as-is. This is even one of the questions during the normal git install procedure on Windows.

> git config --global core.autocrlf input

Ref: ​https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration - section: Formatting and Whitespace

Same should be possible for SVN.

Alternatively, leave the settings as they are, git commit locally first and only run PHPCS after that as the git commit will fix the line-endings automatically.

Solution 2: Locally overload the PHPCS configuration.

The phpcs.xml.dist file are the project rules and should not be changed for something like this.

However, if an individual dev wants to locally ignore certain issues (or add extra rules) to facilitate their personal work-flow, that is perfectly fine and PHPCS facilitates this easily.

Just add a phpcs.xml file to the root of your WP repo (no worries, that file is git-ignored) with the following contents:

<?xml version="1.0"?>
<ruleset name="WP Core local">

	<rule ref="./phpcs.xml.dist">
		<exclude name="Generic.Files.LineEndings"/>
	</rule>
</ruleset>

---

Other than that:

as far as I see are not part of any (popular) coding standard.

I guess PSR2 and PSR12 are not popular coding standards in that case...
​https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md#22-files

Does this rule need to be fixed upstream in WPCS?

No, it is not part of WPCS.

It is part of WPCS, but is not something which should be "fixed". It is correct as it is.

---

Please feel free to ping me about issues like this in the future using my handle. As Meta has not been sending mails for subscribed topics for ages, I won't see it otherwise.

P.S.: You're not the only Windows based dev out there....

[SergeyBiryukov]

Replying to jrf:

This is not an issue with WPCS or even the Core phpcs.xml.dist ruleset, this is an issue with the setup as used by an individual developer.

There are two solutions for this - either one of which should be applied by the individual developer -. Either solution will fix this, you only need to choose which one you prefer to use.

Thanks for the feedback! I agree, let's revert [45455] then.

How git checks out files line-ending wise is a configuration setting of the git install on the local machine.
By default, on Windows, it will check out files Windows-style and commit Unix-style.
However, this can easily be changed to check out as-is. This is even one of the questions during the normal git install procedure on Windows.

> git config --global core.autocrlf input

Ref: ​https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration - section: Formatting and Whitespace

Same should be possible for SVN.

For SVN this seems not as straightforward, as core historically has svn:eol-style set to native for most of the files: #3264, #6065, #12403, [17340] [22798], also #45412. This appears to be a ​global setting, I haven't found a way to change it locally.

Re-reading #3264, maybe the original issue where patches could not be applied due to different line endings (#3217) is no longer relevant now that we have grunt patch, and we could set svn:eol-style to LF instead of native?

[azaozz]

Replying to jrf:

It's not as simple :) Lets summarize.

WP supports both git and svn. In the git case:

• With the default git settings line endings are controlled automatically. It is not advisable to change the defaults unless all contributors to a particular repository change their settings. In addition these are "global" settings in git so requiring a contributor to change them for one project is not a good idea.
• With the default git settings line endings are automatically converted to match the local env. Preemptive line ending checks are redundant. This also means WPCS checking line endings is incompatible with the git default setting on Windows (unless it is set to check for \r\n).

For svn:

• This is somewhat more involved as @SergeyBiryukov mentions above. "Line endings style" is a property of each file there. That property is committed to the repository. Changing the property will invalidate all existing patches, and may cause regressions like bringing back #3264.
• Similarly to git svn automatically sets the line endings style depending on local env. That, again, makes the line endings check in WPCS redundant and incompatible with Windows. In addition svn will throw an error on commit (and block the commit) if line endings are different style, i.e. some are \r\n some are \n.

The phpcs.xml.dist file are the project rules and should not be changed for something like this.

I don't understand what this means. There is a buggy rule that doesn't work properly, and that can be fixed by changing the settings. Should the WordPress project ignore the bug and continue to use the buggy rule?

However, if an individual dev wants to locally ignore certain issues...

This bug affects all contributors that use Windows. If we want to use an exception it should be added to core.

At this point I'm starting to wonder why WPCS is checking line endings at all? Do we expect git and svn to be buggy and fail? How was this handled before the had WPCS sniffs, were there any "line endings accidents"?

I don't mind reverting the commit, but I'd rather remove the line endings check from WPCS as it is:

1. Buggy. Doesn't work on Windows.
2. Not needed. Line endings are handled better by git/svn.

[SergeyBiryukov]

Replying to azaozz:

At this point I'm starting to wonder why WPCS is checking line endings at all? Do we expect git and svn to be buggy and fail?

Good point, I'm also curious about why this check was needed in the first place.

I don't see any mentions in ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/ about enforcing specific line endings.

How was this handled before the had WPCS sniffs, were there any "line endings accidents"?

#28187 comes to mind, but it doesn't look like WPCS would help there, as it appears to be an issue with develop → core SVN sync script, not with the files themselves.

#45138 is also somewhat related.

[netweb]

This can be added to any milestone when ready, punting to 5.4 for now to help clear the way for 5.3

[SergeyBiryukov]

Related: [46586-46588] for #42594.

[46586] made #31432 much more prominent. Basically any test with a multiline string output now fails on Windows, seeing 47 failures on trunk.

It's also bit awkward that phpcbf and phpcs convert all files to \n and enforce Unix EOLs on Windows, then SVN converts them back to \r\n, and so on.

Seems like we have two options here:

1. Don't enforce a particular EOL style.
2. If enforcing, don't change it back and forth.

[SergeyBiryukov]

In 46612:

Build/Test Tools: Ignore EOL differences in tests using multiline string assertions.

Unix vs. Windows EOL style mismatches can cause misleading failures in tests using the heredoc syntax (<<<) or multiline strings as the expected result.

Fixes #31432. See #42594, #47411.

[SergeyBiryukov]

It's still doesn't make sense to me that phpcbf converts all files to \n to enforce Unix EOLs on Windows, then SVN converts them back to \r\n, over and over again.

With 5.4 RC1 approaching, this could be addressed in 5.5 though.

[SergeyBiryukov]

Replying to SergeyBiryukov:

It's still doesn't make sense to me that phpcbf converts all files to \n to enforce Unix EOLs on Windows, then SVN converts them back to \r\n, over and over again.

These back-and-forth changes take a significant amount of time and are completely unnecessary.

Since core has svn:eol-style set to native for most of the files, the Generic.Files.LineEndings sniff is simply not relevant for us and should be excluded entirely, as it's not a part of WPCS.

See 47411.2.diff​.

[azaozz]

Replying to SergeyBiryukov:

These back-and-forth changes take a significant amount of time and are completely unnecessary.

Yes, they are useless. Agree that the Generic.Files.LineEndings sniff is not relevant.

Also agree with @johnbillion that ideally this should be fixed in WPCS. Opened ​https://github.com/WordPress/WordPress-Coding-Standards/issues/1880 for that.

Thinking it would be good to disable the above rule in the settings in core while waiting for WPCS sniffs/settings to be updated. +1 to commit.

[GaryJ]

Replying to azaozz:

• With the default git settings line endings are controlled automatically. It is not advisable to change the defaults unless all contributors to a particular repository change their settings. In addition these are "global" settings in git so requiring a contributor to change them for one project is not a good idea.

Adding a .gitattributes file that defines *.php (and possibly others) as text files (and therefore LF line endings) is all that's needed to address that here. See ​https://adaptivepatchwork.com/2012/03/01/mind-the-end-of-your-line/ or a newer article of your choosing. No global settings changes needed at the local git config level.

For svn:

• This is somewhat more involved as @SergeyBiryukov mentions above. "Line endings style" is a property of each file there. That property is committed to the repository.

Subversion will store the file in the repository using normalized LF EOL markers. So it seems like the presence of the svn:eol-style set to native is the only thing that is allowing Windows users to have those files converted to CRLF.

For Windows users using an editor that supports EditorConfig, then ​this .editorconfig file will already be used to set line-endings to be LF.

That means that the per-file SVN eol-style property is directly conflicting with the .editorconfig, and with pretty much the rest of the PHP community who will be following code standards based off of PSR-2 / PSR-12 which says to use LF.

At this point I'm starting to wonder why WPCS is checking line endings at all? Do we expect git and svn to be buggy and fail? How was this handled before the had WPCS sniffs, were there any "line endings accidents"?

Line-endings can be checked without version control being needed - PHPCS setup for instant checks in an IDE when someone is working on a local environment, for instance.

[SergeyBiryukov]

Replying to SergeyBiryukov:

It's still doesn't make sense to me that phpcbf converts all files to \n to enforce Unix EOLs on Windows, then SVN converts them back to \r\n, over and over again.

These back-and-forth changes take a significant amount of time and are completely unnecessary.

Since core has svn:eol-style set to native for most of the files, the Generic.Files.LineEndings sniff is simply not relevant for us and should be excluded entirely, as it's not a part of WPCS.

After reading @jrf's reply on the ​GitHub issue, I'm not so sure anymore :)

It makes sense to me that the sniff is there to help correct mixed line endings if someone encounters them accidentally.

What doesn't make sense to me is that the sniff enforces Unix line endings regardless of the platform, which is incompatible with our svn:eol-style: native setting. As noted above, this is a global setting and cannot be changed locally. This results in phpcbf and SVN competing with each other and performing conflicting back-and-forth changes on all PHP files.

Certainly, I can exclude that sniff in my local phpcs.xml, and have already done so.

• Is that really the recommended way for anyone contributing to core using SVN on Windows?
• Should we reconsider the usage of svn:eol-style: native?
• Is there any way to configure that sniff to set the eolChar value based on the platform?

[whyisjake]

Not sure this is ready for commit, as there seems to be a few open ideas on how to best accomplish this. I'm adding dev-feedback unless someone thinks we are good to go here.

[SergeyBiryukov]

Since 47411.2.diff​ didn't get enough consensus, let's close this as fixed in [45455] and [46612].

As noted in comment:21, excluding the Generic.Files.LineEndings sniff in a local phpcs.xml file appears to be the only viable option for WordPress development on Windows for now.