[Help wanted] Add PHAR #319

TomasVotruba · 2018-02-14T19:57:37Z

Closes #177

On the Menu

prepare composer script to install and make phar
prepare own phar build script
make local rector.phar run work
box and php-scoper shown as projects WIP not usable in state to this day

Related Sources

carusogabriel · 2018-02-15T13:08:52Z

bin/rector

@@ -1,5 +1,5 @@
 #!/usr/bin/env php
-<?php declare(strict_types=1);
+<?php


Unfortunatelly it's needed for PHAR to make work.

See PHPUnit:

Process Isolation does not work with PHPUnit PHAR sebastianbergmann/phpunit#2791 (comment)

sebastianbergmann/phpunit@70ccf03

I found a way to revert this 👍

theofidry

👍 on the move. But because the difference between the code actually tested and the code shipped in the PHAR (there's a transformation in-between + you filter some files), I highly recommend to have a couple of end to end tests to ensure the PAHR is working correctly

theofidry · 2018-02-16T12:08:49Z

composer.json

@@ -24,23 +24,24 @@
        "webmozart/assert": "^1.2"
    },
    "require-dev": {
+        "humbug/box": "^3.0@dev",


If you are using bamarni/composer-bin-plugin I would recommend you to install humbug/box with it as the later doesn't need to execute any of your code (same for php-scoper)

How would you automate that to don't bother user with that?
I've added it to composer.json script, but it takes too much perfromance.

you could do composer bin {box|php-scoper} install in your composer script. Even if unnecessary once already installed it's like not even half a second. Alternatively I'm using Makefiles lately for this kind of stuff. You can see some examples on alice, AliceDataFixture, php-scoper or box: just do make test or make build and everything is taken care of

That would still bother the user.

Maybe add bash script, that would check if vendor-bin directory for package exist, and install it only if not. What do you think?

Yeah you can definitely have a compile bash script or something instead.

Remember most of your users/contributors won't have to build the PHAR so I don't think it's gonna bother them much. But otherwise a bash/Makefile gives more flexibility on how to do things

I'll look on it.

Remember most of your users/contributors won't have to build the PHAR so I don't think it's gonna bother them much. But otherwise a bash/Makefile gives more flexibility on how to do things

Speed matter for us as well.

Yeah but I think the time taken by composer to dump the autoloader (which is the only thing it will do if the packages are already installed) is negligible compared to the rest.

IMO the places of this process where perf. can be improved:

The scoping itself, i.e. the core of PHP-Scoper

PHP-Scoper as standalone as we could leverage parallelism for example. But I don't think I'll invest much time on it tbh. I'm more interesting in integrating with Box which case the parallelisation already happens there

Box itself

PHP-Parser

Any optimisation in any of those above is more likely to have a higher impact that the half second spent on dumping the autoloader.

That said if it's bugging you, there's nothing wrong with using bash/Makefile to handle this

theofidry · 2018-02-16T12:09:27Z

composer.json

-        "phpstan": "vendor/bin/phpstan.phar analyse packages src tests --level max --configuration phpstan.neon"
+        "phpstan": "vendor/bin/phpstan.phar analyse packages src tests --level max --configuration phpstan.neon",
+        "build-phar": [
+            "vendor/bin/php-scoper add-prefix --output-dir=build --force",


I would remove the dev dependencies first there

I did that before but:

it took so much time to remove, install dev, then install all

it's covered by finder already

it's not possible due to php-cs-fixer, that has /src file in /tests dir; which fails on composer dump

The Finder doesn't cover that. You are ignoring a library test files with the current config but not the whole dev library.

Yeah PHP-CS-Fixer is a bit annoying here cf. https://github.com/api-platform/schema-generator/blob/master/scoper.inc.php#L39-L42 and PHP-CS-Fixer/PHP-CS-Fixer#3462

Although you are right you can also ignore this. This is something that will be handled in Box at some point which will be way more performant for PHP-Scoper as well as it will only scope the files filtered by Box

theofidry · 2018-02-16T12:10:36Z

scoper.inc.php

+    // that this does not work with functions or constants neither with classes belonging to the global namespace.
+    //
+    // Fore more see https://github.com/humbug/php-scoper#whitelist
+    'whitelist' => [


I think you can remove that one

theofidry · 2018-02-16T12:12:51Z

box.json

@@ -0,0 +1,16 @@
+{


You can optimise it later but I would recommend to enable compression ("compression": "GZ")

That failed for me before, but now works. Thanks 👍

IIRC this can fail if the PHAR is too big (with the current implementation) but this should be ok unless you have over 10K files

There was missing argument in constructor error, but I can't reproduce it now

TomasVotruba · 2018-02-16T12:14:24Z

So first step - building phar file - is done. Now we need to test it, that it works :)

Any idea for simple test, that could be added to test suite and run on Travis?

TomasVotruba · 2018-02-16T12:18:22Z

@theofidry Thanks for the feedback, much appreciated from the package's maintainer/author.

Do you have some simple idea how to test rector.phar? What is the best practise to do so in the wild?

TomasVotruba · 2018-02-16T12:24:52Z

composer.json

-        "phpstan": "vendor/bin/phpstan.phar analyse packages src tests --level max --configuration phpstan.neon"
+        "phpstan": "vendor/bin/phpstan.phar analyse packages src tests --level max --configuration phpstan.neon",
+        "build-phar": [
+            "composer bin php-scoper require --dev humbug/php-scoper",


@theofidry Here is the bin install used

theofidry · 2018-02-16T12:50:57Z

You're welcome.

My recommendation for testing is to have a couple of e2e tests, i.e. tests where you run your application with the binary (be it from the source bin/rector or the PHAR) as a black box and test the output. You can then easily switch between the binary of the PHAR when you need to debug.

The way I did it with PHP-Scoper is relatively simple: I have a few directories where you have the classes/composer.json and stuff and a file containing the expected output. I run PHP-Scoper and Box to build a PHAR and test if the output is the same as the expected one. You can find the examples there.

For box the e2e testing is a bit lighter, I have a few integration tests to make sure Box works and for testing the PHAR I simply try to build the Box PHAR with the PHAR. In other words I generate the PHAR once (with the regular binary bin/box) and a second time with the generated PHAR. See this for more details

TomasVotruba · 2018-02-16T16:01:10Z

@theofidry Thank you, that seems like great simple trick! I'll investigave those sources.

carusogabriel · 2018-02-16T21:09:59Z

tests/Phar/PharTest.php

+{
+    public function test(): void
+    {
+        $this->assertTrue(file_exists(__DIR__ . '/../../rector.phar'));


Maybe assertFileExists?

This is WIP. I have new problems with buidling phar :/

I'll follow your discussion with @theofidry and see what I can help 😄

TomasVotruba · 2018-02-17T13:02:02Z

@theofidry I've added detailed steps with prefixing and phar building to Travis to show the error.

I have no idea how to fix that or what it means, being stuck for hours.

Could you give me some hints? It would save me

https://travis-ci.org/rectorphp/rector/builds/342723839#L817

theofidry · 2018-02-17T13:31:51Z

I'll have a look in a bit

theofidry · 2018-02-17T14:08:05Z

Actually maybe a bit later. I need to fix a couple of things in php-scoper and box but that won't take too long. I'll have a look at Rector today still

TomasVotruba · 2018-02-17T14:49:29Z

@theofidry No problem, thank you for any help you find time for

TomasVotruba · 2018-02-17T15:26:59Z

My last finding...

I run php-scoper with output to /build directory
Then I run composer dump

composer dump --working-dir=build

But no classes from build/vendor is loaded.

I see in every composer.json that instead of package/name there is always extra \ char: package\/name.
I tried to remove them as well, but it didn't help.

theofidry · 2018-02-17T15:31:08Z

I'm checking that now :)

TomasVotruba · 2018-02-17T16:45:49Z

Is it possible, that composer dump loads only packages from main composer.json and skips all those from /vendor?

theofidry · 2018-02-17T16:50:22Z

No that part definitely works. I think I got a lead on the issue it's just frigging slow (the scoping takes 1-2min each time...)

theofidry · 2018-02-17T17:13:08Z

Ok I found too bugs I'll need to fix in PHP-Scoper:

When collecting the files, I retrieve the finders and merge them in a AppendIterator(). First I should ditch it for nikic/inter::chain(). Second there's an issue when appending files. Indeed if you use Finder::append([]) twice, for both the key will be a numeric key so when merging the two finders one element will be lost.
Second is more serious but it's good to have found it. When scoping a file, before prefixing anything I resolve the names/symbols into their fully-qualified form whenever possible. For example for:

use Foo\Bar;

$x = new Bar();

The name resolver will resolve the 'Bar' for the statement into '\Foo\Bar'. Then it's more stuff is done to know if the name should be prefixed or not.

The bug here is that the name resolver doesn't take into account the types. It looks at the namespace and use statement, but without any further check, so if you have:

use Foo\Bar();

bar(); // Call the function not the class

It will work in PHP (provided bar is registered properly), but the name resolver will assume the use statement above is for it so will resolve bar into \Foo\bar which is a mistake since it's a class use statement not a function use statement. (The case mismatch is not an issue as PHP is case insensitive for this sort of stuff)

You can see some of my changes there. If you have any question regarding them feel free to ask

theofidry · 2018-02-17T20:22:23Z

Yeah arrays are not supported as well yet I should add it. That said prefixing the autoloader is not supported and won't ever, it requires serious levels of hacking and a tight coupling to Composer internals

TomasVotruba · 2018-02-17T21:45:59Z

I see, I guess this would be fixed by the other 2 issues you mentioned.

theofidry · 2018-02-18T16:33:04Z

Did some PRs so it should work better now. Also the patcher I registered shouldn't be needed anymore either

TomasVotruba · 2018-02-18T18:05:44Z

Thanks for the work!

When I rerun the whole script It fails on PHP error for me:

PHP Fatal error:  Namespace declaration statement has to be the very first statement or after any declare call in the script in phar:///var/www/Rector/rector.phar/bin/rector on line 3

Related code in Phar bin/rector looks like this:

#!/usr/bin/env php
<?php # ← here is extra space, that might cause that, at least what I saw in Stackoverflow answer
namespace PhpScoper5a89bf1332dcc;

theofidry · 2018-02-19T01:57:59Z

That’s weird, it’s not what I’m getting on my machine at all. Do you try my bin/compile?

…

On Sun 18 Feb 2018 at 18:05, Tomáš Votruba ***@***.***> wrote: Thanks for the work! When I rerun the script <https://github.com/rectorphp/rector/pull/319/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R71> It fails on PHP error for me: PHP Fatal error: Namespace declaration statement has to be the very first statement or after any declare call in the script in phar:///var/www/Rector/rector.phar/bin/rector on line 3 Related code in Phar bin/rector looks like this: #!/usr/bin/env php<?php # ← here is extra space, that might cause that, at least what I saw in Stackoverflow answernamespace PhpScoper5a89bf1332dcc; — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#319 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE76gYC7t-OkIJnzuhNxpwhyXZe2leGnks5tWGZ5gaJpZM4SF4oO> .

TomasVotruba · 2018-02-19T07:42:29Z

I run the script I linked. What should I run instead?

theofidry · 2018-02-19T09:49:43Z

No that’s what I did as well. Very strange

…

On Mon 19 Feb 2018 at 07:42, Tomáš Votruba ***@***.***> wrote: I run the script I linked. What should I run instead? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#319 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE76gSS1Fl9omYoWJp5IQ7QrUw84Y8vpks5tWSXlgaJpZM4SF4oO> .

mssimi · 2018-03-27T18:30:59Z

Hey @TomasVotruba , what is missing to make it work? Prefixing?

TomasVotruba · 2018-03-27T18:41:42Z

Hey, well, build doesn't work yet neither.

mssimi · 2018-03-27T18:45:55Z

Building phar itself works for me.

TomasVotruba · 2018-03-27T18:55:55Z

@mssimi Really? What does it mean?

We can close this then. Could you send new PR please?

mssimi · 2018-03-27T18:58:00Z

@TomasVotruba can we discuss it on google hangouts?

TomasVotruba · 2018-03-27T19:18:03Z

@mssimi Sure 👍

mssimi · 2018-03-27T20:48:26Z

@TomasVotruba check this and let me know what you think.

TomasVotruba · 2018-03-27T21:10:01Z

@mssimi That looks awesome 👍 I just love green colour :D PR please!

mssimi · 2018-03-28T19:39:58Z

@TomasVotruba I was loading wrong autoloader. Now I noticed problem which we need to solve to make it work. Method glob() does not work in PHAR and Symfony uses it for autowiring... I am looking for alternatives, we will have to replace all occurrences to make it work:(

mssimi · 2018-03-28T20:33:55Z

Symfony is definitely not compatible with PHAR, Loaders use glob, Kernel uses glob, Finder uses glob. What do you propose @TomasVotruba to overcome this issue?

TomasVotruba · 2018-03-28T22:53:41Z

Not sure, after bit Googling: https://github.com/cakephp/phinx/pull/1042/files

mssimi · 2018-03-29T04:57:58Z

I found this solution too, but Symfony logic is more complex as it uses patterns, we will have to re-implement this class.

Maybe we can discuss it on Symfony's github.

TomasVotruba · 2018-03-29T07:39:13Z

That is doable. I do simalar changes in ECS: https://github.com/Symplify/Symplify/blob/master/packages/EasyCodingStandard/src/DependencyInjection/EasyCodingStandardKernel.php#L96

bendavies · 2018-03-29T12:06:03Z

have just built this phar manually and it work on my symfony project! performance is a problem, but that's another issue i guess.

mssimi · 2018-03-29T12:17:13Z

@bendavies How you solved autowiring issue?

bendavies · 2018-03-29T12:18:36Z

I'm not seeing any...

mssimi · 2018-03-29T12:19:35Z

Are you sure PHAR is autoloading files inside PHAR not outside?

bendavies · 2018-03-29T12:20:33Z

no idea. I just know it's working and upgrading my symfony 2.8 project to 3.0 very nicely...

mssimi · 2018-03-29T12:21:37Z

Maybe you could provide PHAR you compiled so we can check it.

bendavies · 2018-03-29T12:22:25Z

i checked out this branch and ran packages/PharBuilder/bin/compile. that's it

mssimi · 2018-03-29T12:24:16Z

This branch is not working yet, compilator does not include vendor at all, you actually probably run non-phar files.

mssimi · 2018-03-29T12:26:08Z

Same thing happend to me.

bendavies · 2018-03-29T12:31:31Z

you are right. it didn't autoload from the phar.

TomasVotruba · 2018-04-03T07:50:46Z

Continues in #389

rectorphp/rector-src@1a87a3d [TypeDeclaration] Prevent checking class like has external fully qualifieds when class like is null (#319)

…ifieds when class like is null (rectorphp#319)

TomasVotruba changed the title ~~Add PHAR~~ [WIP] Add PHAR Feb 14, 2018

carusogabriel reviewed Feb 15, 2018

View reviewed changes

TomasVotruba mentioned this pull request Feb 16, 2018

[Install] Conflict with PHP-Parser #258

Closed

theofidry reviewed Feb 16, 2018

View reviewed changes

TomasVotruba commented Feb 16, 2018

View reviewed changes

TomasVotruba force-pushed the phar-shim branch from a0ea591 to ede205d Compare February 16, 2018 17:10

carusogabriel previously requested changes Feb 16, 2018

View reviewed changes

TomasVotruba force-pushed the phar-shim branch 2 times, most recently from fcd1fe2 to b62a43e Compare February 17, 2018 12:08

TomasVotruba force-pushed the phar-shim branch from 86ec694 to 95871c4 Compare February 17, 2018 18:39

TomasVotruba mentioned this pull request Mar 17, 2018

Idea - EntityForDoctrineRepositoryProvider automatic provider #368

Closed

mssimi mentioned this pull request Mar 29, 2018

PHAR improvements #389

Closed

TomasVotruba closed this Apr 3, 2018

TomasVotruba deleted the phar-shim branch May 4, 2018 10:07

TomasVotruba added a commit that referenced this pull request Jun 28, 2021

Updated Rector to commit 1a87a3d

5700277

rectorphp/rector-src@1a87a3d [TypeDeclaration] Prevent checking class like has external fully qualifieds when class like is null (#319)

echo511 pushed a commit to echo511/rector that referenced this pull request Sep 4, 2021

[TypeDeclaration] Prevent checking class like has external fully qual…

1a87a3d

…ifieds when class like is null (rectorphp#319)

[Help wanted] Add PHAR #319

[Help wanted] Add PHAR #319

Conversation

TomasVotruba commented Feb 14, 2018 • edited Loading

On the Menu

Related Sources

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry left a comment

Choose a reason for hiding this comment

theofidry Feb 16, 2018 • 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

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

TomasVotruba Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

TomasVotruba commented Feb 16, 2018 • edited Loading

TomasVotruba commented Feb 16, 2018

Choose a reason for hiding this comment

theofidry commented Feb 16, 2018

TomasVotruba commented Feb 16, 2018

Choose a reason for hiding this comment

TomasVotruba Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba commented Feb 17, 2018

theofidry commented Feb 17, 2018

theofidry commented Feb 17, 2018

TomasVotruba commented Feb 17, 2018

TomasVotruba commented Feb 17, 2018 • edited Loading

theofidry commented Feb 17, 2018

TomasVotruba commented Feb 17, 2018

theofidry commented Feb 17, 2018

theofidry commented Feb 17, 2018 • edited Loading

theofidry commented Feb 17, 2018

TomasVotruba commented Feb 17, 2018

theofidry commented Feb 18, 2018 • edited Loading

TomasVotruba commented Feb 18, 2018 • edited Loading

theofidry commented Feb 19, 2018 via email

TomasVotruba commented Feb 19, 2018

theofidry commented Feb 19, 2018 via email

mssimi commented Mar 27, 2018

TomasVotruba commented Mar 27, 2018

mssimi commented Mar 27, 2018

TomasVotruba commented Mar 27, 2018

mssimi commented Mar 27, 2018

TomasVotruba commented Mar 27, 2018

mssimi commented Mar 27, 2018

TomasVotruba commented Mar 27, 2018 • edited Loading

mssimi commented Mar 28, 2018

mssimi commented Mar 28, 2018

TomasVotruba commented Mar 28, 2018

mssimi commented Mar 29, 2018 • edited Loading

TomasVotruba commented Mar 29, 2018

bendavies commented Mar 29, 2018

mssimi commented Mar 29, 2018

bendavies commented Mar 29, 2018

mssimi commented Mar 29, 2018

bendavies commented Mar 29, 2018

mssimi commented Mar 29, 2018

bendavies commented Mar 29, 2018

mssimi commented Mar 29, 2018

mssimi commented Mar 29, 2018

bendavies commented Mar 29, 2018

TomasVotruba commented Apr 3, 2018

TomasVotruba commented Feb 14, 2018 •

edited

Loading

theofidry Feb 16, 2018 •

edited

Loading

TomasVotruba Feb 16, 2018 •

edited

Loading

TomasVotruba commented Feb 16, 2018 •

edited

Loading

TomasVotruba Feb 16, 2018 •

edited

Loading

TomasVotruba commented Feb 17, 2018 •

edited

Loading

theofidry commented Feb 17, 2018 •

edited

Loading

theofidry commented Feb 18, 2018 •

edited

Loading

TomasVotruba commented Feb 18, 2018 •

edited

Loading

TomasVotruba commented Mar 27, 2018 •

edited

Loading

mssimi commented Mar 29, 2018 •

edited

Loading