Fix trying to open non-existant manifest file #1600
Conversation
| @@ -68,7 +68,7 @@ public function process(ContainerBuilder $container): void | |||
| $runtimeDefinition->setArgument(1, $version); | |||
|
|
|||
| if (is_a($versionStrategyDefinition->getClass(), JsonManifestVersionStrategy::class, true)) { | |||
| $jsonManifestString = file_get_contents($version); | |||
| $jsonManifestString = @file_get_contents($version); | |||
There was a problem hiding this comment.
i really dislike ignoring errors. could we instead do
| $jsonManifestString = @file_get_contents($version); | |
| if (!file_exists($version)) { | |
| $this->log($container, 'The manifest file at "'.$version.'" does not yet exist'); | |
| return; | |
| } | |
| $jsonManifestString = file_get_contents($version); |
There was a problem hiding this comment.
I didn't add it in the first place because file_get_contents doesn't trigger an error, only a E_WARNING which doesn't halt the script. It's the cache:clear of symfony who transform this into an error afterwards.
And since $jsonManifestString is set to false with or without the error control operator, the unreadable file is handled by the next condition.
But as you wish, the log message will be clearer with your proposed solution !
There was a problem hiding this comment.
yeah, i guess the check if the contents of the file is empty would have caught it. but this seems clearer to me.
thanks for looking into the issue and providing the fix!
|
/cc @wouterSkepp |
AirBair
changed the title
Hello,
Following #1529,
cache:clearauto-script in fresh composer install for example is crashing because the manifest.json does not exist yet.This PR add a check on the existence of the file before trying to open it.