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

MUC: Memcache server configuration too sensitive to whitespace

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.4
    • Fix Version/s: 2.4.8, 2.5.4, 2.6
    • Component/s: Caching
    • Labels:
    • Testing Instructions:
      Hide

      NOTE: Test requires access to dataroot directory.

      1. Go to cache configuration page (System admin/Plugins/Caching/Configuration).
      2. Under 'Installed cache stores', click 'Add instance' in the memcache row
      3. For store name, type 'Test'. For servers, type: ' test.example.org ' (with spaces at start and end), then hit return to add a line feed after the single line. Also add a line like ' test.example.org:12345 ' (including extra space)
      4. Save changes.
      5. In the dataroot directory, find the file muc/config.php. Search for 'Test' and look for the servers array.

      EXPECTED: The servers array should have a single entry 0 => 0 => 'test.example.org'.

      BEFORE FIX: The array has two entries, one of which is test.example.org but surrounded by spaces and with a carriage return after it, and the other is an empty string.

      Repeat with Memcached

      Show
      NOTE: Test requires access to dataroot directory. 1. Go to cache configuration page (System admin/Plugins/Caching/Configuration). 2. Under 'Installed cache stores', click 'Add instance' in the memcache row 3. For store name, type 'Test'. For servers, type: ' test.example.org ' (with spaces at start and end), then hit return to add a line feed after the single line. Also add a line like ' test.example.org:12345 ' (including extra space) 4. Save changes. 5. In the dataroot directory, find the file muc/config.php. Search for 'Test' and look for the servers array. EXPECTED: The servers array should have a single entry 0 => 0 => 'test.example.org'. BEFORE FIX: The array has two entries, one of which is test.example.org but surrounded by spaces and with a carriage return after it, and the other is an empty string. Repeat with Memcached
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-42839-master

      Description

      The memcache configuration forms are highly sensitive to whitespace. In particular, if you include a blank line at the end of the form, it will result in unexpected behaviour. This is easy to do when copy-pasting lists of servers; we've done this multiple times in live configuration.

      The server lists should have a basic level of tolerance for whitespace.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            quen Sam Marshall added a comment - - edited

            NOTES:

            1. I decided to call trim() twice rather than duplicate the default argument and add colon, to avoid code duplication since this code is not performance-sensitive.

            2. Like all the existing code, this change is duplicated between memcache and memcached.

            3. This is a minor bug but I guess it is a bug, so I'm making branches for the old releases. These are direct cherry-picks without change.

            4. I don't think this bugfix is critical for 2.6 release; although it is a disturbing (hard to diagnose) problem, it's been like this forever. 2.6.1 should be fine.

            Show
            quen Sam Marshall added a comment - - edited NOTES: 1. I decided to call trim() twice rather than duplicate the default argument and add colon, to avoid code duplication since this code is not performance-sensitive. 2. Like all the existing code, this change is duplicated between memcache and memcached. 3. This is a minor bug but I guess it is a bug, so I'm making branches for the old releases. These are direct cherry-picks without change. 4. I don't think this bugfix is critical for 2.6 release; although it is a disturbing (hard to diagnose) problem, it's been like this forever. 2.6.1 should be fine.
            Hide
            timhunt Tim Hunt added a comment -

            +1 looks good to me. Submitting for integration.

            Show
            timhunt Tim Hunt added a comment - +1 looks good to me. Submitting for integration.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            fred Frédéric Massart added a comment -

            Passing. I took the liberty of amending the testing instructions to test the hosts with a port number, and to mention that the test should happen with Memcached as well.

            Show
            fred Frédéric Massart added a comment - Passing. I took the liberty of amending the testing instructions to test the hosts with a port number, and to mention that the test should happen with Memcached as well.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            It's Friday, I'm tired so I won't be very imaginative today.

            No matter of that, yes, you did it! Thanks for your collaboration!

            Closing this as fixed!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - It's Friday, I'm tired so I won't be very imaginative today. No matter of that, yes, you did it! Thanks for your collaboration! Closing this as fixed!

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13