[Cache] Implement PSR-16 SimpleCache v1.0 #20694

nicolas-grekas · 2016-11-30T09:54:37Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	not yet
Fixed tickets	-
License	MIT
Doc PR	symfony/symfony-docs#7409

Second iteration on the topic after #20636 raised some issues.

Please don't squash while merging.

stof · 2016-11-30T09:57:55Z

src/Symfony/Component/Cache/Adapter/SimpleCacheAdapter.php

+        parent::__construct($namespace, $defaultLifetime);
+
+        $this->pool = $pool;
+        $this->miss = new stdClass();


missing \ here

stof · 2016-11-30T10:04:22Z

src/Symfony/Component/Cache/Adapter/SimpleCacheAdapter.php

+     */
+    protected function doFetch(array $ids)
+    {
+        foreach ($this->pool->getMultiple($ids, $this->miss) as $key => $value) {


this call would be performed only when the iteration starts rather than when doFetch is called. Is it an issue ?

The ids have already been validated by the AbstractAdapter. Since this is the only situation that is allowed to throw, it's OK this way to me.

stof · 2016-11-30T10:05:30Z

src/Symfony/Component/Cache/SimpleCache.php

+        } catch (SimpleCacheException $e) {
+            throw $e;
+        } catch (Psr6CacheException $e) {
+            throw new InvalidArgumentException($e->getMessage());


I would link the original exception as previous exception, to keep the original stack trace (same everywhere of course)

stof · 2016-11-30T10:11:29Z

@Seldaek @nicolas-grekas @dragoonis @Nyholm it may be worth starting the work on a shared testsuite for simple-cache too (similar to the one existing for PSR-6)

nicolas-grekas · 2016-12-09T10:30:44Z

Status: needs review
PR updated to fit simple-cache v0.3. The PSR-6 to PSR-16 is now called Symfony\Component\Cache\Simple\Psr6Cache, which opens a new namespace to provide native e.g. Simple\ApcuCache implementations. WDYT?
The PSR-16 to PSR-6 bridge is named Symfony\Component\Cache\Adapter\SimpleCacheAdapter (following existing convention).
Code is tested by the PSR-6 test suite, with both bridges wired together.
This means coverage is full for SimpleCacheAdapter and partial for Psr6Cache, until we work on a shared PSR-16 test suite with @Nyholm.

I'd propose to merge this PR without waiting for more tests nor for more progress from the fig. This would unlock some other things I'd like to do on top of this one, but deserve another PR.
If the fig does not deliver on time for 3.3, then it will still be easy to revert before tagging.

nicolas-grekas · 2016-12-09T10:48:05Z

src/Symfony/Component/Cache/Simple/Psr6Cache.php

+        } catch (SimpleCacheException $e) {
+            throw $e;
+        } catch (Psr6CacheException $e) {
+            throw new InvalidArgumentException($e->getMessage());


here (and similar below), I think we don't need to add $e as a "previous" to the thrown exception: by PSR-6, there can be only one reason for the $e exception: invalid key. Having the previous throwing site (file, line, stack) is not going to help: the error is the $key, so the message is the only thing that is useful IMHO.

there could be a second reason: misbehaving cache implementation. In this case, knowing the original location of the exception could help understand whether the issue is in the key, or in the key validation. And btw, it would still point people to the code implementing the key validation, showing them the logic used to validate it.
So IMO, it is still a good idea to link the original exception.

stof · 2016-12-09T10:58:46Z

src/Symfony/Component/Cache/Tests/Adapter/SimpleCacheAdapterTest.php

+{
+    protected $skippedTests = array(
+        'testDeferredSaveWithoutCommit' => 'Assumes a shared cache which ArrayAdapter is not.',
+        'testSaveWithoutExpire' => 'Assumes a shared cache which ArrayAdapter is not.',


should we run a second set of tests for the SimpleCacheAdapter by making it use a shared cache internally ?

ArrayAdapter replaced by FilesystemAdapter

stof · 2016-12-09T11:01:18Z

src/Symfony/Component/Cache/Simple/Psr6Cache.php

+        } catch (SimpleCacheException $e) {
+            throw $e;
+        } catch (Psr6CacheException $e) {
+            throw new InvalidArgumentException($e->getMessage());


there could be a second reason: misbehaving cache implementation. In this case, knowing the original location of the exception could help understand whether the issue is in the key, or in the key validation. And btw, it would still point people to the code implementing the key validation, showing them the logic used to validate it.
So IMO, it is still a good idea to link the original exception.

stof · 2017-01-03T17:15:41Z

@Nyholm @nicolas-grekas have you made any progress on the shared PSR-16 testsuite ?

Nyholm · 2017-01-03T17:29:10Z

Yes. I do. Expect it to be done at the very latest this weekend.

dragoonis · 2017-01-03T17:30:06Z

Looking forward to seeing it :)

…

On Tue, Jan 3, 2017 at 5:29 PM, Tobias Nyholm ***@***.***> wrote: Yes. I do. Expect it to be done at the very latest this weekend. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20694 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI7kVEVrbPN_eQ9W57oTP6Ns9H8VjKcks5rOoV3gaJpZM4LAAFV> .

Nyholm · 2017-01-04T18:49:10Z

Test PR is here: php-cache/integration-tests#66

nicolas-grekas · 2017-01-05T17:53:01Z

php-cache/integration-tests v0.14.0 integrated, tests green.
PR is ready
some tests could be enhanced, but this should not be a blocker

nicolas-grekas · 2017-01-06T11:11:24Z

Status: needs review
test suite fixed and updated, all green

fabpot · 2017-01-23T21:17:54Z

Thank you @nicolas-grekas.

…kas) This PR was squashed before being merged into the 3.3-dev branch (closes #20694). Discussion ---------- [Cache] Implement PSR-16 SimpleCache v1.0 | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | not yet | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#7409 Second iteration on the topic after #20636 raised some issues. Please don't squash while merging. Commits ------- 6219dd6 [Cache] Create PSR-16 variants of all PSR-6 adapters 99ae9d6 [Cache] Move adapter implementations to traits 848a33e [Cache] Implement PSR-16 SimpleCache v1.0

dragoonis · 2017-01-23T21:51:43Z

👍 🥇

carsonbot added Status: Needs Review Cache Feature labels Nov 30, 2016

stof reviewed Nov 30, 2016

View reviewed changes

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Dec 5, 2016

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

nicolas-grekas force-pushed the simple-cache branch 3 times, most recently from acd7065 to 72ba3a3 Compare December 9, 2016 10:20

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 9, 2016

nicolas-grekas mentioned this pull request Dec 9, 2016

Proposal coming from Symfony trial implementation php-fig/fig-standards#846

Merged

nicolas-grekas modified the milestones: 3.3, 3.x Dec 9, 2016

nicolas-grekas commented Dec 9, 2016

View reviewed changes

stof reviewed Dec 9, 2016

View reviewed changes

nicolas-grekas force-pushed the simple-cache branch 2 times, most recently from da68dae to e543588 Compare December 9, 2016 12:50

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Jan 3, 2017

nicolas-grekas force-pushed the simple-cache branch from e543588 to fadc638 Compare January 4, 2017 21:16

nicolas-grekas changed the title ~~[Cache] Implement PSR-16 SimpleCache v0.3~~ [Cache] Implement PSR-16 SimpleCache v1.0 Jan 4, 2017

nicolas-grekas force-pushed the simple-cache branch from fadc638 to 0610dd8 Compare January 4, 2017 21:21

nicolas-grekas force-pushed the simple-cache branch from e28b69c to 5491454 Compare January 5, 2017 17:51

nicolas-grekas force-pushed the simple-cache branch 4 times, most recently from 1ae92f6 to f1a6827 Compare January 6, 2017 11:09

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 6, 2017

nicolas-grekas force-pushed the simple-cache branch 4 times, most recently from e527505 to 2b5cc7f Compare January 8, 2017 22:27

nicolas-grekas force-pushed the simple-cache branch 3 times, most recently from 781b9d2 to 0ab20ae Compare January 15, 2017 17:01

nicolas-grekas added 2 commits January 23, 2017 14:57

[Cache] Implement PSR-16 SimpleCache v1.0

848a33e

[Cache] Move adapter implementations to traits

99ae9d6

nicolas-grekas force-pushed the simple-cache branch from 0ab20ae to 5b9e9b4 Compare January 23, 2017 13:58

[Cache] Create PSR-16 variants of all PSR-6 adapters

6219dd6

nicolas-grekas force-pushed the simple-cache branch from 5b9e9b4 to 6219dd6 Compare January 23, 2017 14:02

This was referenced Jan 23, 2017

[Cache] Memcached Adapter Docs and Cache Pool Docs Refactoring symfony/symfony-docs#7265

Merged

[Cache] Doc for PSR-16 variants of adapters symfony/symfony-docs#7409

Closed

fabpot merged commit 6219dd6 into symfony:master Jan 23, 2017

nicolas-grekas deleted the simple-cache branch January 23, 2017 21:18

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

ro0NL mentioned this pull request May 30, 2017

[Cache] Ignore missing annotations.php #22965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cache] Implement PSR-16 SimpleCache v1.0 #20694

[Cache] Implement PSR-16 SimpleCache v1.0 #20694

nicolas-grekas commented Nov 30, 2016 •

edited

Loading

stof Nov 30, 2016

stof Nov 30, 2016

nicolas-grekas Dec 9, 2016

stof Nov 30, 2016

stof commented Nov 30, 2016

nicolas-grekas commented Dec 9, 2016

nicolas-grekas Dec 9, 2016

stof Dec 9, 2016

nicolas-grekas Dec 9, 2016

stof Dec 9, 2016

nicolas-grekas Dec 9, 2016

stof Dec 9, 2016

stof commented Jan 3, 2017

Nyholm commented Jan 3, 2017

dragoonis commented Jan 3, 2017 via email

Nyholm commented Jan 4, 2017

nicolas-grekas commented Jan 5, 2017

nicolas-grekas commented Jan 6, 2017

fabpot commented Jan 23, 2017

dragoonis commented Jan 23, 2017

[Cache] Implement PSR-16 SimpleCache v1.0 #20694

[Cache] Implement PSR-16 SimpleCache v1.0 #20694

Conversation

nicolas-grekas commented Nov 30, 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

stof commented Nov 30, 2016

nicolas-grekas commented Dec 9, 2016

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

stof commented Jan 3, 2017

Nyholm commented Jan 3, 2017

dragoonis commented Jan 3, 2017 via email

Nyholm commented Jan 4, 2017

nicolas-grekas commented Jan 5, 2017

nicolas-grekas commented Jan 6, 2017

fabpot commented Jan 23, 2017

dragoonis commented Jan 23, 2017

nicolas-grekas commented Nov 30, 2016 •

edited

Loading