Fix incorrect handling of promises during package reset#124
Conversation
…ng is achieved but without needing to pass Composer instance through different function parameters, should solve #118
…ing-of-promises-during-package-reset
| throw new OperationFailure(sprintf('Download of %s failed', $package->getName())); | ||
| } | ||
|
|
||
| $promise = \React\Promise\all([$uninstallPromise, $downloadPromise]); |
There was a problem hiding this comment.
Wouldn't this still allow for $downloadPromise to complete before $uninstallPromise does? Personally I would make both promises resolve sequentially before moving on to the next step.
There was a problem hiding this comment.
Correct, although I'm not seeing this as a problem since these two should be independent processes from each other, while installation depends on download and uninstallation having happened first. I could still investigate if this can be an issue though.
There was a problem hiding this comment.
Confirmed, the order of download and uninstallation doesn't appear to matter. The uninstall only applies to the installed package, the download of the package is done to a different folder. So the two can be executed in any order and even in parallel.
…ing-of-promises-during-package-reset
jooname
deleted the
fix/incorrect-handling-of-promises-during-package-reset
branch
2 participants
Alternative solution to #119, related to issue #118
Main difference is that in
processReinstallOperationinstead of passing Composer instance through couple of other classes to allow callingComposer\Util\SyncHelper::downloadAndInstallPackageSync, we useReact\Promise\allto combine handling of uninstall and download promises and once that combined promise finishes, then we handle the install promise. I did try out simply chaining the promises, but even that way it seems like first two promises are the only ones that get processed before patching. Now when the first two are combined, the installation promise happens before patching.The solution in #119 also works, but I'm a bit hesitant to modify the public method parameters to pass additional information, especially when there is already quite a few parameters in the methods and the information needs to be passed through other classes as well. Ideally this would probably be refactored to combine all the parameters to some command/payload object, but I wouldn't go too crazy here.
I also simplified validating the promises, since at least I had issues getting the
instanceofbased check working. In couple of projects I tested this change with, the promise instance was something else that didn't pass the check, even when I tried out with promise interface. This simplified check should be fine, since according to the Composer code the method calls should return either null or promise instance.