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

Change the approach of the PSR autoloading and allow PSR-4

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0, 3.2
    • Fix Version/s: 3.2
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide
      Basic class loading test
      1. Purge caches
      2. Run the attached 53016.php file
        1. Confirm that all lines have all three columns filled
      Test Requirements

      For these tests you will need:

      1. outbound mail to be correctly configured and working on your system (you can use mailcatcher, just view the email source and copy the Reply-To address. Ask John for info)
      2. a test e-mail account (John can provide you with details of one, or you can create a new Gmail account)
      3. Purge your caches
      Initial setup
      1. Setup VERP handling:
        1. Open Site administration -> Server -> Incoming mail configuration -> Mail settings
          1. Enable incoming mail processing;
          2. Add your inbound mailbox details
          3. Save changes
        2. Open Site administration -> Server -> Incoming mail configuration -> Message Handlers
          1. Enable the private files handler
      2. Reduce the max editing time of your forum messages to preserve testing sanity
        1. Open Site administration -> Security -> Site policies
        2. Set the maxediting time to 1 minute
      3. Create a new course
      4. Enrol a couple of users - you probably only want one; or two
      5. Create a new forum in that course - choose auto subscription
      Test reply by email
      1. Add a post as one of the users
      2. Check the email of one of the other users
      3. Reply to that email
      4. Run php ./admin/tool/task/cli/schedule_task.php --execute=\\tool_messageinbound\\task
        pickup_task
      5. Ensure the reply is posted
      Show
      Basic class loading test Purge caches Run the attached 53016.php file Confirm that all lines have all three columns filled Test Requirements For these tests you will need: outbound mail to be correctly configured and working on your system (you can use mailcatcher , just view the email source and copy the Reply-To address. Ask John for info) a test e-mail account (John can provide you with details of one, or you can create a new Gmail account) Purge your caches Initial setup Setup VERP handling: Open Site administration -> Server -> Incoming mail configuration -> Mail settings Enable incoming mail processing; Add your inbound mailbox details Save changes Open Site administration -> Server -> Incoming mail configuration -> Message Handlers Enable the private files handler Reduce the max editing time of your forum messages to preserve testing sanity Open Site administration -> Security -> Site policies Set the maxediting time to 1 minute Create a new course Enrol a couple of users - you probably only want one; or two Create a new forum in that course - choose auto subscription Test reply by email Add a post as one of the users Check the email of one of the other users Reply to that email Run php ./admin/tool/task/cli/schedule_task.php --execute=\\tool_messageinbound\\task pickup_task Ensure the reply is posted
    • Affected Branches:
      MOODLE_30_STABLE, MOODLE_32_STABLE
    • Fixed Branches:
      MOODLE_32_STABLE
    • Epic Link:
    • Pull from Repository:
    • Pull Master Branch:
      MDL-53016-master

      Description

      In core_component we have a facility to autoload PSR compliant libraries (it says psr-0 in phpdocs, but think it could be restricted to psr-4) which landed in MDL-47195. It's currently used by horde, and I am using it for GeoIp2 in MDL-48766).

      I'm not a fan of the current approach - because:

      a) we are bloating (169 horde classes atm) the core_component cache with this third party stuff. I'm not clear exactly why we decided to define the classmap in its entirety in the cache for our own classes - but I feel like we certainly shouldn't need it for third party stuff, and we can keep this cleaner by avoiding it. We can load on demand if namespace matches (see below code snippet) .

      b) This is a lesser point than a) but I'm not totally convinced that we should keep these third party libs always ready for inclusion. For example with horde, we only really use it one file, in MDL-48766 it is only needed in one place, and there was talk of using it for google in MDL-51223 - also in only one place.
      It just feels cleaner to me to have the libs restricted in their inclusion.

      We could fix a) regardless of b), but I think the two combined would mean we could consider doing something like:

      core_component::autoload_namespace('Horde', $CFG->libdir.'/horde/framework/');
      

      (as an equivalent to of require_once) in admin/tool/messageinbound/classes/manager.php and it wouldn't have much impact for the use of the library and would keep core clean of it. In fact I made a POC implementation of this:

      /**
       * Register a namespace in PSR-4 standard with autoloader.
       *
       * Adapated from http://www.php-fig.org/psr/psr-4/examples/
       *
       * @param string $prefix The namespace to autoload classes from.
       * @param string $path The fully qualified path to autoload the classes from.
       */
      function autoload_psr4_namespace($prefix, $path) {
          spl_autoload_register(
              function ($class) use ($prefix, $path) {
                  // Does the class use the namespace prefix?
                  $len = strlen($prefix);
                  if (strncmp($prefix, $class, $len) !== 0) {
                      // No, move to the next registered autoloader.
                      return;
                  }
      
                  // Get the relative class name.
                  $relativeclass = substr($class, $len);
      
                  // Replace the namespace prefix with the base directory, replace namespace
                  // separators with directory separators in the relative class name, append
                  // with .php.
                  $file = $path. str_replace('\\', '/', $relativeclass) . '.php';
      
                  // If the file exists, require it.
                  if (file_exists($file)) {
                      require($file);
                  }
              }
          );
      }
      

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    5/Dec/16