Skip to content

[HttpFoundation] missing namespace for RedisProxy #27754

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

Merged
merged 1 commit into from
Jun 28, 2018
Merged

[HttpFoundation] missing namespace for RedisProxy #27754

merged 1 commit into from
Jun 28, 2018

Conversation

bonfante
Copy link
Contributor

@bonfante bonfante commented Jun 27, 2018

I think the intention was to check if is instance of \Symfony\Component\Cache\Traits\RedisProxy

Q A
Branch? 4.1 (becareful when merging)
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 28, 2018

Coincidentally I spotted this also yesterday :)
The namespace should be moved to a "use" statement.
And we should double check that all methods we use on the object exist on the proxy.
I think at least"del()" is missing.

@nicolas-grekas
Copy link
Member

Another option is to just remove the check for it. Wouldn't break anything since it never worked. Note that RedisProxy exists because the Redis class used to be incompatible with autogenerated lazy proxies. That's not the case anymore since phpredis 4.

@bonfante
Copy link
Contributor Author

@nicolas-grekas
I spotted this when testing GAE with memorystore.

the __call should do trick for del() right?

public function __call($method, array $args)

@nicolas-grekas nicolas-grekas changed the title missing namespace for RedisProxy [HttpFoundation] missing namespace for RedisProxy Jun 28, 2018
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

the __call should do trick for del() right?

correct!
to be merged on 4.1

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.1 June 28, 2018 06:23
@nicolas-grekas
Copy link
Member

Thank you @bonfante.

@nicolas-grekas nicolas-grekas merged commit 8e0acfc into symfony:4.1 Jun 28, 2018
nicolas-grekas added a commit that referenced this pull request Jun 28, 2018
This PR was submitted for the master branch but it was squashed and merged into the 4.1 branch instead (closes #27754).

Discussion
----------

[HttpFoundation] missing namespace for RedisProxy

I think the intention was to check if is instance of \Symfony\Component\Cache\Traits\RedisProxy

| Q             | A
| ------------- | ---
| Branch?       | 4.1 (becareful when merging)
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |
<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

8e0acfc [HttpFoundation] missing namespace for RedisProxy
@bonfante
Copy link
Contributor Author

Nice, my first commit on symfony 😄

@fabpot fabpot mentioned this pull request Jul 23, 2018
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.

3 participants