Make WordPress Core

Opened 3 years ago

Closed 20 months ago

#53837 closed enhancement (wontfix)

Move the basepath config out of phpcs.xml.dist

Reported by: johnbillion's profile johnbillion Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

The basepath config option in phpcs.xml.dist is there to shorten the path names in PHPCS reports so they're relative to the project root directory rather than being absolute.

A few PHPCS tools I've used don't support a change to the base path and expect absolute paths in the report, and break without one. The most prominent example is the PHPCS extension for VS Code which has over a million installations. Unfortunately this particular extension hasn't seen any updates for three years, yet remains the number one PHPCS extension.

Clearly this is a problem with the third party tools but I've now seen several developers trying to contribute to WordPress core using VS Code and not being able to get PHPCS annotations working in their editor because the extension is expecting absolute paths in the report but receiving relative ones.

To solve the immediate problem we can move the basepath config out of phpcs.xml.dist and into the --basepath flag in the lint script in composer.json. This means external PHPCS tools will get the full paths but anything that uses composer lint gets the relative paths.

Change History (6)

This ticket was mentioned in PR #1531 on WordPress/wordpress-develop by johnbillion.


3 years ago
#1

  • Keywords has-patch added; needs-patch removed

johnbillion commented on PR #1531:


3 years ago
#2

it is a work-around for a select group of users using a specific tool, which makes things less convenient for the majority of users

My experience says this isn't the case, due to the way people most commonly use PHPCS. I work with various teams of experienced and inexperienced developers and almost none of them run PHPCS directly using ./vendor/bin/phpcs or a global phpcs, the majority of them interface with it as just another tool in their editor/IDE or via a project-level Composer or NPM script. VS Code has 14 million users (!) and the PHPCS extension over a million, so I'm sure this change will convenience many more users than those who are familiar with and use phpcs directly. Plus it's an immediate fix for VS Code users who want to contribute to WordPress, which is the main thing.

I believe the Make Core handbook should probably be enhanced with a few additional pages, informing users about how to install and run PHPCS

Yeah agreed. Maybe one for the docs team and test team to collaborate on.

I do think it needs checking that this change does not impact the cs2pr integration in GH Actions

I don't _think_ it will impact it because the usage example given on https://github.com/staabm/annotate-pull-request-from-checkstyle uses a vanilla PHPCS configuration without any value in basepath, but I'll check to make sure. Thanks.

jrfnl commented on PR #1531:


3 years ago
#3

it is a work-around for a select group of users using a specific tool, which makes things less convenient for the majority of users

My experience says this isn't the case, due to the way people most commonly use PHPCS. I work with various teams of experienced and inexperienced developers and almost none of them run PHPCS directly using ./vendor/bin/phpcs or a global phpcs, the majority of them interface with it as just another tool in their editor/IDE or via a project-level Composer or NPM script. VS Code has 14 million users (!) and the PHPCS extension over a million, so I'm sure this change will convenience many more users than those who are familiar with and use phpcs directly. Plus it's an immediate fix for VS Code users who want to contribute to WordPress, which is the main thing.

But we're not talking about all VS Code users here, we're talking about WP Core contributors. And while I don't have access to stats, I know a wide variety of IDEs and code editors is used by those (PHPStorm, Sublime, Atom, Eclipse, GH Code spaces and yes, VS Code, to name a few) and this change is only for the users of VS Code and may or may not negatively impact users of other editors.

Have any tests been done to see if this change impacts users of other editors ?

johnbillion commented on PR #1531:


3 years ago
#4

The fix is to revert a non-default PHPCS setting to its default; what negative impact in other editors are you imagining?

jrfnl commented on PR #1531:


3 years ago
#5

The fix is to revert a non-default PHPCS setting to its default; what negative impact in other editors are you imagining?

Not imagining anything, just wanted to ask the question as if we're making changes to accommodate one editor, it seemed sensible to make sure there is no impact on other editors.

#6 @johnbillion
20 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.