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

Fix for issue #103 #104

Merged

Conversation

Potherca
Copy link
Member

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

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.

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

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Potherca Thanks for setting up this fix.

I think it needs to be expanded to cover both dev as well as non-dev requirements.

While installing the plugin as a dev requirement will be the more common case, IMO a fix like this should also cover the less likely case where the plugin is installed via require.

I've ran some tests with both master as well as this branch and it looks like the issue also occurs in the below situation which is not fixed by this PR:

  1. Blank folder.
  2. composer require wp-coding-standards/wpcs dealerdirect/phpcodesniffer-composer-installer (note: not --dev)
  3. composer remove wp-coding-standards/wpcs dealerdirect/phpcodesniffer-composer-installer -v

Whether this is run on master or using this branch, in both cases, the result is still:

Running PHPCodeSniffer Composer Installer
PHPCodeSniffer is not installed

Exitcode: 1

P.S.: just wondering why you pushed this to your own fork instead of this repo ?

src/Plugin.php Outdated Show resolved Hide resolved
Previously the check would only run if the plugin was require-dev.
That behaviour is incorrect, as the check should always be done.
If the plugin is not installed, it doesn't make sense to return an
error under any circumstances.

This commit fixes that oversight.
@Potherca
Copy link
Member Author

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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested & found working as expected 👍

@jrfnl jrfnl merged commit 2a57571 into PHPCSStandards:master Jan 27, 2020
@jrfnl
Copy link
Member

jrfnl commented Jan 27, 2020

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.

@Potherca Potherca deleted the issue/103-exit-code-on-unistall branch January 27, 2020 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants