Allow for configuration to take the form of

$settings['memcache'] = array(
  'servers' => array('127.0.0.1:11211' => 'default'),
  'bins' => array('default' => 'default')
);

OR

$settings['memcache'][ 'servers'] = array('127.0.0.1:11211' => 'default');
$settings['memcache']['bins'] = array('default' => 'default');

rather than

$settings['memcache_servers'] = ...
$settings['memcache_bins'] = ...
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timhilliard created an issue. See original summary.

timhilliard’s picture

timhilliard’s picture

Rerolled patch with update to README.txt that reflects the config changes.

timhilliard’s picture

Issue summary: View changes
timhilliard’s picture

Issue summary: View changes
anavarre’s picture

  1. +++ b/README.txt
    -- PHP 5.1 or greater
    +- PHP 5.4 or greater
    

    We probably want to require PHP 5.5 or greater to be aligned with core.

  2. +++ b/README.txt
    -- PHP 5.1 or greater
    +memcache pecl extensions, please see the documentation online at
    

    Nit: PECL is pretty much always written in caps.

  3. +++ b/README.txt
    -- PHP 5.1 or greater
    + 1. Put your site into offline mode.
    

    Are we talking about maintenance mode? If yes, then that's probably what we should explicitly convey.

    Also it seems we'd now be assuming you already have the PECL memcache extension installed.

  4. +++ b/README.txt
    -- PHP 5.1 or greater
    +      $settings['memcache']['servers'] = array('127.0.0.1:11211' => 'default');
    

    Should we recommend PHP's short array syntax?

timhilliard’s picture

README.txt updated to address #6

In installation I removed the need for maintenance mode (I've never used it before when enabling the memcache module). I also added as number 1 to check one of the PECL memcache packages is installed, although that is also listed in requirements.

anavarre’s picture

FileSize
16.84 KB

Interdiff for clarity.

nielsvm’s picture

@timhilliard looking at memcache-config_system-2578789-7-8.patch, it seems as if you forgot the other files and only did README.txt. Reroll?

timhilliard’s picture

@nielsvm right you are, I rerolled correctly this time :)

anavarre’s picture

Status: Active » Needs review
damiankloip’s picture

Sorry for absence on this, I'll review and hopefully merge in the morning. At a glance (on mobile), it looks good.

damiankloip’s picture

Status: Needs review » Needs work

Thanks Tim, overall this is great. I like having that additional abstraction of the MemcacheConfig class too.

  1. +++ b/src/DrupalMemcacheConfig.php
    @@ -0,0 +1,63 @@
    +class DrupalMemcacheConfig {
    

    I would like to name all the classes better. I.e. Drop the Drupal part, as this was mainly an artefact from the port from 7.x. This is fine here though, I can do that in a follow up that fixes all the classes.

  2. +++ b/src/DrupalMemcacheConfig.php
    @@ -0,0 +1,63 @@
    +  private static $settings = array();
    

    Shall we just go with protected here? Then people can extend the class if they like.

  3. +++ b/src/DrupalMemcacheConfig.php
    @@ -0,0 +1,63 @@
    +    self::$settings = $settings->get('memcache', array());
    

    Use static:: for all of these. Also, not sure why the property is static itself? This is just a service instance?

  4. +++ b/src/DrupalMemcacheConfig.php
    @@ -0,0 +1,63 @@
    +   * Returns all the settings. This is only used for testing purposes.
    

    Remove the part about testing. Getting all settings I think is OK anyway. And keeps parity with config in general. This line could just read 'Returns all Memcache settings'

  5. +++ b/src/DrupalMemcacheConfig.php
    @@ -0,0 +1,63 @@
    +   *   All the settings.
    

    'All settings'

  6. +++ b/src/DrupalMemcacheFactory.php
    @@ -160,14 +159,14 @@ class DrupalMemcacheFactory {
    +    $this->memcacheServers = $this->settings->get('servers', array('127.0.0.1:11211' => 'default'));
    

    Can you convert to new [] array syntax while your there? :)

  7. +++ b/src/DrupalMemcached.php
    @@ -31,7 +29,7 @@ class DrupalMemcached extends DrupalMemcacheBase {
    +    foreach ($this->settings->get('options', array()) as $key => $value) {
    

    []

  8. +++ b/src/Tests/DrupalMemcacheConfigTest.php
    @@ -0,0 +1,77 @@
    +    $this->config = array(
    

    []

  9. The test class also should just be in the tests dir instead.
damiankloip’s picture

Actually, Maybe I will just commit this, and fix those things myself.

damiankloip’s picture

Oh, this broke the DUTB too :)

  • damiankloip committed 3e0cfaa on 8.x-2.x
    #2578789 Follow up - Fix MemcacheBackendUnitTest
    
  • damiankloip committed 417aa3e on 8.x-2.x authored by timhilliard
    Issue #2578789 by timhilliard, anavarre, damiankloip, nielsvm: Switch to...
  • damiankloip committed 6a55bb2 on 8.x-2.x
    #2578789 Follow up - Move DrupalMemcacheConfigTest to unit test...

  • damiankloip committed d1d1a98 on 8.x-2.x
    #2578789 Follow up - Remove statics from DrupalMemcacheConfig, misc code...
damiankloip’s picture

Status: Needs work » Fixed

Thanks! closing this one out.

Note: I removed the static method impmelemntation, seems we don't really need that, and the memcache classes were already using the setting object as instances. If that is not OK for any reasn, let me know.

damiankloip’s picture

+++ b/README.txt
@@ -250,43 +146,16 @@ memcache.hash_strategy=consistent
+Locks have not yet been implemented using the memcache module.

We do have a lock class too :) Can create another issue to tackle this.

  • damiankloip committed 222f62f on 8.x-2.x
    #2578789 Follow up - Small code clean up for DrupalMemcacheConfig
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.