Skip to content

[Cache] Use options from Memcached DSN #23978

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
Sep 5, 2017

Conversation

Bukashk0zzz
Copy link
Contributor

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

This PR allows to pass options in cache config.
Example:

framework:
    cache:
        app: cache.adapter.memcached
        default_memcached_provider: 'memcached://%env(MEMCACHE_HOST)%?socket_recv_size=1&socket_send_size=2'

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 24, 2017
@Bukashk0zzz Bukashk0zzz force-pushed the cache-memcached-config-parameters branch 2 times, most recently from e85dcab to 264d764 Compare August 25, 2017 06:16
@Bukashk0zzz
Copy link
Contributor Author

There are needed any work on this PR from me?
I saw that tests failed in 3.4 with php 7.1 already.
Haven't any idea why 'Class 'Memcached' not found' in AppVeyor tests

}
}

public function provideDSNWithOptions()
Copy link
Member

Choose a reason for hiding this comment

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

should be provideDsnWithOptions
the provider should do a class_exists check on Memcached and yield nothing when not, in order to allow skipping the tests when Memcached is not installed

@Bukashk0zzz Bukashk0zzz force-pushed the cache-memcached-config-parameters branch from 264d764 to 1363664 Compare August 25, 2017 08:14
@Bukashk0zzz
Copy link
Contributor Author

@nicolas-grekas fixed.

@nicolas-grekas nicolas-grekas self-requested a review August 25, 2017 16:33
@@ -6,6 +6,7 @@ CHANGELOG

* added PruneableInterface so PSR-6 or PSR-16 cache implementations can declare support for manual stale cache pruning
* added FilesystemTrait::prune() and PhpFilesTrait::prune() implementations
* added Using options from Memcached DSN
Copy link
Member

Choose a reason for hiding this comment

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

using without capital U

@Bukashk0zzz Bukashk0zzz force-pushed the cache-memcached-config-parameters branch from 1363664 to 7abc4b2 Compare August 27, 2017 07:46
@Bukashk0zzz
Copy link
Contributor Author

Updated

@@ -139,11 +117,34 @@ public static function createConnection($servers, array $options = array())
if (isset($params['query'])) {
parse_str($params['query'], $query);
$params += $query;
$options += $query;
Copy link
Member

Choose a reason for hiding this comment

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

this means that the precedence is:

  1. options provided as 2nd argument
  2. win over options provided as part of the DSN
  3. who then win over defaults.

But in RedisTrait, 1. and 2. are swapped:

  1. options provided as part of the DSN
  2. win over options provided as 2nd arg
  3. who then win over defaults.

This allows "the infrastructure" to take over the app config.
I think it also makes sense to do the same here.

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.

see comment about precedence

@Bukashk0zzz Bukashk0zzz force-pushed the cache-memcached-config-parameters branch from 7abc4b2 to 94ad162 Compare September 5, 2017 06:14
@Bukashk0zzz Bukashk0zzz force-pushed the cache-memcached-config-parameters branch from 94ad162 to 5798dd6 Compare September 5, 2017 06:16
@Bukashk0zzz
Copy link
Contributor Author

@nicolas-grekas Fixed. Also updated tests for this case.

@fabpot
Copy link
Member

fabpot commented Sep 5, 2017

Thank you @Bukashk0zzz.

@fabpot fabpot merged commit 5798dd6 into symfony:3.4 Sep 5, 2017
fabpot added a commit that referenced this pull request Sep 5, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

[Cache] Use options from Memcached DSN

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

This PR allows to pass options in cache config.
Example:
```yaml
framework:
    cache:
        app: cache.adapter.memcached
        default_memcached_provider: 'memcached://%env(MEMCACHE_HOST)%?socket_recv_size=1&socket_send_size=2'
```

Commits
-------

5798dd6 [Cache] Use options from Memcached DSN
This was referenced Oct 18, 2017
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.

4 participants