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.

Sign up for GitHub

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

Jump to bottom

feat: Include the original file name in the Phar error #1034

Merged
merged 2 commits into from
Oct 8, 2023
Merged

feat: Include the original file name in the Phar error #1034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0af4fa3
feat: Include the original file name in the Phar error
theofidry Oct 8, 2023
fd09b94
fix
theofidry Oct 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Console/Command/Extract.php
View file
Open in desktop
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
Open in desktop
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 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 @@
$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() ?? '';

Check warning on line 132 in src/Phar/InvalidPhar.php

View workflow job for this annotation

GitHub Actions / Infection (PHP 8.1) 

Escaped Mutant for Mutator "NullSafeMethodCall":

--- Original
+++ New
@@ @@
             $pharObject = $isPharData ? 'PharData' : 'Phar';
         }
         $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() ?? '';
+        $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')) {

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 @@
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 @@
// 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 @@
|| 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
Open in desktop
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
Open in desktop
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
Loading