Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Absolute vs relative paths #156

Open
jrfnl opened this issue Feb 7, 2022 · 9 comments
Open

Absolute vs relative paths #156

jrfnl opened this issue Feb 7, 2022 · 9 comments

Comments

@jrfnl
Copy link
Member

jrfnl commented Feb 7, 2022

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:

  • Global installs use absolute paths
  • Local installs use relative paths

Problems I'm seeing

  • In some tests on Windows, I'm seeing duplicate relative paths being registered, i.e.:
    Using config file: C:\\Users\\runneradmin\\AppData\\Local\\Temp\\PHPCSPluginTest\\RegisterExternalStandardsTest_61fe56765bc692.23339840\\local\\vendor\\squizlabs\\php_codesniffer\\CodeSniffer.conf
    
    installed_paths: ../../../../../../../../../../RUNNER~1/AppData/Local/Temp/PHPCSPluginTest/RegisterExternalStandardsTest_61fe56765bc692.23339840/local/vendor/phpcs-composer-installer,../../phpcs-composer-installer
    
    As this will result in the same standard(s) being registered twice with PHPCS, this is likely to cause problems with the autoloading of sniffs.
  • Even without duplicate paths, I've had reports of relative paths causing problems with the PHPCS autoloading of sniffs: Unable to get stock install working, cannot declare class errors PHPCompatibility/PHPCompatibility#1311 (comment)

Proposal

Always use absolute paths.

Further research needed

  • What about projects in a dual OS setup, like Windows with WSL or a Vagrant environment ?
  • What about projects with a committed vendor directory ?
    => I'd like to suggest that they can add a script to run this plugin to their composer.json and can run that script whenever the project is checked out. After all, the plugin will update the paths based on the runtime environment, so should fix the paths in the committed CodeSniffer.Conf file to the new environment.
  • Can this be made easier ?
    => In Composer 2.x, Plugins can add their own commands to Composer. We could consider adding a custom command to run this plugin. This would negate the need to add the script.
    Ref: https://getcomposer.org/doc/articles/plugins.md#command-provider
  • Should there still be an option for users to use relative paths ?
    => Along the same lines as the search-depth option, we could consider adding a force-relative-paths option.
    We should probably also investigate if custom commands can take custom arguments. In that case, we could possible add support for a --force-relative-paths CLI argument if needs be.
@jrfnl
Copy link
Member Author

jrfnl commented Jun 24, 2022

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

@Potherca
Copy link
Member

Potherca commented Jun 24, 2022

Working on a response, to be updated soon (need to catch the bus now 🚌🏃💨)

@Potherca
Copy link
Member

Potherca commented Jun 25, 2022

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 PHPCompatibility/PHPCompatibility#1311 (comment)

🆚

I manually changed [the aboslute path in] CodeSniffer.conf to [a relative path]
@tmountjr in #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
Copy link
Member Author

jrfnl commented Jun 25, 2022

Just to be complete, we had another report of things not working due to duplicate paths (one relative, one absolute) yesterday: PHPCompatibility/PHPCompatibility#1303 (comment)

@Potherca
Copy link
Member

Thinking about the usecase...

  • For a single machine setup, either absolute or relative paths works fine regardless of "local" or global install is used.
  • For a multi machine setup, only relative paths work, again regardless of local or global.

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
Copy link
Member Author

jrfnl commented Jun 26, 2022

@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
Copy link
Member Author

jrfnl commented Jun 27, 2022

☝️ Turns out that the duplicate path reported in PHPCompatibility/PHPCompatibility#1303 (comment) was not caused by this plugin, but by the user also running a script.

@Potherca
Copy link
Member

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

@jrfnl
Copy link
Member Author

jrfnl commented Jun 28, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants