Skip to content

Conversation

@theofidry
Copy link
Member

@theofidry theofidry commented Feb 14, 2018

The reasoning here is very simple:

  • I think the web PHARs usage is very limited
  • The current implementation & documentation about it is very limited and incomplete
  • I am not knowledgeable enough to neither ensure this is working correctly or to improve it the way I want

So rather than trying to support and edge case I have no idea how to support, I prefer to remove the feature and simplify things for now. I'm not against supporting it in the future, but at least if it is it will be with the guarantee that I understand it and that the doc & tests are there.

That said here's the complete change with that PR:

  • Remove support for web PHARs which allows us to remove a bunch of additional parameters: mung, mimetypes, notFound & isWeb from the configuration
  • Update the documentation about the current usage of some parameters used for the stub
  • Simplify the configuration & StubGenerator
  • Remove support for PHAR extracting

Todo:

  • Review the doc for the stub related config parameters
  • Review the changelog about BC breaks
  • Review the StubGenerator validation: as of now there is no validation whatsoever. I think this is a mistake as it allows one to generate a complete nonsensical stub which doesn't mean anything in practice

@theofidry theofidry self-assigned this Feb 14, 2018
@TomasVotruba
Copy link

I'd like to help. What is web Phar?

@theofidry
Copy link
Member Author

I actually don't think "web PHAR" is the proper term. What I mean is that a a PHP application you can be used to handle the requests and responses or be used for the CLI. You have exactly the same for PHARs.

However, I think this is introducing a lot of complexity for something that has no longer any practical use... There isn't really any good reason to serve a PHAR instead of a regular application:

  • That would require you to bundle your application in a PHAR which is some extra efforts
  • It's nice to be able to compress your PHAR, but if you do so for the HTTP your service (apache or php-fpm) will need to uncompress your PHAR for each request
  • FTP web-hosting are not a thing anymore

So as much as I appreciate help on this project, unless you have a strong use case for it, I think there's other more interesting things like #13, #8 or humbug/php-scoper#117

@theofidry theofidry merged commit 3224b27 into box-project:master Feb 15, 2018
@theofidry theofidry deleted the rework/stub branch February 15, 2018 01:15
@TomasVotruba
Copy link

I've never seen such feature, and it doesn't make much sense in real world, so 👍 for removing that.

@TomasVotruba
Copy link

TomasVotruba commented Feb 17, 2018

I saw extract option there, that was removed. I need to debug PHAR by writing in it, is that possible by another configuration?

I have phar.readonly = 0 in php.ini (cli and fpm) but still can't write generated PHAR.

@theofidry
Copy link
Member Author

theofidry commented Feb 17, 2018

I need to debug PHAR by writing in it

Not to my knowledge. phar.readonly=0 just allows you to create PHARs but nothing more.

When I had a couple of issues requiring debugging I switch between those solutions depending of the issue:

  • Using the source code: in case of PHP-Scoper this could be the prefixed source code
  • Changing the source and building the whole PHAR again
  • Changing just one file (copy the content of the file I want into a temporary file, open PsySH and use `$x = new Phar('my.phar'); $x->addFile('foo');

Note that breakpoints should still work inside the PHAR.

The extract option/feature is to unpack the PHAR in a directory, nothing more. I don't think it's a completely useless feature but the use case is a bit limited so I dropped it for now

@TomasVotruba
Copy link

Using the source code: in case of PHP-Scoper this could be the prefixed source code

That's it! Thanks for the idea.

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

Successfully merging this pull request may close these issues.

2 participants