Ignore the global plugin when updating local projects #39

AydinHassan · 2016-10-26T16:44:29Z

I had to refactor a little to reduce repeating code, anyway this fixes #38.

However, I've had a think and I don't know how we can provide an e2e test. We could provide some project assets but we'd have to mess with the systems global composer project, which I don't think we can do safely.

I've also had to modify existing tests, I wasn't sure how to get them working now that we need ocramius/package-versions to be present in the locker.

Thoughts @Ocramius ?

AydinHassan · 2016-10-26T17:26:10Z

This patch also has the added benefit of fixing another crash. After removing ocramius/package-versions the plugin still runs an attempts to generate the version file, but the folder does not exist anymore.

Running: composer remove ocramius/package-versions (before this patch) results in:

Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Removing ocramius/package-versions (1.1.1)
Package guzzle/guzzle is abandoned, you should avoid using it. Use guzzlehttp/guzzle instead.
Writing lock file
Generating autoload files
ocramius/package-versions:  Generating version class...

Removal failed, reverting ./composer.json to its original content.


  [ErrorException]                                                                                                                                                               
  file_put_contents(/Users/aydin/projects/package-versions-test/vendor/ocramius/package-versions/src/PackageVersions/Versions.php): failed to open stream: No such file or directory  


remove [--dev] [--no-progress] [--no-update] [--no-scripts] [--update-no-dev] [--update-with-dependencies] [--no-update-with-dependencies] [--ignore-platform-reqs] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--] [<packages>]...

Admittedly this can be circumvented using --no-plugins but I think this is slightly more elegant and provides the consumer with no surprises.

AydinHassan · 2016-10-26T19:43:51Z

@Ocramius managed to get an E2E test working - it's a bit gross so I'd appreciate any pointers for it 😄

Ocramius · 2016-10-29T12:54:04Z

src/PackageVersions/Installer.php


-        $composer = $composerEvent->getComposer();
+        $io = $composerEvent->getIO();
+        $io->write('<info>ocramius/package-versions:</info>  Generating version class...');


I'll add an info message in the previous block as well. Otherwise, users may get confused on why this isn't running.

Ocramius · 2016-10-29T12:54:43Z

test/PackageVersionsTest/E2EInstallerTest.php

+/**
+ * @author Aydin Hassan <aydin@hotmail.co.uk>
+ */
+class E2EInstaller extends PHPUnit_Framework_TestCase


Going to rename/move this class around. Also will add @coversNothing

Sure thing - wasn't really sure how to name it

Ocramius · 2016-10-29T12:54:49Z

test/PackageVersionsTest/E2EInstallerTest.php

+ */
+class E2EInstaller extends PHPUnit_Framework_TestCase
+{
+    private $tempGlobalComposerHome;


To be added: docblocks

Ocramius · 2016-10-29T12:55:12Z

test/PackageVersionsTest/E2EInstallerTest.php

+    {
+        $this->createPackageVersionsArtifact();
+
+        file_put_contents($this->tempGlobalComposerHome . '/composer.json', json_encode([


This alignment is not PSR-2 compliant

fixed up - I introduced a method to write

Ocramius · 2016-10-29T12:55:17Z

test/PackageVersionsTest/E2EInstallerTest.php

+        $this->exec(__DIR__ . '/../../vendor/bin/composer global update');
+
+        $this->createArtifact();
+        file_put_contents($this->tempLocalComposerHome . '/composer.json', json_encode([


Ocramius · 2016-10-29T13:00:09Z

test/PackageVersionsTest/E2EInstallerTest.php

+                        $filePath = substr($file->getRealPath(), strlen(realpath(__DIR__ . '/../../')) + 1);
+
+                        if (substr($filePath, 0, 4) === '.git'
+                            || substr($filePath, 0, 5) === '.idea'


Simply skip anything that starts with .

There's something in the recursive directory iterator for that, btw

You got a link? I couldn't find anything other than SKIP_DOTS which doesn't consider anything other than .. & . I've simplified it now anyway - so should be better

Ocramius · 2016-10-29T13:00:37Z

test/PackageVersionsTest/E2EInstallerTest.php

+                    function (SplFileInfo $file) {
+                        $filePath = substr($file->getRealPath(), strlen(realpath(__DIR__ . '/../../')) + 1);
+
+                        if (substr($filePath, 0, 4) === '.git'


This block is a negated boolean expression, not an if/else block

I've simplified this a little

Ocramius · 2016-10-29T13:01:28Z

test/PackageVersionsTest/E2EInstallerTest.php

+        $zip->addFromString('composer.json', json_encode([
+             'name'    => 'test/package',
+             'version' => '2.0.0',
+         ], JSON_PRETTY_PRINT)).


Alignment broken here too

Ocramius · 2016-10-29T13:01:58Z

test/PackageVersionsTest/E2EInstallerTest.php

+        );
+    }
+
+    private function createPackageVersionsArtifact()


Should this return the path to the generated artifact?

I dont think so - the path to the zip isn't used for anything, just the folder where they reside

Ocramius · 2016-10-29T13:06:48Z

test/PackageVersionsTest/E2EInstallerTest.php

+
+    private function exec(string $command) : array
+    {
+        exec($command . ' 2> /dev/null', $output);


You need to assert that the exit code is 0

Added an assertion in

AydinHassan · 2016-10-30T15:17:37Z

Seems to be failing on lowest deps - trying to figure out now

AydinHassan · 2016-10-30T15:25:20Z

Hmm - the composer exec output seems to print errors such as

Fatal error: Uncaught Error: Class 'Symfony\Component\Process\ProcessUtils' not found in /Users/aydin/projects/PackageVersions/vendor/composer/composer/src/Composer/Util/ProcessExecutor.php on line 124

On further investigation I would say this is a bug with Composer:

\Composer\Util\ProcessExecutor uses Symfony\Component\Process\ProcessUtils from symfony/process. Composer constraint on symfony/process is "^2.1 || ^3.0" but it seems ProcessUtils was only introduced in 2.3

Ocramius · 2016-10-31T00:51:58Z

On further investigation I would say this is a bug with Composer:

We should then report it in composer and bump the required version there, I suppose...

…omposer. Refs: composer/composer#5828

AydinHassan · 2016-10-31T08:57:52Z

I've filed composer/composer#5828 & created composer/composer#5831 to fix it (maybe). I've bumped the minimum in here to ^2.3 also - should get the tests running.

Ocramius · 2016-10-31T08:57:59Z

composer.json

@@ -16,7 +16,8 @@
    "require-dev": {
        "phpunit/phpunit":   "^5.4.7",
        "composer/composer": "^1.2.0",
-        "ext-zip": "*"
+        "ext-zip":           "*",
+        "symfony/process":   "^2.3"


Note: this required should be pushed into composer

AydinHassan · 2016-11-03T10:25:50Z

OK composer/composer#5831 is merged now - guessing you want to wait for a tag before we pursue this?

AydinHassan · 2016-12-25T16:14:09Z

hey @Ocramius composer 1.3 has been released with the necessary fixes. Tests are passing now 😄 .

Ocramius · 2016-12-28T13:59:22Z

@AydinHassan looking good! Going through a quick sweep of the code, then merging :-)

Ocramius · 2016-12-28T14:00:54Z

src/PackageVersions/Installer.php


-        $io->write('<info>ocramius/package-versions:</info>  Generating version class...');
+        if (!isset($versions['ocramius/package-versions'])) {
+            //plugin must be globally installed - we only want to generate versions for projects which specifically


@AydinHassan just a doubt, given that there's no real way to know from the code: what about when the package is indirectly depending on package-versions? Will it still run?

Also: what about when we are currently operating in package-versions?

@Ocramius will run in both contexts as it would be in the lock file - is that expected?

The first is actually my use case - A globally installed tool depends on PackageVersions.

@AydinHassan great, thanks!

Ocramius · 2016-12-28T14:01:36Z

src/PackageVersions/Installer.php


-        $io->write('<info>ocramius/package-versions:</info>  Generating version class...');
+        if (!isset($versions['ocramius/package-versions'])) {


Should probably be an array_key_exists() at this point - don't trust the value below it

Ocramius · 2016-12-28T14:02:20Z

test/PackageVersionsTest/E2EInstallerTest.php

+        mkdir($this->tempLocalComposerHome, 0777, true);
+        mkdir($this->tempArtifact, 0777, true);
+
+        putenv('COMPOSER_HOME=' . $this->tempGlobalComposerHome);


This environment variable must be reset in tearDown

Ocramius · 2016-12-28T14:02:35Z

test/PackageVersionsTest/E2EInstallerTest.php

+        $this->tempGlobalComposerHome = sys_get_temp_dir() . '/' . uniqid('InstallerTest', true) . '/global';
+        $this->tempLocalComposerHome = sys_get_temp_dir() . '/' . uniqid('InstallerTest', true) . '/local';
+        $this->tempArtifact = sys_get_temp_dir() . '/' . uniqid('InstallerTest', true) . '/artifacts';
+        mkdir($this->tempGlobalComposerHome, 0777, true);


Please use 0700 for test runs

Ocramius · 2016-12-28T14:02:37Z

test/PackageVersionsTest/E2EInstallerTest.php

+        $this->tempLocalComposerHome = sys_get_temp_dir() . '/' . uniqid('InstallerTest', true) . '/local';
+        $this->tempArtifact = sys_get_temp_dir() . '/' . uniqid('InstallerTest', true) . '/artifacts';
+        mkdir($this->tempGlobalComposerHome, 0777, true);
+        mkdir($this->tempLocalComposerHome, 0777, true);


Please use 0700 for test runs

Ocramius · 2016-12-28T14:02:41Z

test/PackageVersionsTest/E2EInstallerTest.php

+        $this->tempArtifact = sys_get_temp_dir() . '/' . uniqid('InstallerTest', true) . '/artifacts';
+        mkdir($this->tempGlobalComposerHome, 0777, true);
+        mkdir($this->tempLocalComposerHome, 0777, true);
+        mkdir($this->tempArtifact, 0777, true);


Please use 0700 for test runs

Ocramius · 2016-12-30T06:44:56Z

LGTM! \o/

weirdan · 2017-11-11T23:27:38Z

@samsonasik are there chances this will get backported to your fork? There's no way to report issues on forks, unfortunately.

samsonasik · 2017-11-12T06:22:06Z

@weirdan I tried backport but it seems needs very long time to get it work as I state at samsonasik#3 (comment) . Pull Request open if you can invest a time to patch ;)

Ocramius · 2017-11-12T15:39:48Z

I know @samsonasik did the backport with good intentions, but if you want to talk about older PHP versions support here, I'm gonna lock the thread: just upgrade your installation to a contemporary PHP binary.

weirdan · 2017-11-12T18:40:17Z

Oh, we were merely talking about porting changes from upstream to the fork. And I'm running 7.1, which, last time I checked, was pretty contemporary.

Ocramius · 2017-11-12T19:17:12Z

Then you don't need the fork ;-)

Ignore the global plugin when updating local projects

1389111

AydinHassan force-pushed the ignore-global-plugin-in-local branch from 89d18a5 to 1389111 Compare October 26, 2016 16:48

E2E tests for global plugin

bb06fb0

Add E2E test for removing plugin

c2b19bc

Ocramius added the bug label Oct 26, 2016

Ocramius added this to the 1.1.2 milestone Oct 26, 2016

Ocramius requested changes Oct 29, 2016

View reviewed changes

AydinHassan added 9 commits October 30, 2016 12:47

require ext-zip in dev

f41e139

@coversNothing for e2e installer tests

e33915c

class var docblocks

287fd91

alignment

b0e1aae

Use composer from vendor dir

f12ab08

Simplify iterator

7ec5c0e

Assert exec exit code

5c62f98

fix alignment and introduce method for writing composer files

ef6de8c

refactor exec

10e40bd

AydinHassan mentioned this pull request Oct 30, 2016

symfony/process constraint seems to be incorrect composer/composer#5828

Closed

Bump symfony/process to ^2.3 which is actually minimum for composer/c…

19da477

…omposer. Refs: composer/composer#5828

Ocramius reviewed Oct 31, 2016

View reviewed changes

AydinHassan added 2 commits November 3, 2016 17:29

bump composer version

3879b3c

bump composer to 1.3

c054b5e

Ocramius requested changes Dec 28, 2016

View reviewed changes

AydinHassan added 3 commits December 29, 2016 10:35

array_key_exists instead of isset

c03754c

0700 permissions

b0e4201

Unset COMPOSER_HOME env variable in tearDown

0180eab

Ocramius merged commit ea07d4e into Ocramius:master Dec 30, 2016

AydinHassan deleted the ignore-global-plugin-in-local branch January 2, 2017 21:32

weirdan mentioned this pull request Nov 11, 2017

Adding a --version argument to the Psalm executable. vimeo/psalm#226

Merged


		$io->write('<info>ocramius/package-versions:</info> Generating version class...');
		if (!isset($versions['ocramius/package-versions'])) {

Ignore the global plugin when updating local projects #39

Ignore the global plugin when updating local projects #39

Conversation

AydinHassan commented Oct 26, 2016 • edited Loading

AydinHassan commented Oct 26, 2016 • edited Loading

AydinHassan commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AydinHassan Oct 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AydinHassan commented Oct 30, 2016

AydinHassan commented Oct 30, 2016 • edited Loading

Ocramius commented Oct 31, 2016

AydinHassan commented Oct 31, 2016

Choose a reason for hiding this comment

AydinHassan commented Nov 3, 2016

AydinHassan commented Dec 25, 2016

Ocramius commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Dec 30, 2016

weirdan commented Nov 11, 2017

samsonasik commented Nov 12, 2017

Ocramius commented Nov 12, 2017

weirdan commented Nov 12, 2017

Ocramius commented Nov 12, 2017

AydinHassan commented Oct 26, 2016 •

edited

Loading

AydinHassan commented Oct 26, 2016 •

edited

Loading

AydinHassan Oct 30, 2016 •

edited

Loading

AydinHassan commented Oct 30, 2016 •

edited

Loading