why changing the error reporting ?

To return a false instead of an E_WARNING on invalid option passed.

Well, IMO, the warning would be much easier to debug, as it explains what is wrong. So I'm -1 on this.

See https://github.com/symfony/symfony/pull/20863/files#diff-ed071d63655500d46a7c7866cdb74cddR193 for test checking a false return on error instead of an E_WARNING. Is this something you don't think we should do?

@nicolas-grekas @stof Is the consensus to allow the E_WARNING and not handle a bool return here?

HHVM simply ignores invalid options silently anyway, so the options are:

• catch error and return bool on PHP
• always return true on HHVM

OR if we don't catch the E_WARNING

• throw E_WARNING on PHP
• again, silent on HHVM

OR, another option; continue catching the E_WARNNG but transform into an exception, which IMHO, may be the most developer friendly option? Does the cache component require the symfony polyfills? So we'd have access to get_last_error?

Well, we could just let the error handler do it, without doing any special handling for E_WARNING happening inside setOption

Removed error suppression and amended test to check for E_WARNING in latest push.

I don't think we should expose such public API

This was a middle-ground approach to allow a simple string DSN for create as @nicolas-grekas wanted, but also allow for an array of DSNs using a calls during service construction to this method.

As well, without this method, all the logic from setup(), setOption(), resolveOption(), addServer(), parseServerDSN(), and isServerInClientPool() would need to be shoved into create(). I vastly prefer small, simple methods to a behemoth like RedisAdapter::createConnection(), IMHO.

Returning a boolean does not make sense here:

• handling errors by boolean return values is not user-friendly (no message explaining the issue)
• it is ignored by create() anyway, meaning you don't have error handling at all

Returns $this now in latest push.

why protected ?

Oversight from when this was part of a shared method for \Memcached and \Memcache implementations. No longer the case, obviously. Fixed in latest push.

you should avoid shifting the array. Mutating an array for reading it is a bad idea: it forces PHP to copy it (not that bad here as it has only a few items, but very bad in general

Gone

using array_filter is a bad idea: it forces to compare all servers even if the first one is a match, meaning that you force to always reach the worse case (when there is no match, we are indeed forced to compare them all)

Changed to foreach in latest push.

the min version is 2.2.0 in the code, not 2.1.0

>2.1.0 and >=2.2.0 are the same thing, but I follow the logic of clarifying this. 👍

not the same. 2.2.0RC1 is > 2.1.0 but is not >= 2.2.0 (the same would be true for 2.1.1 if there was such a release)

Clarified in latest push.

shouldn't it be flushed between tests instead of at the end of the testcase, to ensure test isolation ?

I would even go further and avoid making the client static, ensuring we don't reuse it between tests at all.

This is how the Redis adapter handles it, but yeah; makes sense. Changed.

The base test class already calls clear on the pool, that's why it's not needed, isn't it?

Looks like you're right! Reverted to:

    public static function setupBeforeClass()
    {
        if (!MemcachedAdapter::isSupported()) {
            self::markTestSkipped('Extension memcached >=2.2.0 required.');
        }

        static::$client = static::getClientFromAdapter(MemcachedAdapter::create(
            static::defaultClientServer(),
            static::defaultClientOptions()
        ));

        parent::setupBeforeClass();
    }

shouldn't it be adapter ?

Yup ;-) But that test is actually gone now.

This class should be removed: creating a Memcached layer is not the responsibility of the Cache component. But by having this class, people will use it as such. An alternative could be to mark it as internal, but I'm not sure this makes more sense.

Marked @internal and final for now. I really, really, really want to avoid shoving all this logic into a single createConnection method within the Adapter itself...

I think we don't need server defaults in the cache component (we may have in FrameworkBundle later on, but that's another topic.)

These are the distribution defaults for an installed memcached server with no user configuration applied. Additionally, this doesn't actually add a default server if no DSN is passed. At a minimum, the user still needs to pass memcached: as the DSN. An empty array or empty string will not add a server.

What do you think about enabling binary mode by default?

Almost added it in my first pass actually, but decided not to be presumptuous. Since you feel similarly, I say yes.

What about username+ password for sasl?

Fixed for both host and socket DSNs:

memcached://user:password@127.0.0.1
memcached://user:password@/var/local/run/memcached.socket

Shouldn't the parent be called before?

This is not done automatically when the connection is destructed?

TBH, this method isn't even documented yet, found it when looking through the extension source to determine available options for the documentation PR. It seems like this is supposed to be called if the new buffer_writes option is enabled. But I'd have to take a closer look at the source of the extension to confirm if/when it is called by the extension itself. As far as I can tell via a first pass, it is only exposing the function from the library itself and not utilizing it within the extension otherwise.