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

Phar file cannot be renamed #595

Closed
jakzal opened this issue Mar 17, 2018 · 17 comments
Closed

Phar file cannot be renamed #595

jakzal opened this issue Mar 17, 2018 · 17 comments

Comments

@jakzal
Copy link

jakzal commented Mar 17, 2018

Psalm stops working after renaming the downloaded psalm.phar to psalm. Steps to reproduce below.

environment: php 7.2, os x

curl -Ls https://github.com/vimeo/psalm/releases/download/1.0.3/psalm.phar > psalm
chmod +x psalm
./psalm -h
PHP Warning:  require(phar:///Users/kuba/Projects/foo/psalm/psalm): failed to open stream: phar error: no directory in "phar:///Users/kuba/Projects/foo/psalm/psalm", must have at least phar:///Users/kuba/Projects/foo/psalm/psalm/ for root directory (always use full path to a new phar) in /Users/kuba/Projects/foo/psalm on line 10

Warning: require(phar:///Users/kuba/Projects/foo/psalm/psalm): failed to open stream: phar error: no directory in "phar:///Users/kuba/Projects/foo/psalm/psalm", must have at least phar:///Users/kuba/Projects/foo/psalm/psalm/ for root directory (always use full path to a new phar) in /Users/kuba/Projects/foo/psalm on line 10
PHP Fatal error:  require(): Failed opening required 'phar:///Users/kuba/Projects/foo/psalm/psalm' (include_path='.:') in /Users/kuba/Projects/foo/psalm on line 10

Fatal error: require(): Failed opening required 'phar:///Users/kuba/Projects/foo/psalm/psalm' (include_path='.:') in /Users/kuba/Projects/foo/psalm on line 10
@weirdan
Copy link
Collaborator

weirdan commented Mar 18, 2018

Only affects files without extension. For example, file.phar or file.q work fine.

@weirdan
Copy link
Collaborator

weirdan commented Mar 18, 2018

Might be related: https://bugs.php.net/bug.php?id=76061

@weirdan
Copy link
Collaborator

weirdan commented Mar 18, 2018

Line 10 points to:

require 'phar://' . __FILE__ . '/psalm';

Composer's stub (which does not seem to suffer from this problem) looks a bit different:

Phar::mapPhar('composer.phar');
require 'phar://composer.phar/bin/composer';

@muglug
Copy link
Collaborator

muglug commented Mar 18, 2018

@muglug muglug closed this as completed in 5728ef4 Mar 18, 2018
@muglug
Copy link
Collaborator

muglug commented Mar 18, 2018

I've fixed the generation step and updated the Phar attached to the release. The repro code you pasted above now works.

Thanks for ticketing!

@jakzal
Copy link
Author

jakzal commented Mar 18, 2018

Thank you for your quick reaction! 🍺

@jakzal
Copy link
Author

jakzal commented Mar 18, 2018

I'm getting another issue while running psalm -h though:

./psalm -v

PHP Fatal error:  Uncaught Error: Undefined constant 'PsalmPhar\PSALM_VERSION' in phar:///Users/kuba/Projects/foo/psalm/src/psalm.php:117
Stack trace:
#0 phar:///Users/kuba/Projects/foo/psalm/psalm(4): require_once()
#1 /Users/kuba/Projects/foo/psalm(11): require('phar:///Users/k...')
#2 {main}
  thrown in phar:///Users/kuba/Projects/foo/psalm/src/psalm.php on line 117

@theofidry
Copy link
Contributor

theofidry commented Mar 18, 2018

'PsalmPhar\PSALM_VERSION' looks like a PHP-Scoper issue to me: humbug/php-scoper#175

@muglug the usual workaround for those kind of scoping errors are patchers. You can find some examples in PHP-Scoper and Box themselves.

I couldn't find anything about the build step you're using, but I highly recommend you to add an e2e tests, i.e. a test where you use the psalm bin first and then the PHAR (isolated or not). In my experience it's very easy to have PHAR related issues especially if

  • There is PHAR specific things: commands registered only in PHARs, some placeholders registered...
  • PHP-Scoper is applied: as much as I'm trying to make it work the best out of the box, there will almost always be an issue that can either be patched by a patcher or fixed upstream by making the code more understandable for this kind of process

Edit: actually that's weird as I don't see the PHP-Scoper compactor registered in the config, is the config used to build the uploaded PHAR different?

@muglug
Copy link
Collaborator

muglug commented Mar 18, 2018

@jakzal thanks, I've fixed and updated the release's Phar

@theofidry everything is generated in bin/build-phar.sh. There are a number of scopers that exist in scoper.inc.php, including the one I just added to fix the const issue.

That build tool allows for some testing – I should be able to run a reduced version of Psalm checks on the scoped folder.

Edit: actually that's weird as I don't see the PHP-Scoper compactor registered in the config

Yeah, I ran into a bug when using files-bin in the same directory as compacted files when using the scoper compactor and PHP compactor together, so I broke the compacting out into a separate step and ultimately abandoned the PHP compactor (Psalm relies on docblocks being present in a couple of files, and the savings were negligible post-compression).

@theofidry
Copy link
Contributor

Oh I see. Once you add an e2e test I can have a look at what the issue was with the files-bin unless it's an issue breaking right away the PHAR?

ultimately abandoned the PHP compactor

Completely fair.

Also just in case you missed it, if you do box build --dev then you'll get a dumped directory of what is actually added to the PHAR except the bin files because they are added as it is without any processing.

@muglug
Copy link
Collaborator

muglug commented Mar 18, 2018

@theofidry I've added Psalm checks in 83811e6

There are a couple of issues introduced because the scoper removes a closure docblock (though it's easy to suppress those issues for that file in the custom Psalm config I created.

@muglug
Copy link
Collaborator

muglug commented Mar 18, 2018

And thanks for the awesome tool. Really really amazing.

@theofidry
Copy link
Contributor

There are a couple of issues introduced because the scoper removes a closure docblock (though it's easy to suppress those issues for that file in the custom Psalm config I created.

Sounds like a legit bug to me, PHP-Scoper shouldn't remove any doc block. So whilst you can work around it with patchers I do recommend to open an issue for it :)

And thanks for the awesome tool. Really really amazing.

Glad you enjoy. If there's anything don't hesitate to open an issue for it. Both PHP-Scoper and Box are quite young so I do expect some issues :)

@jakzal
Copy link
Author

jakzal commented Mar 18, 2018

@muglug thanks for working on this! The original issue seems to be back though :(

@muglug
Copy link
Collaborator

muglug commented Mar 18, 2018

Sorry – I patched the phar from the 1.0.3 source in a slightly haphazard way as I got more issues (e.g. -v not working). I've given up, and updated the Phar with dev-master. Everything should work as expected.

@theofidry
Copy link
Contributor

I actually think this is a legit bug on Box. Sure configuring an alias solves the issue, but you shouldn't have to IMO. So here's a fix: box-project/box#92

@jakzal
Copy link
Author

jakzal commented Mar 18, 2018

Works now 👍 cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants