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

Add support to force extracting archives #16

Open
wants to merge 5 commits into
base: 1.x
Choose a base branch
from

Conversation

smartinm
Copy link

Refs #10

@smartinm
Copy link
Author

Added new option force-extract to build command:

$ phar-composer help build
Usage:
 build [-x|--force-extract] [path] [target]
Arguments:
 path                  Path to project directory or composer.json (default: ".")
 target                Path to write phar output to (defaults to project name)
Options:
 --force-extract (-x)  Enforce extracting the phar to a temporary directory.
 ...

@clue
Copy link
Owner

clue commented Dec 28, 2013

Thanks for this PR! 👍 The changes look good to me, and I'd like to see this included soon :)

Once we support this option, we should probably also consider to add a config key (extra.phar.force-extract ?), which defaults to false and can be overridden by the CLI option.

Also, would you care to add a note to the README.md and CHANGELOG.md?

Thanks!

@smartinm
Copy link
Author

Hi, pull request updated based on your comments, please review :)

@clue
Copy link
Owner

clue commented Dec 29, 2013

Really great! 👍 The changes looks really good, and I'm perfectly happy with merging them as-is.

One minor thing I'm concerned with is (again.. sigh) naming :) Personally, I'm a bit undecided about the name "force-extract". It certainly describes what is does, but it doesn't really convey when to use this option. Can we come up with a better name?

Other than that, your changes look really good. Thanks for your input!

@smartinm
Copy link
Author

I agree, how about "self-extracting" name? e.g.:

--self-extracting option allow to create a self-extracting phar archive. ?

@clue
Copy link
Owner

clue commented Dec 30, 2013

Way better!

This is actually a tricky one, given that the stub file always includes the Extractor. This is done for platforms which to not have the ext-phar enabled. This will degrade performance for them, but at least allow them to still run the phar.

Now what this option does is that is always enforces to use the Extractor, no matter whether the ext-phar is enabled or not. This is useful in the situation where there's absolutely no way to run the script from within a phar because it relies on filesystem paths (chmod(), realpath(), chdir() etc.)

So in fact each phar is always "self-extracting". So far I'm also having some trouble finding the right wording either, which highlights this subtle nuance :)

@clue
Copy link
Owner

clue commented Jan 7, 2014

Disregard my previous comment :)

how about "self-extracting" name?

Unless we can come up with a better name, I'm okay with it. I'd just like to merge this soon.

Would you care to update the docs accordingly?

@smartinm
Copy link
Author

After reading your comment, I understand that "self-extracting" name can also be confusing...

Maybe an option to control the use of the php-box Extract class?

 --custom-extractor  Uses a custom phar extractor class: no, yes, force (default: yes)
  • --custom-extractor=no not embeds Extract class from php-box (uses phar-extension extract)
  • --custom-extractor=yes embeds Extract class from php-box and uses it whether the ext-phar is not enabled
  • --custom-extractor=force embeds Extract class from php-box and always uses it

@clue
Copy link
Owner

clue commented Jun 6, 2015

Your changeset looks good to and I'd love to finally get this in! :)

How about an --extract=force option which defaults to yes/true for maximum compatibility that can explicitly be disabled with no/false?

@clue
Copy link
Owner

clue commented Jun 18, 2015

Your changeset looks good to and I'd love to finally get this in! :)

Ping @smartinm, is there anything I can help you with? :)

@hopeseekr
Copy link

All of the work in this PR is now in PR #83, with the conflicts resolved.

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

Successfully merging this pull request may close these issues.

3 participants