[Fix] Introduce an option to configure cached reader for annotations #136, #358 and #245 #359

TheCelavi · 2017-08-28T16:33:56Z

Fixes for several issues, mostly in regards to caching mechanism.

Added possibility for users to define custom cache driver for annotations, under option annotationCache. Any Doctrine cache can be used.

TheCelavi · 2017-08-28T16:38:33Z

src/Core/GoAspectContainer.php

@@ -91,12 +91,11 @@ public function __construct()
        $this->share('aspect.annotation.reader', function(Container $container) {
            $options = $container->get('kernel.options');
            $reader  = new AnnotationReader();
-            if (!empty($options['cacheDir'])) {
-                $reader = new FileCacheReader(
+            if (!empty($options['annotationCache'])) {


I didn't fixed this one.

annotationCache is not optional, however, current unit tests do not mocks engine configuration, so if I presume that annotationCache will always exist here, test fails.

In general - if you want, I can deal with test as well and remove if (!empty(...)) here, however, both code and tests should be revised -> what is required and what is optional...

It's ok to check with !empty($options['annotationCache']) here.

…ache works though...

TheCelavi · 2017-08-28T19:46:37Z

src/Core/AspectKernel.php

@@ -216,9 +216,9 @@ protected function normalizeOptions(array $options)
        $options['excludePaths']   = PathResolver::realpath($options['excludePaths']);

        if (null === $options['annotationCache']) {
-            $options['annotationCache'] = new PhpFileCache(
+            $options['annotationCache'] = new FilesystemCache(


This is extremely stupid and I don't have any idea why this causing issues...

If Doctrine\Common\Cache\ PhpFileCache is used, test fails (and it does not work in general) on PHP 5.x, and 7.0. It works on PHP 7.1 and 7.2.

On the other hand, with same settings, if Doctrine\Common\Cache\ FilesystemCache is used, all test passes and it does work.

@lisachenko -> do you have any idea what is happening here? And how to proceed? Should we keep FilesystemCache as default implementation, or? Version check and pick accordingly?

BTW I will report issue since it can be easily reproduced...

TheCelavi · 2017-08-29T08:52:56Z

@paventuri In general, it could be implemented by adding some more logic into, per example, caching transformer. However, more logic make us maintain it. Maybe in the future, since, with possibility to choose annotation cache storage this issue may be considered as fixed... I don't know how Alexander feels about it...

lisachenko · 2017-08-29T12:44:08Z

README.md

+#### 6.1 Custom annotation cache
+
+By default, Go! AOP uses `Doctrine\Common\Cache\PhpFileCache` for caching
+annotations. However, should you need to use any other caching engine


Is it ok to use "...should you need to use.." here? Maybe replace "should" with "if"?

Mission impossible? "Your mission is, should you choose to accept it, ..." ? :D

For my English knowledge it is strange phrase with "should" usage )) But we can ignore my comment.

My version is: "However if you need to use any other caching engine...". More straightforward ) But I can not be sure if your version isn't correct )

lisachenko · 2017-08-29T12:48:24Z

src/Core/AspectKernel.php

+        $options['excludePaths'][] = $options['cacheDir'];
+        $options['excludePaths'][] = __DIR__ . '/../';
+        $options['appDir']         = PathResolver::realpath($options['appDir']);
+        $options['cacheFileMode']  = (int) $options['cacheFileMode'];


There is a gotcha here, cacheFileMode MUST be integer, otherwise strange effects occurs:

echo (int) '0770'; // Outputs 770 echo (int) 0770; // 504, which is correct

lisachenko · 2017-08-29T12:52:08Z

src/Core/AspectKernel.php

+                '.annotations.cache',
+                0777 & (~$options['cacheFileMode'])
+            );
+        }


In my opinion this should be done in GoAopContainer class, kernel should not work with such classes directly.

I can propose several solutions:

To extend GoAopContainer class and to redefine definition of annotation.reader class.

Make annotation cache an option that accepts a closure that should return instance of annotation reader and then just store it in the container if declared. By default, GoAopContainer will define a closure that returns simple FilesystemCache cached reader.

lisachenko · 2017-08-29T12:54:47Z

src/Core/GoAspectContainer.php

@@ -91,12 +91,11 @@ public function __construct()
        $this->share('aspect.annotation.reader', function(Container $container) {
            $options = $container->get('kernel.options');
            $reader  = new AnnotationReader();
-            if (!empty($options['cacheDir'])) {
-                $reader = new FileCacheReader(
+            if (!empty($options['annotationCache'])) {


It's ok to check with !empty($options['annotationCache']) here.

Nitpick-CI · 2017-08-29T15:04:24Z

src/Core/GoAspectContainer.php

@@ -88,20 +89,35 @@ public function __construct()
            );
        });

-        $this->share('aspect.annotation.reader', function(Container $container) {
+        $this->share('aspect.annotation.cache', function(Container $container) {


Expected 1 space after FUNCTION keyword; 0 found

Nitpick-CI · 2017-08-29T15:04:26Z

src/Core/GoAspectContainer.php

        });
+
+        $this->share('aspect.annotation.reader', function(Container $container) {


Expected 1 space after FUNCTION keyword; 0 found

TheCelavi · 2017-08-29T15:07:20Z

src/Core/GoAspectContainer.php

                );
            }

-            return $reader;
+            return new DoctrineCache\ArrayCache();


This can occur when running unit tests, so I fallback to ArrayCache in order for tests to pass.

TheCelavi · 2017-08-29T15:08:19Z

src/Core/GoAspectContainer.php

@@ -88,19 +89,35 @@ public function __construct()
            );
        });

-        $this->share('aspect.annotation.reader', function(Container $container) {
+        $this->share('aspect.annotation.cache', function(Container $container) {


Ok, so moving to container - agree, much better. Defined new service, aspect.annotation.cache, so reader can get that service for cache engine for annotations.

Much better, thanks.

lisachenko

Looks good now

paventuri · 2017-08-30T02:15:44Z

Is this going to be merged into goaop:1.X too?

lisachenko · 2017-08-30T06:42:47Z

@paventuri unfortunately, no, versions 1.x are no maintained anymore. Consider to upgrade to 2.x

Fix for goaop#136, goaop#358 and goaop#245

192e8f4

TheCelavi added the Improvement label Aug 28, 2017

TheCelavi requested a review from lisachenko August 28, 2017 16:33

Fixed typo.

ea373f7

TheCelavi commented Aug 28, 2017

View reviewed changes

TheCelavi added 2 commits August 28, 2017 19:57

It is not possible to use PathResolver to check for cache dir.

df2f12e

I have no idea why PhpFileCache does not work for PHP v <= 7.0, FileC…

0260482

…ache works though...

TheCelavi commented Aug 28, 2017

View reviewed changes

TheCelavi mentioned this pull request Aug 28, 2017

PhpFileCache strange issues when used for Doctrine Annotations reader doctrine/cache#236

Closed

lisachenko requested changes Aug 29, 2017

View reviewed changes

Moved service init. to container, fixed Readme.

5a17033

Nitpick-CI reviewed Aug 29, 2017

View reviewed changes

TheCelavi commented Aug 29, 2017

View reviewed changes

lisachenko approved these changes Aug 29, 2017

View reviewed changes

Merge branch '2.x' into bugfix/upgrade-cache-annotation-reader

be414a0

This was referenced Aug 29, 2017

Using annotation always resulting in writing to cache #358

Closed

Interception of internal functions doesn't work if no cacheDir #136

Closed

lisachenko changed the title ~~Fix for #136, #358 and #245~~ [Fix] Introduce an option to configure cached reader for annotations #136, #358 and #245 Aug 29, 2017

TheCelavi mentioned this pull request Aug 29, 2017

[Insight] Deprecated class usage found - in src/Core/GoAspectContainer.php, line 91 #245

Closed

lisachenko added this to the 2.x milestone Aug 29, 2017

lisachenko merged commit 2411eea into goaop:2.x Aug 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Introduce an option to configure cached reader for annotations #136, #358 and #245 #359

[Fix] Introduce an option to configure cached reader for annotations #136, #358 and #245 #359

TheCelavi commented Aug 28, 2017

TheCelavi Aug 28, 2017

lisachenko Aug 29, 2017

TheCelavi Aug 28, 2017

TheCelavi commented Aug 29, 2017

lisachenko Aug 29, 2017

TheCelavi Aug 29, 2017

lisachenko Aug 29, 2017

lisachenko Aug 29, 2017

lisachenko Aug 29, 2017

lisachenko Aug 29, 2017

lisachenko Aug 29, 2017

Nitpick-CI Aug 29, 2017

Nitpick-CI Aug 29, 2017

TheCelavi Aug 29, 2017

TheCelavi Aug 29, 2017

lisachenko left a comment

paventuri commented Aug 30, 2017

lisachenko commented Aug 30, 2017 •

edited

Loading

		});

		$this->share('aspect.annotation.reader', function(Container $container) {

[Fix] Introduce an option to configure cached reader for annotations #136, #358 and #245 #359

[Fix] Introduce an option to configure cached reader for annotations #136, #358 and #245 #359

Conversation

TheCelavi commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCelavi commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lisachenko left a comment

Choose a reason for hiding this comment

paventuri commented Aug 30, 2017

lisachenko commented Aug 30, 2017 • edited Loading

lisachenko commented Aug 30, 2017 •

edited

Loading