search

PHPCSStandards / composer-installer

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

Fix for issue #103 #104

Closed Potherca closed 4 years ago

Potherca commented 4 years ago

If the phpcodesniffer-composer-installer plugin is installed as a dev requirement and it is then uninstalled as part of a "--no-dev" install, a bug occurs. The bug that occurs is that the plugin complains that the package "squizlabs/php_codesniffer" is not installed without checking if the package should be present.

Proposed Changes

This commit adds a check to verify that this plugin is actually installed before complaining about the missing package. If this plugin itself is removed, then it should not complain about the missing package.

Related Issues

103

Potherca commented 4 years ago

You are correct... The "Is this plugin installed?" check should always run. It doesn't matter if it is require-dev or require. I've updated my test scenarios and removed the $isDev.

I guess I was a bit monofocussed on the reported scenario. 🤷

Anyway, good catch! 👍

Regarding the "P.S."... I tend to always work from my own fork, for various reasons.

  1. I would rather not clutter the main repo with more branches than needed. If anyone clones the main repo, they get the other branches as well. So unless it is a large feature that more developers are working on, I would rather keep it out of the main repo.
  2. I have a habit of keeping my (work) processes as uniform as possible. Since I can't push to any repo of choice, I tend to just always work from my own fork. This also avoids issues when/if I no longer have access to the main repo. By working from my fork, there is no need to change (or even notice) when this happens.
  3. I don't feel any reason why I should, to be honest... I've gotten so used to the distributed nature of git that the "one blessed repository" with a bunch of developer pushing to it feels like an anti-pattern. In FOSS this is less of an issue, but in a corporate setting I have (almost always) seen this lead to situations where people don't know if certain branches can be deleted or not. It creates overhead, so it feels like an anti-pattern to me.
jrfnl commented 4 years ago

Thanks @Potherca for sorting this out.

I think I will still open an issue in the Composer repo to ask about the principle of plugins running after they've been uninstalled. Depending on the answer there, we can iterate on this further.

Re: the "P.S.":

Thanks for the extensive answer. I was mostly wondering about it as it makes (manual) testing by other committers or even by casual watchers of the repo more awkward.

To be able to test this PR, as you will no doubt be aware, I had to add a new remote + add a repositories vcs directive to the composer.json for the test scenario I was using. If the branch had been in this repo, neither would have been necessary.

As PR branches should "normally" be short-lived, I'm not too concerned about branches being added to a repo, especially as it is now possible for them to be auto-deleted on merge (setting in the repo settings).

I'm not trying to persuade you to change your workflow. Just wanted to give you my perspective.