search

contao / contao-manager

Contao Manager
GNU Lesser General Public License v3.0
85 stars 33 forks source link

Use is_readable check #584

Closed fritzmg closed 4 years ago

fritzmg commented 4 years ago

If a config file is not readable for whatever reason, file_get_contents will return false and subsequently the following error will be thrown:

Error: Uncaught TypeError: json_decode() expects parameter 1 to be string, boolean given in phar://…/web/contao-manager.phar.php/api/Process/AbstractProcess.php:60 Stack trace: 
#0 phar://…/web/contao-manager.phar.php/api/Process/AbstractProcess.php(60): json_decode(false, true) 
#1 phar://…/web/contao-manager.phar.php/api/Process/ProcessController.php(284): Contao\ManagerApi\Process\AbstractProcess::readConfig('…...') 
#2 phar://…/web/contao-manager.phar.php/api/Process/ProcessController.php(160): Contao\ManagerApi\Process\ProcessController->updateStatus() 
#3 phar://…/web/contao-manager.phar.php/api/Process/ProcessController.php(145): Contao\ManagerApi\Process\ProcessController->getStatus() 
#4 phar://…/web/contao-manager.phar.php/api/TaskOperation/AbstractProcessOperation.php(56): Contao\ManagerApi\Process\ProcessController->isStarted()

is_readable checks both whether the file exists and is readable. Though may be it should be two separate exceptions after all? Since, if the file exists, it's not really an invalid argument.

ausi commented 4 years ago

Though may be it should be two separate exceptions after all?

👍 The error messages would also be more useful IMO if they tell which one of the two errors it actually is.

fritzmg commented 4 years ago

I have changed the PR accordingly. Keep in mind that this is completely untested, as I am yet unable to run the testing environment 😬

aschempp commented 4 years ago

Shouldn't we rather check the return value of file_get_contents, to 1. prevent another filesystem call and handle any other issues (not sure what else, but anyway…)?

fritzmg commented 4 years ago

Imho that would be an additional check afterwards. Unless you don't want to give more detailed reasons in the front end about why the file could not be read?

aschempp commented 4 years ago

I think "is not readable or does not exist" would be sufficient in any case. There's not much the user can do anyway, right?

aschempp commented 4 years ago

I have improved this in d5fcf2e9119c8b53b441c4af02055d37398c9a5f