Open func0der opened 3 years ago
Hmmm? Which version of framework do you use?
Because in the master branch, method loadFromCache()
has return type defined as array. Thus, it can't simply return false :)
Also, AspectLoader
also returns only array https://github.com/goaop/framework/blob/596fcaefc18dfc4e3982b3ee552b0a3ceca9a8b1/src/Core/AspectLoader.php#L71
Regarding static analysis: this project uses PhpStan now, every single PR is analysed, see https://github.com/goaop/framework/blob/master/phpstan.neon
Hey @lisachenko,
I am using 2.3.4, but this is still present in the master. Even though there are return type hints in the doc block and a return type defined, the code does not comply with those: https://github.com/goaop/framework/blob/master/src/Core/CachedAspectLoader.php#L104-L105
Both file_get_contents()
and unserialize
both return false
in case of any errors. This could of course be anything. Either the file could not be loaded and $contents
is then false
, which would result in unserialize(false)
and therefore the method returns false
and not an array.
Or the content in the file that is written is somehow broken, so unserialize
can not properly unserialize and therefore the method returns false
and not an array.
I know that this is probably not supposed to happen, but with filesystems becoming unavailable or files getting corrupted, because processes are killed mid-writing, I would say, that it is possible that cases like these occur. In fact I had the case where I had a cache file, but it had no content. If that was because of me debugging or the application crashing of some reason, I do not know.
I think the decision here is: Is an invalid cache a problem that goaop WANTS to handle, or do we simply crash?
It is great that you are using phpstan :) I found sooooo many bugs with it already, that I wonder how I could ever live without static code analysis :shrug: :D I did not see that it was already it place, sorry.
@func0der Checked your comment - could you please send a PR with additional guard/exception to ensure that no false was returned from both unserialize
and file_get_contents
function calls? Additional safety checks will improve stability of code.
The method called in https://github.com/goaop/framework/blob/596fcaefc18dfc4e3982b3ee552b0a3ceca9a8b1/src/Core/CachedAspectLoader.php#L66 can result in
false
, which is then returned.\Go\Core\AspectLoader::load()
and their respective implementations are not supposed to returnfalse
, but onlyarray
s.Simple fix is to not just put the return value in
$loadedItems
.An idea to prevent such problems in the future could be static code analysis.