Skip to content

[WIP] [Cache] Allow to chain adapters #17452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Jan 19, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR todo

Allows to chain adapters (e.g: use the ArrayAdapter and fallback to the DoctrineAdapter).

  • ArrayAdapter
  • ProxyAdapter

@DavidBadura
Copy link
Contributor

I think the decorator pattern is the wrong way. All adapters must be able to handle it.
A chain class is as much better or not?

Like this: https://github.com/php-cache/chain-adapter/blob/master/src/CachePoolChain.php

@dunglas
Copy link
Member Author

dunglas commented Jan 21, 2016

The chain suits probably better to this case, indeed.

@dunglas
Copy link
Member Author

dunglas commented Jan 21, 2016

I was thinking that all adapters was extending the abstract class but it's not the case (and it's why this PR is marked as WIP).

@@ -109,7 +129,6 @@ function ($deferred, $namespace) {
public function getItem($key)
{
$id = $this->getId($key);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be reverted

@dunglas
Copy link
Member Author

dunglas commented Jan 27, 2016

Replaced by #17556.

@dunglas dunglas closed this Jan 27, 2016
private $decorated;

/**
* @var array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Just by looking at the default value, I know this is an array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has been replaced by #17556. The new one doesn't include docblocks changes anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants