CachedAspectLoader::load() does not comply with \Go\Core\AspectLoader::load() signature

[func0der]

The method called in

framework/src/Core/CachedAspectLoader.php

Line 66 in 596fcae

$loadedItems = $this->loadFromCache($fileName);

can result in false, which is then returned.

\Go\Core\AspectLoader::load() and their respective implementations are not supposed to return false, but only arrays.

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.

[lisachenko]

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

framework/src/Core/AspectLoader.php

Line 71 in 596fcae

public function load(Aspect $aspect): array

Regarding static analysis: this project uses PhpStan now, every single PR is analysed, see https://github.com/goaop/framework/blob/master/phpstan.neon

[func0der]

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 🤷 :D
I did not see that it was already it place, sorry.

[lisachenko]

@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.