[ExpressionLanguage] Making cache PSR6 compliant #19741

ReDnAxE · 2016-08-26T08:20:29Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	#7064

Adding Cache component compatible ParserCache in ExpressionLanguage component.
I hope you will find it useful :) I would like to make tests also

nicolas-grekas · 2016-08-26T08:56:59Z

src/Symfony/Component/ExpressionLanguage/ParserCache/AdapterParserCache.php

+     */
+    private $adapter;
+
+    public function __construct(AdapterInterface $adapter)


Should be type hinted against Psr\Cache\CacheItemInterface

You mean Psr\Cache\CacheItemPoolInterface, right ?

nicolas-grekas · 2016-08-26T09:03:50Z

New features should be submitted on master (but no need to close this PR: you can either rebase then change the base branch yourself in the github UI, or we can do it while merging).

Btw, shouldn't we deprecate ParserCacheInterface in favor of Psr\Cache\CacheItemPoolInterface?

stof · 2016-08-26T09:16:01Z

src/Symfony/Component/ExpressionLanguage/ParserCache/AdapterParserCache.php

+/**
+ * @author Alexandre GESLIN <alexandre@gesl.in>
+ */
+class AdapterParserCache implements ParserCacheInterface


this is not an AdapterParserCache. It is a PSR6ParserCache

stof · 2016-08-26T09:16:18Z

tests are missing btw

ReDnAxE · 2016-08-26T16:07:18Z

@nicolas-grekas you're right we should deprecate ParserCacheInterface in favor of Psr\Cache\CacheItemPoolInterface.

But what does this involve in EL component ?

I can add @deprecated tag on ParserCacheInterface ? And then, how to stop use of ParserCacheInterface in ExpressionLanguage without creating BC break ? I mean, changing type hint on ExpressionLanguage::__construct is not conceivable, right ?

This is my first PR tentative, so be indulgent ^^ hope my questions are not too boring

nicolas-grekas · 2016-08-26T16:21:45Z

add @deprecated tag on ParserCacheInterface

yes

changing type hint on ExpressionLanguage::__construct

yes! by removing the type hint (not replacing it with CacheItemPoolInterface), then doing dynamic checks in the constructor and triggering a deprecation when required

ReDnAxE · 2016-08-28T21:52:54Z

Thanks for your help @nicolas-grekas.
I've made some changes in order to make cache PSR6 compliant, using adapter pattern.

I have a few other questions :

I use Symfony\Component\Cache\Adapter\ArrayAdapter by default in ExpressionLanguage.php. Is it a bad thing to hard coupling with Cache component ? Perhaps it's better to use the existing ArrayParserCache with the adapter.
With PSR6 specifications, some characters are forbidden in keys declarations. For now my sanitize method is not sufficient, but I don't know exactly how to assure unique keys with expressions. Replacing with a hash ?

linaori · 2016-08-29T08:49:20Z

src/Symfony/Component/ExpressionLanguage/ExpressionLanguage.php

    {
-        $this->cache = $cache ?: new ArrayParserCache();
+        if (null !== $cache && false === is_a($cache, CacheItemPoolInterface::class)) {


Should be:

if (null !== $cache && !$cache instanceof CacheItemPoolInterface) {

Since 5.0.0 :P

This function became deprecated in favour of the instanceof operator. Calling this function will result in an E_STRICT warning.

even shorter: if ($cache instanceof ParserCacheInterface) {

xabbuh · 2016-09-30T12:48:44Z

src/Symfony/Component/ExpressionLanguage/ExpressionLanguage.php

+                @trigger_error(sprintf('Passing an instance of %s as constructor argument for %s is deprecated as of 3.2 and will be removed in 4.0. Pass an instance of %s instead.', ParserCacheInterface::class, self::class, CacheItemPoolInterface::class), E_USER_DEPRECATED);
+                $cache = new ParserCacheAdapter($cache);
+            } elseif (!$cache instanceof CacheItemPoolInterface) {
+                throw new \InvalidArgumentException(sprintf('Cache argument has to implements %s.', CacheItemPoolInterface::class));


xabbuh · 2016-09-30T12:54:17Z

src/Symfony/Component/ExpressionLanguage/ParserCache/ParserCacheAdapter.php

+    public function getItem($key)
+    {
+        $value = $this->pool->fetch($key);
+        $f = $this->createCacheItem;


Is this necessary?

Yes, not working with $this->createCacheItem($key, $value, $isHit);
picked up here https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Adapter/ArrayAdapter.php#L75

xabbuh · 2016-09-30T12:55:44Z

👍

ReDnAxE · 2016-09-30T17:13:56Z

Could someone explain me why in the last build PHP:7 deph=high, I have 54 errors in Symfony\Bundle\SecurityBundle\Tests\Functional ?

Here is the exception thrown all the time :
Symfony\Component\DependencyInjection\Exception\LogicException: Asset support cannot be enabled as the Asset component is not installed.

fabpot · 2016-09-30T18:05:59Z

@ReDnAxE Should be fixed now

ReDnAxE · 2016-09-30T18:45:38Z

@stof with a @trigger_error in ParserCache\ParserCacheInterface.php as you asked, PHP:7 deph=high was broken as explained above, I could not explain why.
@fabpot Builds are ok.

nicolas-grekas · 2016-09-30T20:03:01Z

I thinkwe should try harder following @stof's advice. The tests don't decide.

ReDnAxE · 2016-09-30T20:28:37Z

@nicolas-grekas I agree... This is why I prefer to mention him

nicolas-grekas · 2016-09-30T20:41:05Z

Can you add the trigger again so that we can see what's failing in the tests?

ReDnAxE · 2016-09-30T21:08:57Z

@nicolas-grekas Why not ! I re-push the @trigger_error.
I confess I do not understand. Now, it's no more the same errors : we have fails in PHP7 dep=high|low in TwigBundle Test Suite

With the same Symfony\Component\DependencyInjection\Exception\LogicException exception type thrown as before

nicolas-grekas · 2016-09-30T21:13:59Z

Not related I guess

ReDnAxE · 2016-09-30T21:15:36Z

I rebased my branch to 533c11c (master)
I will rebase one more time

ReDnAxE · 2016-09-30T21:26:36Z

I rebased on 418944b
same errors... uh no, worst, 1 error in appVeyor o_O :
Symfony\Component\HttpKernel\Tests\ClientTest::testGetScript RuntimeException: OUTPUT: ERROR OUTPUT:

ReDnAxE · 2016-09-30T22:08:22Z

I know we are in 1 october, so it's probably too late ^^, but I saw master is not stable on the last 418944b commit. Perhaps I would rebase on a older stable master commit ?

EDIT: I'm on c2d8356 now. Appveyor recovered..! good night ;)

ReDnAxE · 2016-10-03T08:24:28Z

@nicolas-grekas it works better when rebasing on master's clean commit ;)
I changed deprecation notices versions from "3.2" to "3.3"

xabbuh · 2016-10-03T08:50:20Z

Please also add the deprecation in the upgrade files.

nicolas-grekas · 2016-10-05T13:01:38Z

I changed deprecation notices versions from "3.2" to "3.3"

why that? that could still happen for 3.2 , who knows :)
also please add a note in the upgrade files as suggested by @xabbuh

fabpot · 2016-10-06T00:03:25Z

src/Symfony/Component/ExpressionLanguage/ParserCache/ParserCacheAdapter.php

+/**
+ * @author Alexandre GESLIN <alexandre@gesl.in>
+ *
+ * @internal


You add a comment to indicate that this class should be removed in 4.0.

fabpot · 2016-10-06T00:04:00Z

Indeed, I'd like to include that in 3.2 if possible.

xabbuh · 2016-10-06T09:17:38Z

👍 from me after the remaining comments have been addressed

ReDnAxE · 2016-10-06T09:21:40Z

Thank's ! I'll make thoses changes this afternoon :) I learned that features was freezed for 3.2. This is why I changed to 3.3

ReDnAxE · 2016-10-06T15:40:52Z

The changes are done.

ReDnAxE · 2016-10-17T08:14:05Z

ping @nicolas-grekas ;)

fabpot · 2016-10-17T18:35:50Z

Thank you @ReDnAxE.

…andre GESLIN) This PR was merged into the 3.2-dev branch. Discussion ---------- [ExpressionLanguage] Making cache PSR6 compliant | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | [#7064](symfony/symfony-docs#7064) Adding Cache component compatible ParserCache in ExpressionLanguage component. I hope you will find it useful :) I would like to make tests also Commits ------- a7352ff [ExpressionLanguage] Making cache PSR6 compliant

This PR was merged into the master branch. Discussion ---------- Update caching.rst Updating doc for this PR Making cache PSR6 compliant : [#19741](symfony/symfony#19741) Commits ------- f03f2f3 Update caching.rst

carsonbot added Status: Needs Review ExpressionLanguage Feature labels Aug 26, 2016

nicolas-grekas reviewed Aug 26, 2016
View reviewed changes

stof reviewed Aug 26, 2016
View reviewed changes

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Aug 28, 2016

ReDnAxE changed the title ~~[ExpressionLanguage] adding cache component compatible ParserCache~~ [ExpressionLanguage] Making cache PSR6 compliant Aug 28, 2016

linaori reviewed Aug 29, 2016
View reviewed changes

xabbuh reviewed Sep 30, 2016

View reviewed changes

fabpot reviewed Oct 6, 2016

View reviewed changes

[ExpressionLanguage] Making cache PSR6 compliant

a7352ff

ReDnAxE mentioned this pull request Oct 17, 2016

Update caching.rst symfony/symfony-docs#7064

Merged

fabpot merged commit a7352ff into symfony:master Oct 17, 2016

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

[ExpressionLanguage] Making cache PSR6 compliant #19741

[ExpressionLanguage] Making cache PSR6 compliant #19741

Conversation

ReDnAxE commented Aug 26, 2016 • edited Loading

Choose a reason for hiding this comment

ReDnAxE Aug 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 26, 2016 • edited Loading

Choose a reason for hiding this comment

stof commented Aug 26, 2016

ReDnAxE commented Aug 26, 2016

nicolas-grekas commented Aug 26, 2016

ReDnAxE commented Aug 28, 2016 • edited Loading

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

ReDnAxE Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

xabbuh commented Sep 30, 2016

ReDnAxE commented Sep 30, 2016

fabpot commented Sep 30, 2016

ReDnAxE commented Sep 30, 2016 • edited Loading

nicolas-grekas commented Sep 30, 2016

ReDnAxE commented Sep 30, 2016

nicolas-grekas commented Sep 30, 2016

ReDnAxE commented Sep 30, 2016 • edited Loading

nicolas-grekas commented Sep 30, 2016

ReDnAxE commented Sep 30, 2016 • edited Loading

ReDnAxE commented Sep 30, 2016 • edited Loading

ReDnAxE commented Sep 30, 2016 • edited Loading

ReDnAxE commented Oct 3, 2016

xabbuh commented Oct 3, 2016

nicolas-grekas commented Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

fabpot commented Oct 6, 2016

xabbuh commented Oct 6, 2016

ReDnAxE commented Oct 6, 2016 • edited Loading

ReDnAxE commented Oct 6, 2016

ReDnAxE commented Oct 17, 2016

fabpot commented Oct 17, 2016

ReDnAxE commented Aug 26, 2016 •

edited

Loading

ReDnAxE Aug 26, 2016 •

edited

Loading

nicolas-grekas commented Aug 26, 2016 •

edited

Loading

ReDnAxE commented Aug 28, 2016 •

edited

Loading

ReDnAxE Sep 30, 2016 •

edited

Loading

ReDnAxE commented Sep 30, 2016 •

edited

Loading

ReDnAxE commented Sep 30, 2016 •

edited

Loading

ReDnAxE commented Sep 30, 2016 •

edited

Loading

ReDnAxE commented Sep 30, 2016 •

edited

Loading

ReDnAxE commented Sep 30, 2016 •

edited

Loading

nicolas-grekas commented Oct 5, 2016 •

edited

Loading

ReDnAxE commented Oct 6, 2016 •

edited

Loading