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

Move loadInstalledPaths from init to onDependenciesChangedEvent #51

Merged
merged 3 commits into from Oct 25, 2018

Conversation

gapple
Copy link
Contributor

@gapple gapple commented Jul 11, 2018

Proposed Changes

The plugin executes phpcs when initialized, which may cause it to include the autoloader while it's in an inconsistent state.

For example, if a package that includes a static file is removed during an update (e.g. a compat library that provides global PHP functions):

  1. Composer removes the outdated package from the file system
  2. The phpcodesniffer-composer-installer plugin is initialized and attempts to use the autoloader files on disk
  3. The outdated autoloader files on disk require the file from the removed package, and the composer update / install fails with a fatal error.

This PR moves the initialization of installedPaths to the method which responds to the POST_INSTALL_CMD and POST_UPDATES_CMD events, so that it is only initialized just before it's actually needed.

Related Issues

Fixes #49

@Potherca
Copy link
Member

The failing build on Travis is caused by an issue with the security-checker.

It uses code that is only PHP7.1+ compatible.

@gapple
Copy link
Contributor Author

gapple commented Jul 12, 2018

I've tagged security checker to the last release that supported PHP 5, and the Travis build is now successful.

Would it be better (and possible) to manage the version of security-checker via require-dev in composer.json?

@Potherca
Copy link
Member

I would be all for it but that is up to @frenck as I am no longer part of the organisation.

(I could ask @frenck to add me as a contributor... then I could make more contributions).

@frenck
Copy link
Contributor

frenck commented Sep 9, 2018

This seems to be a sane fix for the issue at hand. I will test this one asap.

@frenck frenck self-assigned this Sep 9, 2018
mickaelperrin pushed a commit to mickaelperrin/phpcodesniffer-composer-installer that referenced this pull request Sep 14, 2018
@Potherca Potherca added this to the Next Major Release - v0.5.0 milestone Oct 15, 2018
@frenck
Copy link
Contributor

frenck commented Oct 25, 2018

@gapple About the security checker as a dependency, I'm kinda in the middle of this.
IMHO, security package checking is part of a CI and not part of a development workflow (e.g., GitHub is now moving towards providing that kind of things).

So I'm fine with both solutions tbh.

This has been changed by @jrfnl in #58.

@frenck frenck merged commit d2042de into PHPCSStandards:master Oct 25, 2018
@frenck frenck mentioned this pull request Oct 25, 2018
@gapple gapple deleted the autoloader-bug branch June 3, 2021 19:09
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.

None yet

3 participants