Skip to content

Conversation

@nicolas-grekas
Copy link
Member

Q A
Branch? 3.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

private $isHit;
private $expiry;
private $defaultLifetime;
protected $key;
Copy link
Member Author

@nicolas-grekas nicolas-grekas Jun 13, 2016

Choose a reason for hiding this comment

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

Since the class is final, changing to protected here has no bc-promise-related impact.

Copy link
Member

Choose a reason for hiding this comment

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

what is the goal of using protected visibility in a final class ?

Copy link
Member Author

Choose a reason for hiding this comment

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

shorter code, and const removal (to be sure nobody will use it)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @stof meant that you could keep it private, since no other classes could possibly exist to access it.

@fabpot
Copy link
Member

fabpot commented Jun 14, 2016

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 040f53d into symfony:3.1 Jun 14, 2016
fabpot added a commit that referenced this pull request Jun 14, 2016
This PR was merged into the 3.1 branch.

Discussion
----------

[Cache] Fix double fetch in ProxyAdapter

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Commits
-------

040f53d [Cache] Fix double fetch in ProxyAdapter
@nicolas-grekas nicolas-grekas deleted the cache-proxy branch June 14, 2016 11:27
@fabpot fabpot mentioned this pull request Jun 15, 2016
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.

5 participants