doctrine / annotations

Annotations Docblock Parser
https://www.doctrine-project.org/projects/annotations.html
MIT License
6.74k stars 235 forks source link

CachedReader fails if Reflection class is eval'd code #186

Open yakobe opened 6 years ago

yakobe commented 6 years ago

If the ReflectionClass is eva'd code, then it crashes when checking when the cache is fresh. It tries to CachedReader::getLastModification of the eval'd code and results in:

Warning:` filemtime(): stat failed for /app/src/MyCode/EntityGenerator.php(36) : eval()'d code

yakobe commented 6 years ago

I guess this could be avoided if the file existence was checked? eg

return max(array_merge(
            [file_exists($filename) ? filemtime($filename) : 0],
            array_map([$this, 'getTraitLastModificationTime'], $class->getTraits()),
            array_map([$this, 'getLastModification'], $class->getInterfaces()),
            $parent ? [$this->getLastModification($parent)] : []
        ));

What do you think?

Ocramius commented 6 years ago

It should rather throw a more explicative exception, explaining that the annotations cannot be scanned.

On 21 Mar 2018 18:34, "Jake Bishop" notifications@github.com wrote:

I guess this could be avoided if the file existence was checked? eg

return max(array_merge( [file_exists($filename) ? filemtime($filename) : 0], array_map([$this, 'getTraitLastModificationTime'], $class->getTraits()), array_map([$this, 'getLastModification'], $class->getInterfaces()), $parent ? [$this->getLastModification($parent)] : [] ));

What do you think?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/doctrine/annotations/issues/186#issuecomment-375030582, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakARpkNYocy-vmil-sUZXzXhrD4XUks5tgo8OgaJpZM4S06hd .

yakobe commented 6 years ago

Possibly. But then what should handle this exception? It would just bubble up (like the current error) and something unrelated. Technically it’s not a problem because it’s just signalling whether the cash is fresh or not. The change suggested above seems to fix it. It might always report the cache is old for eval’d code, but I would say that’s an edge case?

Ocramius commented 6 years ago

Let me re-formulate a few questions to clear if this should be an exception or not:

  1. Are the normal readers capable of reading annotations from this class?
  2. Fid this class previously exist on disk or is it always purely evaluated?
  3. Is the cached reader the only failing endpoint?

If the CachedReader is really the only failing component here, and the file is never touched (always eval'd), then we must assume that the file mtime is somewhere in the future (not yet written to disk), which would let the cache be completely skipped. No exception in that scenario.

On 21 Mar 2018 23:08, "Jake Bishop" notifications@github.com wrote:

Possibly. But then what should handle this exception? It would just bubble up (like the current error) and something unrelated. Technically it’s not a problem because it’s just signalling whether the cash is fresh or not. The change suggested above seems to fix it. It might always report the cache is old for eval’d code, but I would say that’s an edge case?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/doctrine/annotations/issues/186#issuecomment-375112257, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakJ4_W2lGaGRso2TAhd0PvhylESVaks5tgs94gaJpZM4S06hd .

yakobe commented 6 years ago

@Ocramius

  1. Are the normal readers capable of reading annotations from this class? Yes
  2. Fid this class previously exist on disk or is it always purely evaluated? Never on disk, purely evaluated
  3. Is the cached reader the only failing endpoint? It seems to be.

This makes a bit of a question of what to do about the cache for this eval'd code. If it is in the future, then the cache will always be dirty and re-written? Would this be a performance problem?

Ocramius commented 6 years ago

Would this be a performance problem?

Yes, but since we can't determine the age of the file, then all caching should be skipped.

VaN-dev commented 5 years ago

i'm facing this issue too when running local tests (everything works fine on remote CI) :

1) App\Tests\Http\Response\Presenter\Course\Session\SessionPresenterTest::test_commitments_visibility_is_restricted_with_business_rules with data set #0 (true)
filemtime(): stat failed for /var/www/symfony/vendor/phpunit/phpunit/src/Framework/MockObject/MockClass.php(42) : eval()'d code

/var/www/symfony/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:125
/var/www/symfony/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/CachedReader.php:242
/var/www/symfony/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/CachedReader.php:223
/var/www/symfony/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/CachedReader.php:189
/var/www/symfony/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/CachedReader.php:78
/var/www/symfony/vendor/jms/serializer/src/JMS/Serializer/Metadata/Driver/AnnotationDriver.php:68
/var/www/symfony/vendor/jms/metadata/src/Metadata/Driver/DriverChain.php:22
/var/www/symfony/vendor/jms/serializer/src/JMS/Serializer/Metadata/Driver/AbstractDoctrineTypeDriver.php:68
/var/www/symfony/vendor/jms/metadata/src/Metadata/Driver/LazyLoadingDriver.php:23
/var/www/symfony/vendor/jms/metadata/src/Metadata/MetadataFactory.php:87
/var/www/symfony/vendor/jms/serializer/src/JMS/Serializer/GraphNavigator.php:193
/var/www/symfony/vendor/jms/serializer/src/JMS/Serializer/JsonSerializationVisitor.php:145
/var/www/symfony/vendor/jms/serializer/src/JMS/Serializer/GraphNavigator.php:248
/var/www/symfony/vendor/jms/serializer/src/JMS/Serializer/Serializer.php:177
/var/www/symfony/vendor/jms/serializer/src/JMS/Serializer/Serializer.php:96
/var/www/symfony/vendor/phpoption/phpoption/src/PhpOption/Some.php:89
/var/www/symfony/vendor/jms/serializer/src/JMS/Serializer/Serializer.php:99
/var/www/symfony/src/App/Http/Response/Responder/GenericSerializableResponder.php:43
/var/www/symfony/src/App/Tests/Http/Response/Presenter/Course/Session/SessionPresenterTest.php:73

is it a doctrine/annotations or a phpunit/phpunit issue ?

yakobe commented 5 years ago

It appears to be the CachedReader.php on doctrine/annotations because it can accept eval'd code but does not handle it.

@VaN-dev Could you try to to replace the section in the comment https://github.com/doctrine/annotations/issues/186#issuecomment-375030582 to see if it helps? If so, then I could PR a change.

However... It looks appears that the CachedReader.php no longer exists in the 2.0 branch. @Ocramius would this problem just "go away" with 2.0? Where did the caching go 🙂?

VaN-dev commented 5 years ago

@yakobe yes, wrapping with file_exists do solve the problem.

yakobe commented 5 years ago

ok. @Ocramius / other contributors... does it make sense to PR a change? If so, to the v1.8 or v1.7 branch? Or is 2.0 planned soon and solves it anyway?

VaN-dev commented 5 years ago

/summon @Ocramius

Ocramius commented 5 years ago

I've described this recently in https://github.com/sebastianbergmann/phpunit/issues/3606#issuecomment-529099418

This is not just a problem with caching, but with annotation parsing in general: you just happen to be stuck with caching, because a cache hit/miss check already fails.

doctrine/annotations cannot parse annotations in symbols that were generated via eval().

bopoda commented 4 years ago

Hello,

This is not just a problem with caching, but with annotation parsing in general: you just happen to be stuck with caching, because a cache hit/miss check already fails.

Wrapping to file_exists helps. But I wonder if there any way to understand what's wrong with annotations? I can not reproduce the problem on my local env (using env dev and prod both) to debug it properly with xdebug, but reproduce every time on the server (still is looking for differences in the environment but unsuccessfully).

I have the error on server:

Warning: filemtime(): stat failed for /<project-path>/vendor/ocramius/proxy-manager/src/ProxyManager/GeneratorStrategy/EvaluatingGeneratorStrategy.php(53) : eval()'d code

in vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/CachedReader.php (line 251)

Error happens when this string is called: serializer->normalize($app); where $app is MongoDB application Document:

src/Application/Document/Application.php:

namespace Easy\Application\Document;

use Doctrine\ODM\MongoDB\Mapping\Annotations as MongoDB;

/**
 * @MongoDB\Document(repositoryClass="Easy\Application\Repository\ApplicationRepository",
 *     collection="application")
 */
class Application
{
     ...

    /**
     * @MongoDB\ReferenceMany(
     *     targetDocument=Easy\Application\Document\AppGroup::class,
     *     cascade={"persist"},
     *     name="groups"
     * )
     * @MongoDB\Index()
     */
    private $groups;
}

src/Application/Document/AppGroup.php:

namespace Easy\Application\Document;

/**
 * @MongoDB\Document(repositoryClass="Easy\Application\Repository\AppGroupRepository",
 *     collection="app_group")
 */
class AppGroup
{
   //usual fields
}

If the document application has at least one reference to the group then I have an error as described above when normalize document application. There are no errors when I normalize app_group separately.

rm -rf var/cache/prod helps to solve a problem on server exactly for one request. I mean ->normalize($app) works fine when I call the server-endpoint when the cache is removed.

spantaleev commented 3 years ago

I also started hitting this problem today for some yet unknown reason.

What causes ReflectionClass to say the filename is /.../.../vendor/friendsofphp/proxy-manager-lts/src/ProxyManager/GeneratorStrategy/EvaluatingGeneratorStrategy.php(54) : eval()'d code and why does CachedReader::getLastModification() insist on calling filemtime on such a non-path?

Millon15 commented 3 years ago

Update doctrine/annotations to v1.13.1 And bin/console cache:clear --env=prod; bin/console cache:clear --env=dev OR Just rm -rf var/cache Solved the problem for me.

Univalgo commented 2 years ago

Hallo guys, I also run into the same problem which you have with PsrCachedReader.php. A file_exists fixes it.

When is the new code going to be provided to us?

I have v1.13.3 of doctrine/annotations

greg0ire commented 2 years ago

@Univalgo when this comment is addressed I suppose: https://github.com/doctrine/annotations/pull/436#discussion_r811456259