WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 3 months ago

#47411 reopened defect (bug)

Build tools, WPCS: disable line ending check

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

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 3 months ago.

Download all attachments as: .zip

Change History (9)

@azaozz
3 months ago

#1 @azaozz
3 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
3 months ago

  • Keywords commit added

Nice, looks good to me 👍🏼

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

#3 @azaozz
3 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
3 months ago

Replying to netweb:

Nice, looks good to me

Thanks for testing :)

#5 follow-up: @johnbillion
3 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
3 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 3 months ago by azaozz (previous) (diff)

#7 in reply to: ↑ 6 ; follow-up: @SergeyBiryukov
3 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
3 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.

Note: See TracTickets for help on using tickets.