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

Make OPT_BINARY_PROTOCOL configurable #106

Closed
danieledangeli opened this Issue May 17, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@danieledangeli

danieledangeli commented May 17, 2017

Hello guys,
we're using your library for the cache pool.
We have exprienced some problems with the OPT_BINARY_PROTOCOL and sadly realized that you set this option under the hood.

Would be really useful if this option could be configurable (like if I want disable it by passing an option to the __construct method).

It does make sense?

Followoing the piece of code I'm talking about

**
 * @author Aaron Scherer <aequasi@gmail.com>
 * @author Tobias Nyholm <tobias.nyholm@gmail.com>
 */
class MemcachedCachePool extends AbstractCachePool implements HierarchicalPoolInterface
{
    use HierarchicalCachePoolTrait;
    use TagSupportWithArray;

    /**
     * @type \Memcached
     */
    protected $cache;

    /**
     * @param \Memcached $cache
     */
    public function __construct(\Memcached $cache)
    {
        $this->cache = $cache;
        $this->cache->setOption(\Memcached::OPT_BINARY_PROTOCOL, true);
    }

...
}
@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm May 18, 2017

Member

Hey.

Im not sure at the moment why we set this option. Of course it is better for performance but I believe it also solved some issues. Anyhow, since I do not have a clear reson, I would be happy if you send a PR allowing people to configure this behavior.

Member

Nyholm commented May 18, 2017

Hey.

Im not sure at the moment why we set this option. Of course it is better for performance but I believe it also solved some issues. Anyhow, since I do not have a clear reson, I would be happy if you send a PR allowing people to configure this behavior.

@danieledangeli

This comment has been minimized.

Show comment
Hide comment
@danieledangeli

danieledangeli May 18, 2017

Yes of course, if it make sense I can do it. I had to rewrite the MemcacheCachePool because this option was causing some problems for us

danieledangeli commented May 18, 2017

Yes of course, if it make sense I can do it. I had to rewrite the MemcacheCachePool because this option was causing some problems for us

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm May 18, 2017

Member
Member

Nyholm commented May 18, 2017

@prisis

This comment has been minimized.

Show comment
Hide comment
@prisis
Member

prisis commented Nov 7, 2017

@danieledangeli any news?

@prisis

This comment has been minimized.

Show comment
Hide comment
@prisis

prisis Mar 29, 2018

Member

No feedback was provided so im closing this.

Member

prisis commented Mar 29, 2018

No feedback was provided so im closing this.

@prisis prisis closed this Mar 29, 2018

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