[Cache] Fix decoration of TagAware adapters in dev #22960
Conversation
chalasr
commented
May 30, 2017
•
| Q | A |
|---|---|
| Branch? | 3.3 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #22956 |
| License | MIT |
| Doc PR | n/a |
| { | ||
| $event = $this->start(__FUNCTION__, $tags); | ||
| try { | ||
| return $event->result = $this->pool->invalidateTags($tags); |
There was a problem hiding this comment.
should it throw when the pool doesn't implement TagAwareAdapterInterface itself?
There was a problem hiding this comment.
I still hesitate, calling invalidateTags() on it would gives the same error as when calling it on the inner directly, right?
|
Shouldn't we have a separate |
358132f to
c677879
Compare
|
|
c677879 to
c369b72
Compare
| /** | ||
| * @author Robin Chalas <robin.chalas@gmail.com> | ||
| */ | ||
| class TraceableTagAwareAdapter implements TagAwareAdapterInterface |
There was a problem hiding this comment.
Wouldn't it make sense for this class to extend the TraceableAdapter?
There was a problem hiding this comment.
Instead of adding a trait? That would require to open the $pool property but I've no strong preference
There was a problem hiding this comment.
Else we will need to update CacheDataCollector as its addInstance() method is type hinted with TraceableAdapter.
There was a problem hiding this comment.
I agree with @xabbuh. I don't really understand the trait purpose appart from allowing us to make it @internal while TraceableAdapater is already released as "public".
Using inheritance and adding the protected TraceableAdapter::getPool() method looks a better option to me for this.
a780213 to
9103167
Compare
| public function __construct(TagAwareAdapterInterface $pool) | ||
| { | ||
| parent::__construct($pool); | ||
| } |
There was a problem hiding this comment.
it is: it adds a type hint constraint
| @@ -22,7 +22,8 @@ | |||
| */ | |||
| class TraceableAdapter implements AdapterInterface | |||
| { | |||
| private $pool; | |||
| /** @internal */ | |||
There was a problem hiding this comment.
let's remove that (and the one on the start method)
they are part of the internal api/respo of the class
9103167 to
28e615a
Compare
|
Thank you @chalasr. |
This PR was merged into the 3.3 branch. Discussion ---------- [Cache] Fix decoration of TagAware adapters in dev | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22956 | License | MIT | Doc PR | n/a Commits ------- 28e615a Fix decorating TagAware adapters in dev
…(chalasr) This PR was merged into the 3.3 branch. Discussion ---------- [Cache] Fix decoration of TagAware adapters in dev | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#22956 | License | MIT | Doc PR | n/a Commits ------- 28e615a Fix decorating TagAware adapters in dev
|
@chalasr, I just pulled the 3.3.x-dev branch and tried your fix. It seems the $definition->getClass() in src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CacheCollectorPass.php:47 always returns null so we always register the TraceableAdapter::class. |
|
@tifabien ok I'm looking at it |
This PR was merged into the 3.3 branch. Discussion ---------- [FrameworkBundle] Fix CacheCollectorPass priority | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | #22960 (comment) It was run before optimization, so child definitions were not resolved yet. Commits ------- 28b253a Fix CacheCollectorPass priority