Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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'] = ...
Comment | File | Size | Author |
---|---|---|---|
#10 | interdiff-2578789-3-10.patch | 3.83 KB | timhilliard |
#10 | memcache-config_system-2578789-10-8.patch | 29.92 KB | timhilliard |
#8 | interdiff-3-7.txt | 16.84 KB | anavarre |
#7 | memcache-config_system-2578789-7-8.patch | 15.14 KB | timhilliard |
#3 | memcache-config_system-2578789-3-8.patch | 29.93 KB | timhilliard |
Comments
Comment #2
timhilliard CreditAttribution: timhilliard at Acquia commentedComment #3
timhilliard CreditAttribution: timhilliard at Acquia commentedRerolled patch with update to README.txt that reflects the config changes.
Comment #4
timhilliard CreditAttribution: timhilliard at Acquia commentedComment #5
timhilliard CreditAttribution: timhilliard at Acquia commentedComment #6
anavarreWe probably want to require PHP 5.5 or greater to be aligned with core.
Nit: PECL is pretty much always written in caps.
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.
Should we recommend PHP's short array syntax?
Comment #7
timhilliard CreditAttribution: timhilliard at Acquia commentedREADME.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.
Comment #8
anavarreInterdiff for clarity.
Comment #9
nielsvm CreditAttribution: nielsvm as a volunteer and at Acquia commented@timhilliard looking at
memcache-config_system-2578789-7-8.patch
, it seems as if you forgot the other files and only didREADME.txt
. Reroll?Comment #10
timhilliard CreditAttribution: timhilliard at Acquia commented@nielsvm right you are, I rerolled correctly this time :)
Comment #11
anavarreComment #12
damiankloip CreditAttribution: damiankloip commentedSorry for absence on this, I'll review and hopefully merge in the morning. At a glance (on mobile), it looks good.
Comment #13
damiankloip CreditAttribution: damiankloip commentedThanks Tim, overall this is great. I like having that additional abstraction of the MemcacheConfig class too.
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.
Shall we just go with protected here? Then people can extend the class if they like.
Use static:: for all of these. Also, not sure why the property is static itself? This is just a service instance?
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'
'All settings'
Can you convert to new [] array syntax while your there? :)
[]
[]
Comment #14
damiankloip CreditAttribution: damiankloip commentedActually, Maybe I will just commit this, and fix those things myself.
Comment #15
damiankloip CreditAttribution: damiankloip commentedOh, this broke the DUTB too :)
Comment #18
damiankloip CreditAttribution: damiankloip commentedThanks! 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.
Comment #19
damiankloip CreditAttribution: damiankloip commentedWe do have a lock class too :) Can create another issue to tackle this.