Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Cache] Fix decoration of TagAware adapters in dev#22960

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:3.3fromchalasr:debug-adapter-tags
May 31, 2017

Conversation

@chalasr
Copy link
Member

@chalasrchalasr commentedMay 30, 2017•
edited
Loading

QA
Branch?3.3
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#22956
LicenseMIT
Doc PRn/a

{
$event =$this->start(__FUNCTION__,$tags);
try {
return$event->result =$this->pool->invalidateTags($tags);

Choose a reason for hiding this comment

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

should it throw when the pool doesn't implement TagAwareAdapterInterface itself?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I still hesitate, callinginvalidateTags() on it would gives the same error as when calling it on the inner directly, right?

@nicolas-grekasnicolas-grekas added this to the3.3 milestoneMay 30, 2017
@jvasseur
Copy link
Contributor

jvasseur commentedMay 30, 2017•
edited
Loading

Shouldn't we have a separateTagAwareTraceableAdapter to decorate tag aware adapters and keep the current one for non tag aware adapters to prevent having an adapter implementing theTagAwareAdapterInterface when in doesn't?

chalasr reacted with thumbs up emoji

@chalasrchalasr changed the title[Cache] Make TraceableAdapter TagAware[Cache] Fix decoration of TagAware adapters in devMay 30, 2017
@chalasr
Copy link
MemberAuthor

TraceableTagAwareAdapter added and wired to decorate tag aware adapters

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

/**
* @author Robin Chalas <robin.chalas@gmail.com>
*/
class TraceableTagAwareAdapterimplements TagAwareAdapterInterface
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense for this class to extend theTraceableAdapter?

ogizanagi reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Instead of adding a trait? That would require to open the$pool property but I've no strong preference

Copy link
Member

Choose a reason for hiding this comment

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

Else we will need to updateCacheDataCollector as itsaddInstance() method is type hinted withTraceableAdapter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with@xabbuh. I don't really understand the trait purpose appart from allowing us to make it@internal whileTraceableAdapater is already released as "public".
Using inheritance and adding the protectedTraceableAdapter::getPool() method looks a better option to me for this.

Choose a reason for hiding this comment

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

makes sense

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

updated

@chalasrchalasrforce-pushed thedebug-adapter-tags branch 2 times, most recently froma780213 to9103167CompareMay 31, 2017 07:56
publicfunction__construct(TagAwareAdapterInterface$pool)
{
parent::__construct($pool);
}
Copy link
Member

Choose a reason for hiding this comment

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

the constructor is not needed

Choose a reason for hiding this comment

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

it is: it adds a type hint constraint

Copy link
Member

Choose a reason for hiding this comment

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

oh indeed

class TraceableAdapterimplements AdapterInterface
{
private$pool;
/** @internal */

Choose a reason for hiding this comment

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

let's remove that (and the one on the start method)
they are part of the internal api/respo of the class

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

@nicolas-grekas
Copy link
Member

Thank you@chalasr.

@nicolas-grekasnicolas-grekas merged commit28e615a intosymfony:3.3May 31, 2017
nicolas-grekas added a commit that referenced this pull requestMay 31, 2017
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/aCommits-------28e615a Fix decorating TagAware adapters in dev
@chalasrchalasr deleted the debug-adapter-tags branchMay 31, 2017 09:59
chalasr pushed a commit to chalasr/symfony that referenced this pull requestMay 31, 2017
…(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/aCommits-------28e615a Fix decorating TagAware adapters in dev
@tifabien
Copy link
Contributor

@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.

@chalasr
Copy link
MemberAuthor

@tifabien ok I'm looking at it

@chalasr
Copy link
MemberAuthor

@tifabien Can you please try#23018 on your project?

fabpot added a commit that referenced this pull requestJun 1, 2017
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
@fabpotfabpot mentioned this pull requestJun 5, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

+2 more reviewers

@ogizanagiogizanagiogizanagi left review comments

@tifabientifabientifabien approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

7 participants

@chalasr@jvasseur@nicolas-grekas@tifabien@xabbuh@ogizanagi@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp