This repository has been archived by the owner on Jan 21, 2020. It is now read-only.
Fixes composer installation errors due to problematic configuration files #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…iles If a module configuration file contains elements that depend on autoloading and/or elements that are clearly not configuration, calling `include` on it can lead to failure to complete a composer installation/removal action. This patch modifies the logic to tokenize the configuration file and look for problematic elements; when encountered, the (un)installer will skip including that configuration file, and instead emit a warning.
.travis.yml
Outdated
| @@ -47,18 +48,22 @@ matrix: | |||
| - php: hhvm | |||
|
|
|||
| before_install: | |||
| - if [[ $TRAVIS_PHP_VERSION != "hhvm" && $TEST_COVERAGE != 'true' ]]; then phpenv config-rm xdebug.ini ; fi | |||
| - if [[ $TEST_COVERAGE != 'true' ]]; then phpenv config-rm xdebug.ini || return 0 ; fi | |||
| - travis_retry composer self-update | |||
There was a problem hiding this comment.
This line is redundant. Travis update composer automatically, please see:
https://travis-ci.org/zfcampus/zf-asset-manager/jobs/296348988#L473
https://travis-ci.org/zfcampus/zf-asset-manager/jobs/296348988#L478
…_manager configuration If a configuration file does not contain `asset_manager` configuration, we do not need to do anything with it in the first place, and do not need to emit warnings. If the file contains any class pseudo-constant usage (e.g. `AssetInstallerFactory::class`), we do not need to flag the file as unusable, as PHP will resolve these regardless of autoloading. As such, we'll only have issues with class constants, static members, and cases where we are calling `new` or extending or implementing classes or interfaces.
src/UnparseableTokensTrait.php
Outdated
| ]; | ||
|
|
||
| /** | ||
| * @var string $packageConfigPath |
src/UnparseableTokensTrait.php
Outdated
| } | ||
|
|
||
| /** | ||
| * @var string $packageConfigPath |
There was a problem hiding this comment.
As above, @param instead of @var
…dumping Per conversations with @Xerkus, this patch modifies the `Plugin` class to create closures around each install, uninstall, and update event in order to delay their execution until the `post-autoload-dump` event executes. What that event executes, it will: - loop through all uninstall closures and execute them. - loop through all install closures and execute them. This approach should solve autoloading issues (e.g., when resolving class constants).
Uninstall operations should not suffer the same autoloading issues, as the autoloader will already be in place. However, if they run during post-autoload-dump, they _will_ have issues. As such, this commit modifies the logic to run uninstall operations immediately, instead of memoizing them to run later.
weierophinney
commented
Nov 2, 2017
src/Plugin.php
Outdated
| } | ||
|
|
||
| /** | ||
| * Updates assets provided by the package, if any. | ||
| * | ||
| * Uninstalls any previously installed assets for the package, and then | ||
| * runs an install for the package. | ||
| * memoizes an install operation to run post-autoload-dump. |
There was a problem hiding this comment.
No typo: read about memoization on wikipedia. 😄
Xerkus
reviewed
Nov 2, 2017
src/UnparseableTokensTrait.php
Outdated
| } | ||
|
|
||
| // ::class notation is okay | ||
| if ($token[0] === T_DOUBLE_COLON |
Xerkus
reviewed
Nov 2, 2017
src/UnparseableTokensTrait.php
Outdated
| * @param string|array Token to test as a class pseudoconstant | ||
| * @return bool | ||
| */ | ||
| private function isClassPseudoConstant($token) |
Moves the uninstall operations originally in the post-package-update hook to a pre-package-update hook. This should autoloading stays the same (in cases where classes, class constants, config files, etc. disappear).
Prevents deprecation notices.
weierophinney
added a commit
that referenced
this pull request
Nov 2, 2017
weierophinney
added a commit
that referenced
this pull request
Nov 2, 2017
Forward port #5 Conflicts: CHANGELOG.md
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If a module configuration file contains elements that depend on autoloading and/or elements that are clearly not configuration, calling
includeon it can lead to failure to complete a composer installation/removal action.This patch modifies the logic as follows:
asset_managerconfiguration, it is skipped.exec()andeval()calls; these raise warnings by the installer if detected.Pluginclass now aggregates install operations as closures around the relevant events, and executes these during apost-autoload-dumpplugin hook; this fixes autoloading issues. Uninstall operations still occur immediately as the relevant uninstall and update events.Fixes #4