PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards
https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer
MIT License
542 stars 36 forks source link

Absolute vs relative paths #156

Open jrfnl opened 2 years ago

jrfnl commented 2 years ago

I'd like to re-open the discussion around whether the paths registered with PHPCS should be absolute or relative paths.

Some previous issues related to this: #14, #33, #73

Current status after #28:

Problems I'm seeing

Proposal

Always use absolute paths.

Further research needed

jrfnl commented 2 years ago

@Potherca Would you mind pitching in with an opinion please ?

Potherca commented 2 years ago

Working on a response, to be updated soon (need to catch the bus now :bus::runner::dash:)

Potherca commented 2 years ago

Okay,so... I was trying to remember why relative paths were initially used (other than for convenience) and "how we got here"™️.

One path was through the MRs/Issues linked above, the other was through the commit log.

The current state of afairs (absolute paths for global install, relative path for local install) comes from the use-case outlined in #14.

My suggestion [..] is to use relative paths on "local" repositories and absolute paths when using globals installations.

This should make it work in all use cases.

and

There does not seem to be any downside to always using relative paths for "local" (project specific) installs. As this might cause problems for "global" (system-wide) installs, it makes sense to always use absolute paths there.

I can not think of any scenario's where this distinction might cause a problem.

The user feedback basically gives two competing problems/solutions:

the CodeSniffer.conf file [..] had been set to a relative path, and I updated it to an absolute path. \@bendauphineewp in https://github.com/PHPCompatibility/PHPCompatibility/issues/1311#issuecomment-1011697527

🆚

I manually changed [the aboslute path in] CodeSniffer.conf to [a relative path] \@tmountjr in https://github.com/PHPCSStandards/composer-installer/issues/14

So this got me thinking about what the uses-cases and scenarios there actually are.

I think this one:

What about projects in a dual OS setup, like Windows with WSL or a Vagrant environment ?

is the most relevant, as this is the most common problem (also including docker and running the plugin in CI/CD environments like Gitlab Pipelines and GitHub Actions).

Still need to think about this some more, thought. 🤔

jrfnl commented 2 years ago

Just to be complete, we had another report of things not working due to duplicate paths (one relative, one absolute) yesterday: https://github.com/PHPCompatibility/PHPCompatibility/issues/1303#issuecomment-1165417385

Potherca commented 2 years ago

Thinking about the usecase...

But multi-machine is only when two (or more) environments use the same directory at the same time.

The remote CI/CD for instance is still a single-machine.

What I think I am trying to say is that a user is unlikely to accidentally have a multi-machine setup (so we need a better description for this). The majority of use-cases I personally encounter in that area are people using docker (or other VMs) and running PHP (or other) commands outside the VM. Which they should not do but it still happens.

I get the feeling that for the majority of the usecases using absolute paths would be fine, but we don't know what the users wants or needs. So they need to tell us.

Which leads back to the solution originally proposed in #14 of adding a config setting.

A force-relative-paths config setting could be implemented right away, without causing any backward breaking defects. Conversely, a force-absolute-paths could be added at the same time. That would give users a way to decide, and the documentation of such a thing should also raise awareness of the underlying implementation and repercussions.

That should also give us more time to gather usecases and decide if moving all path to absolute by default is then still needed.

jrfnl commented 2 years ago

@Potherca Thanks for adding your perspective. While I agree that adding the config settings would be nice addition, I have a feeling that it doesn't address the real-life problems people currently have with duplicate paths.

jrfnl commented 2 years ago

:point_up: Turns out that the duplicate path reported in https://github.com/PHPCompatibility/PHPCompatibility/issues/1303#issuecomment-1165417385 was not caused by this plugin, but by the user also running a script.

Potherca commented 2 years ago

I think we might still want to consider adding the flags to force absolute or relative paths, to give users more control...

jrfnl commented 2 years ago

@Potherca Oh, totally! And the fact that the above mentioned issue was not caused by the plugin, doesn't take anything away from the results I reported in the original description where I was seeing duplicate paths being registered during tests runs (for tests not yet pulled), so the original issue still stands.