samdark/sitemap

[BUG] @ should not be used

touchweb-vincent opened this issue · 9 comments

Hello,

Thanks you for all this, you did a good job :)

In the futur, if you can remove the "@" from unlink (Sitemap.php, inside createNewFile()) to make your code compliant with the strict dev environnement, it would be nice.

if(is_file($filePath)){ unlink($filePath);}

Best regards,

V

Can't do that since unlink triggers warnings in some cases additionally to returning false. Implementing custom handler to catch these is a bit too much.

If you have a solution, feel free to propose it. Until then, closing the issue.

In 10 years, i never have any problem with this :

if(is_file($file)){
unlink($file);
}

Or at least :
if(is_file($file)){
@Unlink($file);
}
In case there is rights issues which forbid PHP to write against the file.

Well, I did. We've received lots of reports for Yii framework not handling edge cases.

I understand this point but you cannot deal with frameworks related issues. That's not your problem if frameworks doesn't catch efficiently errors.

It is not framework problem. It is PHP general one. You can't catch warnings normally, these aren't fatal to code execution and totally ignoring warnings for everything isn't an option. Hence @.

In this particular case it should actually be if (!@unlink($filePath)) { throw.

All except fatals can be caught (this will be updated with http://php.net/manual/fr/language.errors.php7.php).

I don't understand that point, warnings can be caught, and in all case, there is function to deal with rights issues (such as is_readable, is_writable)

This library supports 5.3+ so PHP7 error handling doesn't count.

Of course, we can check it like the following:

$filePath = relpath($filePath);
if (!file_exists($filePath) || is_writeable($filePath)) {
    unlink($filePath);
} else {
    throw \Exception('File is not writable');
}

That still leaves more margin for errors because unlike @unlink it's not atomic. In this particular case it's acceptable though since concurrency issues are unlikely.

Obviously, you mean : if (file_exists($filePath) ?

That sounds like a good compromise.

Done.