PHPSocialNetwork / phpfastcache

A high-performance backend cache system. It is intended for use in speeding up dynamic web applications by alleviating database load. Well implemented, it can drops the database load to almost nothing, yielding faster page load times for users, better resource utilization. It is simple yet powerful.
https://www.phpfastcache.com
MIT License
2.36k stars 452 forks source link

Configuration validation issue with Memcached socket (path) #853

Closed leegarner closed 2 years ago

leegarner commented 2 years ago

Configuration

Describe the bug Unable to add a path to a Memcached socket. Adding the path to the server config results in PHP Fatal error: Uncaught Phpfastcache\Exceptions\PhpfastcacheInvalidConfigurationException: Unknown keys for memcached server: path in /tmp/fastcache/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Config.php:121

Adding 'path' to the 2nd array check in Config.php seems to solve the issue, and the socket does work


-- a/private/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Config.php
+++ b/private/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Config.php
@@ -117,7 +117,7 @@ class Config extends ConfigurationOption
             if ($diff = array_diff(['host', 'port', 'saslUser', 'saslPassword'], array_keys($server))) {
                 throw new PhpfastcacheInvalidConfigurationException('Missing keys for memcached server: ' . implode(', ', $diff));
             }
-            if ($diff = array_diff(array_keys($server), ['host', 'port', 'saslUser', 'saslPassword'])) {
+            if ($diff = array_diff(array_keys($server), ['path', 'host', 'port', 'saslUser', 'saslPassword'])) {
                 throw new PhpfastcacheInvalidConfigurationException('Unknown keys for memcached server: ' . implode(', ', $diff));
             }
             if (!is_string($server['host'])) {
@@ -186,4 +186,4 @@ class Config extends ConfigurationOption
         $this->optPrefix = trim($optPrefix);
         return $this;
     }```

**To Reproduce**
Steps to reproduce the behavior:
1. Add a socket path to the servers provided to the config.```$cm = CacheManager::getInstance('Memcached', new MemcachedConfig([
    'servers' => [
        [
            'host' => '127.0.0.1',
            'port' => 11211,
            'path' => '/var/run/memcache/memcached.sock',
            'saslUser' => null,
            'saslPassword' => null,
        ]
    ]
]));```

**Expected behavior**
Normal cache connection and usage

**Screenshots**
n/a

**Additional context**
github-actions[bot] commented 2 years ago

Hello curious contributor ! Since it seems to be your first contribution, make sure that you've been:

Geolim4 commented 2 years ago

Hello,

You right, I missed that. I will push a fix tomorrow for 8.1 and 9.0 versions as well for Memcache driver too.

But for now, time to sleep !

Cheers, Georges

Geolim4 commented 2 years ago

Note for myself:

leegarner commented 2 years ago

Cool, no worries. Glad to know I'm not imagining things.

Geolim4 commented 2 years ago

I will completely refactor this validation part, as it is completely broken and not strict enough along with copy/pasted config from Memcache

leegarner commented 2 years ago

Great. It does seem like if there's a socket path then no need to check for server & port, and vice-versa. Wasn't sure how deep you wanted to delve ;)

Geolim4 commented 2 years ago

I pushed the validation really deep xD

Geolim4 commented 2 years ago

The following test file will give you an idea of how deep the validation is now:

https://github.com/PHPSocialNetwork/phpfastcache/commit/5612795a9c50a710ebb1dfbc82125ed487de8969#diff-ab68cc63ac865591d38d3d60818738423668b5ad0c7be6491c1e080fa193b81d

Geolim4 commented 2 years ago

The tests passes (except for 8.1 but its due a to a Travis bug not pulling php 8.1.x image correctly), so expect a new release by the end of weeks.

You can however, test it on your side before and give me some feedback before if you want to.

leegarner commented 2 years ago

I'll check it out, thanks. It looks like it'll cover all the combinations.

leegarner commented 2 years ago

Ran a couple of quick tests, including actual use on my dev site, and it looks good. Thanks.

Geolim4 commented 2 years ago

Ok, I'll merge that tonight ! Thanks for the feedback.

Geolim4 commented 2 years ago

The fixes has been pushed and releases done

Thank you again.