Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40700

MUC: memcache instance key prefix does not allow numeric values

    Details

    • Testing Instructions:
      Hide

      Add memcache store
      Fill out fields as normal
      Change key prefix to use non-alphanumeric characters
      Save changes

      Click "Edit store"
      (Behaviour before patch)
      Expected result: key prefix is as set.
      Actual result: key prefix has non-alpha chars stripped out.

      (Behaviour after patch)
      On "Save changes" - form responds with error for characters other than [a-zA-Z0-9\-_ ]
      On "Edit store" - key prefix is set as expected.

      Show
      Add memcache store Fill out fields as normal Change key prefix to use non-alphanumeric characters Save changes Click "Edit store" (Behaviour before patch) Expected result: key prefix is as set. Actual result: key prefix has non-alpha chars stripped out. (Behaviour after patch) On "Save changes" - form responds with error for characters other than [a-zA-Z0-9\-_ ] On "Edit store" - key prefix is set as expected.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-40700-m26

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            chrisw 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
            chrisw 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
            samhemelryk Sam Hemelryk added a comment -

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

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks for your work on this Chris, I'll have a play with it tomorrow.
            Hide
            samhemelryk 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
            samhemelryk 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
            samhemelryk Sam Hemelryk added a comment -

            Oops forgot to update a few things

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

            Integrated (24, 25 & master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (24, 25 & master), thanks!
            Show
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - MDLSITE-2356 created about to ensure unit tests runs are complete anot skipping anything in CI servers.
            Hide
            jerome 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
            jerome 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 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 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:
                  Fix Release Date:
                  9/Sep/13