phar-composer icon indicating copy to clipboard operation
phar-composer copied to clipboard

Add support to force extracting archives

Open smartinm opened this issue 11 years ago • 11 comments

Refs clue/phar-composer#10

smartinm avatar Dec 28 '13 12:12 smartinm

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.
 ...

smartinm avatar Dec 28 '13 13:12 smartinm

Thanks for this PR! :+1: 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!

clue avatar Dec 28 '13 14:12 clue

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

smartinm avatar Dec 29 '13 15:12 smartinm

Really great! :+1: 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!

clue avatar Dec 29 '13 22:12 clue

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

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

smartinm avatar Dec 30 '13 13:12 smartinm

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 avatar Dec 30 '13 13:12 clue

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?

clue avatar Jan 07 '14 15:01 clue

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

smartinm avatar Jan 11 '14 17:01 smartinm

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 avatar Jun 06 '15 15:06 clue

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

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

clue avatar Jun 18 '15 12:06 clue

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

hopeseekr avatar May 31 '19 14:05 hopeseekr