Skip to content

Commit

Permalink
feat: Include the original file name in the Phar error (#1034)
Browse files Browse the repository at this point in the history
  • Loading branch information
@theofidry
theofidry authored Oct 8, 2023
1 parent 28f09ad commit 398f62a
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 50 deletions.
2 changes: 1 addition & 1 deletion src/Console/Command/Extract.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ private static function dumpPhar(string $file, string $tmpDir): string
$pubKeyContent = FS::getFileContents($pubKey);
}

$phar = PharFactory::create($tmpFile);
$phar = PharFactory::create($tmpFile, $file);
$pharMeta = PharMeta::fromPhar($phar, $pubKeyContent);

$phar->extractTo($tmpDir);
Expand Down
93 changes: 58 additions & 35 deletions src/Phar/InvalidPhar.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,40 @@

final class InvalidPhar extends PharError
{
public static function fileNotLocal(string $file): self
{
public static function fileNotLocal(
string $file,
?string $originalFile = null,
): self {
// Covers:
// https://github.com/php/php-src/blob/930db2b2d315b2acc917706cf76bed8b09f94b79/ext/phar/phar.c#L1328
return new self(
sprintf(
'Could not create a Phar or PharData instance for the file path "%s". PHAR objects can only be created from local files.',
'Could not create a Phar or PharData instance for the file path "%s"%s. PHAR objects can only be created from local files.',
$file,
null === $originalFile
? ''
: sprintf(
' (of the original file "%s")',
$originalFile,
),
),
);
}

public static function fileNotFound(string $file): self
{
public static function fileNotFound(
string $file,
?string $originalFile = null,
): self {
return new self(
sprintf(
'Could not find the file "%s".',
'Could not find the file "%s"%s.',
$file,
null === $originalFile
? ''
: sprintf(
' (of the original file "%s")',
$originalFile,
),
),
);
}
Expand All @@ -57,32 +73,42 @@ public static function fileNotReadable(string $file): self
);
}

public static function forPhar(string $file, ?Throwable $previous): self
{
public static function forPhar(
string $file,
?string $originalFile,
?Throwable $previous,
): self {
return new self(
self::mapThrowableToErrorMessage($file, $previous, false),
self::mapThrowableToErrorMessage($file, $originalFile, $previous, false),
previous: $previous,
);
}

public static function forPharData(string $file, ?Throwable $previous): self
{
public static function forPharData(
string $file,
?string $originalFile,
?Throwable $previous,
): self {
return new self(
self::mapThrowableToErrorMessage($file, $previous, true),
self::mapThrowableToErrorMessage($file, $originalFile, $previous, true),
previous: $previous,
);
}

public static function forPharAndPharData(string $file, ?Throwable $previous): self
{
public static function forPharAndPharData(
string $file,
?string $originalFile,
?Throwable $previous,
): self {
return new self(
self::mapThrowableToErrorMessage($file, $previous, null),
self::mapThrowableToErrorMessage($file, $originalFile, $previous, null),
previous: $previous,
);
}

private static function mapThrowableToErrorMessage(
string $file,
?string $originalFile,
?Throwable $throwable,
?bool $isPharData,
): string {
Expand All @@ -92,15 +118,24 @@ private static function mapThrowableToErrorMessage(
$pharObject = $isPharData ? 'PharData' : 'Phar';
}

$message = $throwable->getMessage();
$errorMessageStart = sprintf(
'Could not create a %s instance for the file "%s"%s',
$pharObject,
$file,
null === $originalFile
? ''
: sprintf(
' (of the original file "%s")',
$originalFile,
),
);
$message = $throwable?->getMessage() ?? '';

if ($throwable instanceof UnexpectedValueException) {
// https://github.com/php/php-src/blob/930db2b2d315b2acc917706cf76bed8b09f94b79/ext/phar/phar.c#L1330
if (str_ends_with($message, 'file extension (or combination) not recognised or the directory does not exist')) {
return sprintf(
'Could not create a %s instance for the file "%s". The file must have the extension "%s".',
$pharObject,
$file,
$errorMessageStart.'. The file must have the extension "%s".',
$isPharData ? '.zip", ".tar", ".tar.bz2" or ".tar.gz' : '.phar',
);
}
Expand All @@ -111,9 +146,7 @@ private static function mapThrowableToErrorMessage(
preg_match('/^internal corruption of phar \".+\" \((?<reason>.+)\)$/', $message, $matches);

return sprintf(
'Could not create a %s instance for the file "%s". The archive is corrupted: %s.',
$pharObject,
$file,
$errorMessageStart.'. The archive is corrupted: %s.',
ucfirst($matches['reason']),
);
}
Expand All @@ -122,11 +155,7 @@ private static function mapThrowableToErrorMessage(
// https://github.com/php/php-src/blob/930db2b2d315b2acc917706cf76bed8b09f94b79/ext/phar/phar.c#L892
// https://github.com/php/php-src/blob/930db2b2d315b2acc917706cf76bed8b09f94b79/ext/phar/phar.c#L903
if (str_contains($message, ' openssl signature ')) {
return sprintf(
'Could not create a %s instance for the file "%s". The OpenSSL signature could not be read or verified.',
$pharObject,
$file,
);
return $errorMessageStart.'. The OpenSSL signature could not be read or verified.';
}

// https://github.com/php/php-src/blob/930db2b2d315b2acc917706cf76bed8b09f94b79/ext/phar/phar.c#L1002
Expand All @@ -137,18 +166,12 @@ private static function mapThrowableToErrorMessage(
|| str_contains($message, ' signature could not be verified')
|| str_contains($message, ' has a broken or unsupported signature')
) {
return sprintf(
'Could not create a %s instance for the file "%s". The archive signature is broken.',
$pharObject,
$file,
);
return $errorMessageStart.'. The archive signature is broken.';
}
}

return sprintf(
'Could not create a %s instance for the file "%s": %s',
$pharObject,
$file,
$errorMessageStart.': %s',
$message,
);
}
Expand Down
34 changes: 20 additions & 14 deletions src/Phar/PharFactory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,21 @@ private function __construct()
/**
* @throws InvalidPhar
*/
public static function create(string $file): Phar|PharData
{
public static function create(
string $file,
?string $originalFile = null,
): Phar|PharData {
if (!Path::isLocal($file)) {
// This is needed as otherwise Phar::__construct() does correctly bail out on a URL
// path, but not on other non-local variants, e.g. FTPS, which case it may fail still
// but after a timeout, which is too slow.
throw InvalidPhar::fileNotLocal($file);
throw InvalidPhar::fileNotLocal($file, $originalFile);
}

if (!file_exists($file)) {
// We need to check this case since the goal of this factory is to instantiate an existing
// PHAR, not create a new one.
throw InvalidPhar::fileNotFound($file);
throw InvalidPhar::fileNotFound($file, $originalFile);
}

try {
Expand All @@ -57,45 +59,49 @@ public static function create(string $file): Phar|PharData
try {
return new PharData($file);
} catch (Throwable) {
throw InvalidPhar::forPharAndPharData($file, $cannotCreatePhar);
throw InvalidPhar::forPharAndPharData($file, $originalFile, $cannotCreatePhar);
}
}

/**
* @throws InvalidPhar
*/
public static function createPhar(string $file): Phar
{
public static function createPhar(
string $file,
?string $originalFile = null,
): Phar {
if (!Path::isLocal($file)) {
// This is needed as otherwise Phar::__construct() does correctly bail out on a URL
// path, but not on other non-local variants, e.g. FTPS, which case it may fail still
// but after a timeout, which is too slow.
throw InvalidPhar::fileNotLocal($file);
throw InvalidPhar::fileNotLocal($file, $originalFile);
}

if (!file_exists($file)) {
// We need to check this case since the goal of this factory is to instantiate an existing
// PHAR, not create a new one.
throw InvalidPhar::fileNotFound($file);
throw InvalidPhar::fileNotFound($file, $originalFile);
}

try {
return new Phar($file);
} catch (Throwable $throwable) {
throw InvalidPhar::forPhar($file, $throwable);
throw InvalidPhar::forPhar($file, $originalFile, $throwable);
}
}

/**
* @throws InvalidPhar
*/
public static function createPharData(string $file): PharData
{
public static function createPharData(
string $file,
?string $originalFile = null,
): PharData {
if (!Path::isLocal($file)) {
// This is needed as otherwise Phar::__construct() does correctly bail out on a URL
// path, but not on other non-local variants, e.g. FTPS, which case it may fail still
// but after a timeout, which is too slow.
throw InvalidPhar::fileNotLocal($file);
throw InvalidPhar::fileNotLocal($file, $originalFile);
}

if (!file_exists($file)) {
Expand All @@ -107,7 +113,7 @@ public static function createPharData(string $file): PharData
try {
return new PharData($file);
} catch (Throwable $throwable) {
throw InvalidPhar::forPharData($file, $throwable);
throw InvalidPhar::forPharData($file, $originalFile, $throwable);
}
}
}
13 changes: 13 additions & 0 deletions tests/Phar/PharFactoryTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,19 @@ public function test_it_fails_with_a_comprehensive_error_when_cannot_create_a_ph
PharFactory::createPhar($file);
}

public function test_it_fails_with_a_comprehensive_error_when_cannot_create_a_phar_which_is_a_copy(): void
{
$this->expectException(InvalidPhar::class);
$this->expectExceptionMessageMatches(
'/^Could not create a Phar instance for the file ".+"\ \(of the original file "original\.phar"\)\. The file must have the extension "\.phar"\.$/',
);

PharFactory::createPhar(
self::FIXTURES_DIR.'/empty-pdf.pdf',
'original.phar',
);
}

/**
* @dataProvider invalidPharDataProvider
*/
Expand Down

0 comments on commit 398f62a

Please sign in to comment.