WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 7 weeks ago

#47411 reopened defect (bug)

Build tools, WPCS: disable line ending check

Reported by: azaozz Owned by: azaozz
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: 2nd-opinion
Focuses: coding-standards Cc:
PR Number:

Description

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.

Attachments (1)

47411.diff (762 bytes) - added by azaozz 7 months ago.

Download all attachments as: .zip

Change History (16)

@azaozz
7 months ago

#1 @azaozz
7 months ago

  • Keywords needs-testing added

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.

#2 follow-up: @netweb
7 months ago

  • Keywords commit added

Nice, looks good to me 👍🏼

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

#3 @azaozz
7 months ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

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.

#4 in reply to: ↑ 2 @azaozz
7 months ago

Replying to netweb:

Nice, looks good to me

Thanks for testing :)

#5 follow-up: @johnbillion
7 months ago

  • Focuses coding-standards added
  • Keywords 2nd-opinion added; needs-testing commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

#6 in reply to: ↑ 5 ; follow-up: @azaozz
7 months ago

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 :)

Last edited 7 months ago by azaozz (previous) (diff)

#7 in reply to: ↑ 6 ; follow-up: @SergeyBiryukov
7 months ago

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

#8 in reply to: ↑ 7 @azaozz
7 months ago

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.

#9 follow-ups: @jrf
3 months ago

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....

#10 in reply to: ↑ 9 @SergeyBiryukov
3 months ago

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?

#11 in reply to: ↑ 9 ; follow-up: @azaozz
3 months ago

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.

#12 in reply to: ↑ 11 @SergeyBiryukov
3 months ago

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.

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

#13 @netweb
3 months ago

  • Milestone changed from 5.3 to 5.4

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

#14 @SergeyBiryukov
7 weeks ago

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 not change it back and forth.
Last edited 7 weeks ago by SergeyBiryukov (previous) (diff)

#15 @SergeyBiryukov
7 weeks ago

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.

Note: See TracTickets for help on using tickets.