Moodle
  1. Moodle
  2. MDL-40700

MUC: memcache instance key prefix does not allow numeric values

    Details

    • Rank:
      51554

      Description

      When configuring a memcache store, the key prefix field allows the user to submit numbers, and save the changes.
      Because the form definition uses ALPHAEXT for this field, the numbers are stripped out silently.
      A simple fix is to change the field type to ALPHANUMEXT, and add some form validation.

      diff --git a/cache/stores/memcache/addinstanceform.php b/cache/stores/memcache/addinstanceform.php
      index 984966a..29e1239 100644
      --- a/cache/stores/memcache/addinstanceform.php
      +++ b/cache/stores/memcache/addinstanceform.php
      @@ -49,7 +49,7 @@ class cachestore_memcache_addinstance_form extends cachestore_addinstance_form {
               $form->addElement('text', 'prefix', get_string('prefix', 'cachestore_memcache'),
                       array('maxlength' => 5, 'size' => 5));
               $form->addHelpButton('prefix', 'prefix', 'cachestore_memcache');
      -        $form->setType('prefix', PARAM_ALPHAEXT);
      +        $form->setType('prefix', PARAM_ALPHANUMEXT);
               $form->setDefault('prefix', 'mdl_');
           }
      

      Is there a reason behind not allowing numeric prefixes? With a 5 character limit, very few sites can use the same memcache servers, and have meaningful key prefixes.

        Issue Links

          Activity

          Hide
          Chris Wharton added a comment -

          I have added a patch for this issue.
          It may be worth checking if the key prefixes are duplicated between stores as well.

          Show
          Chris Wharton added a comment - I have added a patch for this issue. It may be worth checking if the key prefixes are duplicated between stores as well.
          Hide
          Sam Hemelryk added a comment -

          Thanks for your work on this Chris, I'll have a play with it tomorrow.

          Show
          Sam Hemelryk added a comment - Thanks for your work on this Chris, I'll have a play with it tomorrow.
          Hide
          Sam Hemelryk added a comment -

          Thanks Chris, I've taken your patch and made a couple of minor changes. I'm now pushing this for integration review so that we can get it in.

          The changes were as follows:

          1. Moved the lang string to the cache store lang file.
          2. Applied the same change to the memcached prefix.
          3. Added unit tests for both cache stores.
          4. Removed the ability to use spaces after finding issues with their handling.

          Integrator: although this could be seen as an improvement I think it is fair to view it as a bug of the original prefix implementation which was being too restrictive.
          As it largely a safe change I would purpose this gets backported. Of course the final decision is yours

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Chris, I've taken your patch and made a couple of minor changes. I'm now pushing this for integration review so that we can get it in. The changes were as follows: Moved the lang string to the cache store lang file. Applied the same change to the memcached prefix. Added unit tests for both cache stores. Removed the ability to use spaces after finding issues with their handling. Integrator: although this could be seen as an improvement I think it is fair to view it as a bug of the original prefix implementation which was being too restrictive. As it largely a safe change I would purpose this gets backported. Of course the final decision is yours Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Oops forgot to update a few things

          Show
          Sam Hemelryk added a comment - Oops forgot to update a few things
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (24, 25 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (24, 25 & master), thanks!
          Show
          Eloy Lafuente (stronk7) added a comment - Baboom! (25_STABLE and master) 25: http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20(25_STABLE)/221/console master: http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20(master)/1387/console (24 not failing, no tests there) Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Just guessing if we should mark those tests as skipped if the servers (aka the instances) are not available...uhm..

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Just guessing if we should mark those tests as skipped if the servers (aka the instances) are not available...uhm..
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've added a commit verifying the store has been really instantiated. Applied both to master and 25:

          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=e71c47bf8c78b30920945c6e30ffd61e56dec487

          Feel free to modify it, but I think that will cause CI servers to start passing (by skipping) again. Also I'm creating an issue about to prepare everything in CI servers to get 0 skipped tests there.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I've added a commit verifying the store has been really instantiated. Applied both to master and 25: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=e71c47bf8c78b30920945c6e30ffd61e56dec487 Feel free to modify it, but I think that will cause CI servers to start passing (by skipping) again. Also I'm creating an issue about to prepare everything in CI servers to get 0 skipped tests there. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          MDLSITE-2356 created about to ensure unit tests runs are complete anot skipping anything in CI servers.

          Show
          Eloy Lafuente (stronk7) added a comment - MDLSITE-2356 created about to ensure unit tests runs are complete anot skipping anything in CI servers.
          Hide
          Jérôme Mouneyrac added a comment -

          As it's the first time I installed memcache, I explain what I did on MacOSX:

          • brew install memcached
          • Follow the instruction at the end of the install process.
            From here you should have the memcached server installed and starting at every startup.
          • brew install libmemcached
          • sudo ./pecl install memcached
          • add extension = memcached.so to your /private/etc/php.ini
          • sudo apachectl restart
            From here you should see memcached instance in Admin > Plugins > Chaching > Configuration
          • sudo ./pecl install memcache
          • add extension = memcache.so to your /private/etc/php.ini
            From here you should see memcache instance available
            PS: some command may be skipped but it is just my command line history, this is working for sure.

          All tested passed, thanks.

          Show
          Jérôme Mouneyrac added a comment - As it's the first time I installed memcache, I explain what I did on MacOSX: brew install memcached Follow the instruction at the end of the install process. From here you should have the memcached server installed and starting at every startup. brew install libmemcached sudo ./pecl install memcached add extension = memcached.so to your /private/etc/php.ini sudo apachectl restart From here you should see memcached instance in Admin > Plugins > Chaching > Configuration sudo ./pecl install memcache add extension = memcache.so to your /private/etc/php.ini From here you should see memcache instance available PS: some command may be skipped but it is just my command line history, this is working for sure. All tested passed, thanks.
          Hide
          Damyon Wiese added a comment -

          Thanks again for another week of fixes, improvements and testing. These changes have been released to the world.

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - Thanks again for another week of fixes, improvements and testing. These changes have been released to the world. Cheers, Damyon

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: