Plugins: Swappable subsystems for core

Add #352951: Make JS & CSS Preprocessing Pluggable to the list.

So we wrote DB:TNG and only now, after having like 1-2 months not working constantly on it, it dawns on me how hard it is to follow the codeflow of that. I have written #363687: DB:TNG codeflow is impossible to follow and I have more to follow up that, probably as soon as tomorrow (just a bunch of inline comments to help read). I am squarely opposed to adding another convoluted OOP subsystem to Drupal until we are very, very sure we are going the right direction and the resulting code is easy to understand.

I'm pretty sure that #358663: Implement a locking framework and make caching pluggable should be on the list too.

I think it would be beneficial to store slot_load() and handler_load() in the cache system. Right now it makes a call straight to the database if it's not statically cached....

Subscribe.

subscribe.

subscribe. I'll be giving this a good look over in the next couple days.

subscribing

This would also be useful for #64866: Pluggable architecture for drupal_http_request(). Subscribing.

Before I nitpick the code I'd like to hear back from Dries or webchick on the overall concept.

Overall, I like the concept. Reading the tests is a good way to get an understanding on handlers and how to use them.

What I don't like is the 2 queries per hander use in most cases. First, you do a slot_load() to get the factory function (1 query required) and then you call handler_factory_default() which looks up the class to call (1 query required). The second query can be removed if you have your own factory function. The proposed solution in #6 would work for me, though.

I was, also, wondering why we have interfaces defined for the slots but we aren't checking that a class implements the interface before we instance it.

This patch is a kitten-killer in wanting to do at least to radically different things at the same time:

• Making Drupal stateful and easier to test by implementing a DrupalEnvironment class
• Implementing a generic framework for pluggeable subsystems

Those have (different) impact on the code base, and could easily be implemented separately. There is no real need to mix them.

Having thought about this, I doubt we really need a generic framework for our pluggeable subsystems. Simply using a proper Strategy pattern consistently across core would be more than enough. I doubt that most factory functions will share the same code anyway.

What I would like to see is simply a way to implement consistent "routing tables" defined in settings.php, as discussed already elsewhere.

The only reason then to have your own factory would be if you had pparticularly odd logic, or if you wanted a cleaner function name to call directly. (E.g., cache('page')->set() instead of handler('cache', 'page')->set()).

That's a damn good reason.

cache($cache_id, $cache_bin)->get();
email($recipient)->send($mail);
password_generator($user)->hash($password);
http_request($url)->post();

None of them can use a one-size-fits-all, mono dimensional approach.

My conclusion: factoring all this into a single implementation serves no purpose.

Plus:

handler('cache', $cache_id, $cache_bin)->get();

is way uglier than:

cache($cache_id, $cache_bin)->get();

I'm generally in agreement with Damien - I think the environment object is a very useful idea on its own.

There doesn't to me appear to be a compelling reason to have a system for handlers so much as a pattern as Damien suggests. A few of these, especially the cache subsystem, need some special handling because they come into play before other systems are initialized -but let's not make all the rest extra complex as a side effect.

@Crell - if you look at my patch for the (very simple) e-mail back-end interface, a major feature is that you can specify via the factory a different class to handle the sending based on the message ID or the module that's sending the e-mail. I can readily imagine cases where one would want 2 or 3 different mail back-ends to be active in parallel.

@pwolanin Where and how are you suggesting we store the configuration for these type systems? The settings.php file won't work because you can't have a gui that alters the config. What is your proposed alternative? The variable system?

If we don't go this route we need an alternative way because this is popping up in a lot of active patches. What's the alternative?

Storing the whole slot registry in the cache system, and storing its configuration in the variable system won't work?

@Rob Loach if we use the caching system to store the information we can't use handler system for the caching system.

What if we do something similar to how the variable system works. In the settings.php file we have $slots and $handlers. If caching and the variables aren't initialized we are using data from the settings.php file. When they get initialized we add more data to them for other systems. See variable_init and it's usage in the bootstrap process to see what I mean.

This would give us the performance boost by allowing the config to be in the settings.php file and it would cut down the lookup to one cache lookup for each handler use. But, the storage for the config would be the variable system. Do we want to store them there?

Subscribe.

Looks fairly good. Like some of how handlers like this could work.

the db query in handler_factory_default() has a problem where the argument is 'slot' instead of ':slot'

I'm throwing a 3rd implementation for handlers/routing into the ring. See #367282: A simple handler solution. It can be used before the database/registry is available (example included). It's rough and needs some work but the concept is there with notes.

@mfer So from a quick glance thought this I took out that it removes the interfaces, the environment object and stores the information in the variable table. I'm not sure that really is so much a 3rd approach as a toned down version of this patch. chx's patch is quite a bit different approach in that it directly maps functions to individual implementations. No concept of handlers in the way these to patches group them(objects).

I think the interfaces isn't a bad thing so I'm not really sure why its gone. It does have some advantages like forcing the proper implementations of the constructor at the language level. The constructor is also gone which I think might be a bit of a mistake since it means the handler looses its context.

The environment object is something I'm a bit on the fence about. My initial notes on this patch have a big Kitten Killer scribbled down much like your earlier comment. But then I talked to Crell about why he decided to put that in this patch and he did convince me it has some merits. Accessing the subsystem of Drupal through this object does feel a bit like overkill but then having some things setup and available to you like the handler function are really handy. It also demonstrates some of the design reasoning to me for things like the interfaces and some of the algorithms.

I do however feel the env system could actually carry itself as its own patch despite Crell's concerns. If nothing else, for its testing benefits. Also, I think this particular patch could work just as well without it. So like I said, on the fence.

The variable system storage I feel is a bad choice. It does give you that $conf settings.php hackery but it just feels bad. There's the possibility for a lot of information being stored here and this just feels like a really bad idea. What if a system like views used these handlers for example? I'd rather the previous patch get the ability to be setup in setting.php.

So my general feeling is that while leaner, this implementation doesn't net us much gain.

You can ignore my last comment I guess. It seem that patch is being worked on at #367282: A simple handler solution now.

@crell what about storing the handlers config in a sqlite database. It could be available much earlier then. It could be in the same database as registry system if we go that way.

@mfer that's a separate issue. @damz we need the hooks and the central system to make it possible for an UI to be written.

Overall I'm a fan of this patch. It's simpler than the last go around. The environment object has been removed (but can and should be added back in another patch. It would be good for testing.) This is similar to the simple handlers patch I rolled so I've closed that one to go this route instead. It better solves some of the issues run into there.

Crell (or anyone else), can you provide a use case for the reuse flag? This came up in the patch I wrote as a question. What would be a case when we don't reuse a handler? I imagine one exists but can't think of one and was challenged not to have the feature and always reuse it.

If we go with a signature of handler_attach($slot, $target, $handler) $target will no longer be an optional parameter. For a majority of the use cases there will be just one target. I'd like to see target optional everywhere and default to 'default'. Though, this does lead to some inconsistencies in the signatures.

Some finer details in the patch itself:

• handler_factory_default() in handlers.inc is not used in this patch. Looks like a holdover from the last version of the patch.
• There's a .htaccess rewritebase in the patch from Crells dev enviornment.
• The comment on handler() still refers to calling the proper factory. This is no longer the case and should be updated.
• In the comment for hander_attach() it says, "Asociate a given handler to a give slot for the specified target." Notice the misspelling of associate.
• Is there a reason require_once is used in handlers_rebuild to load handlers.inc and drupal_function_exists() is used in cases like handler_attach() to lazy load the functions (and handlers.inc)? Is it because handler_build is used in cases where the registry isn't available (like installation?) If not this is inconsistent. If it is handlers_rebuild might need to be inserted into install.php somewhere (and maybe update.php). Also, would handler_attach()/detach() need to be called in an install profile (I think so)? Is the registry available there?
• There is a double period in the comment about handlers in the defgroup for handlers in handlers.inc.

These comments are just what popped out at me on a quick pass. I'll look it over in more detail later.

After this patch lands I'll roll a patch to add a variable to settings.php to allow for them to be configured there (unless someone else wants to). This will be important for systems like the cache system.

What about this? This is similar to what core does for languages:

function handler($slot_id, $target = NULL) {
  static $handlers;
  if (!isset($target)) {
    $target = '';
  }

  if (empty($handlers[$slot_id][$target])) {
    $record = db_query_range("SELECT class, reuse FROM {handler_attachments} WHERE slot = :slot_id AND target IN (:target_1, :target_2) ORDER BY target DESC" array(
      ':slot_id' => $slot_id,
      ':target_1' => $target,
      ':target_2' => '',
    ), 0, 1)->fetchObject();

and yes, the empty string is a fine array index.

I dunno why that's better than the UNION, we should ask some MySQL experts and make languages and handlers use the same construct.

Crossposted, sry!

@crell I was thinking the variable in settings.php would be $handlers but that's another issue. I agree, it shouldn't be in $conf.

http://www.mysqlperformanceblog.com/2007/10/05/union-vs-union-all-perfor... both UNION ALL and UNION distinct use temporary table for result generation. definitely not good!

About the sort order, sure, this is not NULL which causes all sorts of weirdness.

Subscribing by request of chx. I need to look into the UNION vs. OR issue.

Subscribing.

I have asked MySQL Enterprise Support whether

SELECT class, reuse FROM handler_attachments WHERE slot = $slot AND target = $target
UNION
SELECT class, reuse FROM handler_attachments WHERE slot = $slot AND target = '' LIMIT 1

or SELECT class, reuse FROM handler_attachments WHERE slot = $slot AND target IN ($target, '') ORDER BY target DESC LIMIT 1 is faster. The answer is, the first query is incorrect, you wanted

(SELECT class, reuse, target FROM handler_attachments WHERE slot = $slot AND target = $target) 
UNION
(SELECT class, reuse, target FROM handler_attachments WHERE slot = $slot AND target = '')
ORDER BY target DESC LIMIT 1;

and

If there are a large number of records which match either $target or '', your second query would be faster.

I can ask further clarification what's the case for a few records, I bet the answer is that for a few records the results are approximately the same.

As

(SELECT class, reuse FROM handler_attachments WHERE slot = $slot AND target = $target) 
UNION
(SELECT class, reuse FROM handler_attachments WHERE slot = $slot AND target = 'default') LIMIT 1

relies on the fact that the result from the first query comes first and the UNION'd results come after. Relying on the order of any result set without an explicit ORDER in SQL leads to unpredictable result. Because of this I recommend renaming 'default' to ''. This allows you to easily order the result set and does not make the 'default' word a taboo for targets. And changing the UNION to OR because it's a shorter query.

Did not want to remove the critical priority.

There is no need to expose the '' to the end user just make it target optional, done. Once again, relying on the order of any result set without an explicit ORDER in SQL leads to unpredictable result. We are working as an ANSI SQL system not one that holepefully works...

If you are bent on 'default' then add a weight and index on that. Edit: weight in the DB, 0 for default, 1 for everything else, not visible to end users, and order on that.

Edit2: language subsystem already uses empty string for language neutral.

It's not bikeshedding at all. It's better queries and code I am after. All I asked for was renaming 'default' to '' and I adjusted the code already showing that so if you ask for target 'foo' and that's not found it'll look for the empty target which is the default.

Now, review

1. Store this overdiscussed SQL and args in variables so when you re-run it, it's less code.
2. You want to check target in the retrieved record before creating a new handler. It very well might that you netted the default and if it's reusable, then win, you do not need to create a new one.
3. No review is complete without whitespace complaints. handler_ensure_defaults query looks untidy and you added a new empty line to path.test.
4. In the past, variable_get in url() indicated that sometimes it is worthy to static cache something like handler in drupal_get_path_alias to save one function call and an array lookup by its key, it got removed in #356721: Regression: Test failures with clean URLs. I would love to see the comment here enhanced by adding a few words about performance ("improve performance") instead of "avoid lookup" and a moment of meditation over whether we will get a bug here because of the static or not.
5. Also care to mention that the two path wrapper static cached the same object?
6. Add a big fat TODO to the admin screen that this needs to be extended to some sort of generic UI. Which is not the concern of this patch.

   so far I found these. Nice that path tests pass!

yessir! working on it.

Here we come. It not just passes the tests but there is more caching, too.

With the numerous new files.

Hopefully this version of handler() is easier to understand. Tests still pass.

[chx said he had a new patch so I've only reviewed includes/handlers.inc from #65]

Since I'm unclear on how all these pieces fit together I'm mostly reviewing it from a documentation standpoint. First off let me say the docs are a great starting point I was able to read them and get a rough idea of how the pieces work together so kudos for that. That said now I'm going to get all anal about this while it's still new to me and the shortcomings still apparent.

+ * Slot
+ * A slot is a system or subsystem which we want to be easily swappable. A slot
+ * is defined by a name and an interface. For example, the Cache system is a
+ * "slot".  All handlers for a given slot must conform to the PHP interface
+ * defined by the slot.

Since it doesn't seem like the Cache system is implemented in this patch could we reference the "Path lookup system"? And specifically list the name (path_lookup) and interface (PathAliasInterface) that way I could go look at them if I was so inclined I'd also make it clear that it's an interface in the PHP OOP sense (I think listing the class name will help).

+ *
+ * Handler
+ * A Handler is a class that implements the interface for a given slot. For example,
+ * a database cache implementation and a memcache cache implementation are both
+ * handlers for the Cache slot.  Handlers may be transparently swapped out for
+ * one another.

Same goes here. I'd stick to concrete examples that are in core rather than plan to be in core. So lets reference PathAliasPrecache and PathAliasLazy as two alternative Path lookup systems that can be swapped out.

+ * Targets
+ * A target is a specific case within a slot. Every slot may have an unlimited
+ * number of arbitrarily defined targets, and every target may have a different
+ * handler attached to it.  For example, "page", "menu", and "filter" are
+ * different targets used by Drupal for the Cache slot.

This could use some work... I've got no idea what "specific case within a slot" means... Who defines the targets? The Handler? Target? Caller? It's not clear. The menu example helps but it would be nice to follow the Path lookup example all the way through.

I'd really emphasize that you pick the handler per Target. So you could associate different algorithms to different Targets for performance reasons.It seems like that's a big part of the win of the whole handlers system and explaining that will justify making someone figure out the complexity.

+ * @code
+ * handler_attach('slot_name', 'handler_name', 'target');
+ * @endcode
+ *
+ * That will overwrite any previously associated handler. A handler may then
+ * be requested using the handler() factory function:
+ *
+ * @code
+ * $handler = handler('slot_name', 'target');
+ * $handler->someMethod();
+ * @endcode
+ *
+ * If no handler has been attached to that slot and target, the handler attached
+ * to the target "default" is used instead.  A slot/target may also be "reset"
+ * to the default handler:
+ *
+ * @code
+ * handler_detach('slot_name', 'target');
+ * @endcode
+ *

I appreciate seeing the examples but it would be better to see them with real code. Paths perhaps?

+ * If no target is specified, "default" is assumed.  That allows for a given
+ * slot to not make use of the system's multi-target capabilities by simply
+ * omitting the target whenever it is called, which results in a target of
+ * "default" being used in all cases.

Where are the defaults defined? Does the Handler come with a default? How is that associated.

+/**
+ * Rebuild the handler slot info registry.
+ *
+ * @return
+ *   The array of slots just declared.
+ */
+function handler_slot_build() {

Does this operate on the last handler added or all handlers? the "just declared" has me wondering.
Could we get some @see tags to the hook(s) this invokes?

+/**
+ * Rebuild the handler info registry.
+ *
+ * @return
+ *   The array of handlers just defined.
+ */
+function handler_handler_build() {

The function name and description seem at odds... build or rebuild?
Again, could we get some @see tags to the hook(s) this invokes?

+     ->updateExcept('class', 'handler')
+    ->execute();
+
+	  // If there's no attachments for this slot other than the default, cache
+	  // that to the variable system for faster lookups.
+	  $attachments = db_query("SELECT target, class, reuse FROM {handler_attachments} WHERE slot = :slot", array(':slot' => $record->slot))->fetchAllAssoc('target', PDO::FETCH_ASSOC);
+	  if (count($attachments) == 1 && key($attachments) == 'default') {
+	    $attachments = current($attachments);
+	    unset($attachments['target']);
+	    variable_set('handler_default_' . $record->slot, $attachments);
+	  }
+  }
+
+
+
+}

Has some tab indenting problems and extra new lines.

BTW, handler() is missing a PHPdoc for the $reset parameter.

Worked a bit the comments. Assigning to myself until Crell swears to Druplicon that he will fix his IDE and comment style and neither post patches with TABs nor with period-space-spaces in them. I hope I killed all of them.

This looks very interesting for swappable Field API storage engines.

man crell and his dot space space was pervasive... though at least it's clear where all the great documentation came from:

1. +      // Try to get the associated handler.  If the first query finds an associated
   +      // handler, we use that.  If not, the second query will always find the
   +      // default handler.  By UNIONing them together we get the fallback default
   

2. + * behavior and a way to type-check a handler object.  Interfaces for specific
   

3. +  // slot-defined default handler.  That ensures that we always have at least
   

4. +  // that doesn't cause a problem.  Note that because objects pass by handle
   

5. +    // Now request the handler.  We should get back the default.
   

6. +      'description' => 'This slot does nothing.  It\'s just to test lookup behavior of reusable slots.',
   

7. +         'description' => "This handler doesn't do anything to the string.  It passes through unaltered.",
   

8. + * appropriate interface.  How they do so is entirely up to them.
   

9. + * This hook should return a nested array of slot definitions.  Each key
   

10. + *   The PHP interface that defines this slot.  The interface must extend
    

11. + *   in hook_handler_info().  All slots must have at least one handler
    + *   available.  If not specified, "default" is used.
    + * reuse (optional)
    + *   By default, if a given target on a slot is requested multiple times in
    + *   the same page request the same object will be returned each time.  The
    + *   object will be statically cached.  To disable that behavior and return
    + *   a newly instantiated object for every request, set this value to FALSE.
    + *
    + * This is a registry-style function.  It should normally be placed in the
    

12. +      'description' => 'This slot does nothing.  It\'s just to test lookup behavior of reusable slots.',
    

13. + * This hook should return a doubly-nested array of handler definitions.  The
    + * first key is an existing slot.  Its value is an associative array of handler
    + * machine-readable names and handler definitions.
    

14. + *   The PHP class that defines this handler.  The class must implement the
    

15. + * This is a registry-style function.  It should normally be placed in the
    

16. +         'description' => "This handler doesn't do anything to the string.  It passes through unaltered.",
    

17. +        'description' => 'The PHP class for the handler attached to this slot/target.  It is stored in this table to make lookups faster.',
    

18. +        'description' => 'Whether or not this handler may be reused.  It is stored in this table to make lookups faster.',
    

19. +        'description' => 'The PHP class for the handler attached to this slot/target.  It is stored in this table to make lookups faster.',
    

20. +        'description' => 'Whether or not this handler may be reused.  It is stored in this table to make lookups faster.',
    

21. +    'description' => 'Aliases are looked up one at a time as they are requested.  Good for sites with a very large number of path aliases.',
    

22. +    'description' => 'All aliases are loaded from the database at once and then looked up in memory.  Good for sites with a much smaller number of path aliases than pages.',
    

I don't like the term "slot" since this is really just the name of an interface plus some meta-data. It's also a rather uninformative name. I'd prefer "subsystem" or even just "interface".

chx says that we have a separate "slot ID" so we can have shorter names like "cache" - I'm not convinced that this is better than just using the name of the interface itself. e.g.

function system_interface_info() 
  $interfaces['PathAliasInterface'] = array(
    'title' => 'Path aliases',
    'description' => 'Lookup engine for path aliases',
    'default_handler' => 'lazy',
  );
  return $interfaces;
}

function drupal_clear_path_cache() {
  handler('PathAliasInterface')->clearCache();
}

I guess I can see that one class may implement multiple interfaces, so there may not be a 1-to-1 mapping between a handler ID and class name, as distinct from what it seems the case is for "slots".

More comment fixes in the defgroup section thanks to keithsmith and merlinofchaos.

Posted a bunch of other comment and string changes to #drupal-dev, for chx to pick up when he has a chance.

Heaps of comment fixes by keithsmith and pwolanin and removed finally all the badness in spaces and tabs. Grrrr.

Reviewers: please start with handlers.inc , that's where the @defgroup is so it's likely anyone trying to understand will start there. It's a lot harder otherwise.

Reviewing some of this in more detail - handler_attach() and handler_slot_rebuild() do not seem to check that the desired handler or default handler implements the required interface. It might also be useful to actually require that all the interfaces extend HandlerInterface.

Without these sorts of checks, it seems to me that we are throwing away one of the more useful feature of doing this OO style.

Nit-pick: we define the _info() hooks to return arrays for each slot or handler, but then load them out of the DB as objects and use them as such. Can we be consistent?

Also, there is an inconsistency in the DB schema vs. the variable naming - the table has 'slot' while in code we call it $slot_id. I think these should match. Same for $handler_id. Or better yet, jsut use the interface name and class name and do away with these ID mappings.

Suggested (not fully tested) code for interface checking below. I think we first need to re-order the calls here:

/**
 * Rebuild the handler and slot registries.
 */
function handlers_rebuild() {
  require_once DRUPAL_ROOT . '/includes/handlers.inc';
  handler_handler_rebuild();
  handler_slot_rebuild();
  handler_ensure_defaults();
}

Then we should not accept a handler class that does not implement the correct interfaces. Note also the change to the foreach ($slot_id).

function handler_slot_rebuild() {
  $slots = module_invoke_all('slot_info');

  // Set default values, to avoid NULL issues if nothing else.
  foreach ($slots as $slot => $info) {
    $slots[$slot] += handler_slot_defaults();
  }

  // Let other modules alter the handler target registry if needed.
  drupal_alter('slot_info', $slots);

  if ($slots && drupal_function_exists('handler_load')) {
    // Build the target data.
    $insert = db_insert('handler_slot_info')->fields(array('slot', 'title', 'interface', 'reuse', 'default_handler', 'description'));

    foreach ($slots as $slot_id => $info) {
      $handler = handler_load($slot_id, $info['default_handler']);
      if (isset($handler->class)) {
        $interfaces = class_implements($handler->class);
      }
      // Make sure the handler implements the expected interfaces.
      if ($handler && $slot && isset($interfaces['HandlerInterface']) && isset($interfaces[$info['interface']])) {
        $insert->values(array(
         'slot' => $slot_id,
         'title' => $info['title'],
         'interface' => $info['interface'],
         'reuse' => (int)$info['reuse'],
         'default_handler' => $info['default_handler'],
         'description' => $info['description'],
        ));
      }
      else {
        watchdog('error', 'Invalid default handler @handler_id for @slot_id in function handler_slot_rebuild()', array('@handler_id'=> $info['default_handler'], '@slot_id' => $slot_id));
      }
    }

    // Don't do the deletion until after we've gotten the new data prepared.
    // That reduces the potential race condition window.
    db_delete('handler_slot_info')->execute();
    $insert->execute();
  }

  return $slots;
}

Note - this function seems to have had some flow-control bugs also:

function handler_attach($slot_id, $handler_id, $target = 'default') {

  // Lazy-load the utility function.
  if (drupal_function_exists('handler_load')) {

    $handler = handler_load($slot_id, $handler_id);
    $slot = slot_load($slot_id);
    if (isset($handler->class)) {
      $interfaces = class_implements($handler->class);
    }
    // Make sure the handler implements the expected interfaces.
    if ($handler && $slot && isset($interfaces['HandlerInterface']) && isset($interfaces[$slot->interface])) {
      // We store the class name in the database in addition to the handler ID
      // so that we can immediately instantiate the class upon lookup.
      db_merge('handler_attachments')
       ->key(array(
         'slot' => $slot_id,
         'target' => $target,
       ))
       ->fields(array(
         'handler' => $handler->handler,
         'class' => $handler->class,
         'reuse' => (int)$slot->reuse,
       ))
       ->execute();
  
      // If we've associated something other than the default target, disable the
      // extra variable system caching of handler attachments.
      if ($target != 'default') {
        variable_del('handler_default_' . $slot_id);
      }
    }
    else {
      watchdog('error', 'Failed to attach handler @handler_id to @slot_id in function handler_attach()', array('@handler_id'=> $handler_id, '@slot_id' => $slot_id));
    }
  
    // Flush the target cache.
    handler(NULL, NULL, TRUE);
  }
}

These have come a long way in a short time. Yay. Warning: this review was done at 3am for me. Not my most awake time.

+/**
+ * Get the handler object for the handler associated to this slot/target.
+ *
+ * @param $slot_id
+ *   The slot for which we want to look up a handler.
+ * @param $target
+ *   The target for which we want to look up a handler.
+ * @return
+ *   A loaded handler object.
+ */
+function handler_get_attached_handler($slot_id, $target = 'default') {

The comment on this could be confusing to some. I imagine someone will ask what's the difference for a handler object like $handler = handler('cache', 'filter'); and a handler object listed here? Can we make this comment more clear.

In the test function testDefaultHandler() we have

+    if ($handler instanceof HandlerInterface) {
+      $handler->setString($this->sampleString);
+      $mangled = $handler->getString();
+
+      $this->assertEqual($this->sampleString, $mangled, t('Default handler returned correct string.'));
+    }

If $handler is not and instance of HandlerInterface doesn't that mean our test failed? Shouldn't this be a test itself in here? The same things happens in most of the methods on HandlersNotReusedTestCase. Not sure if we need to check this every time.

I was only able to make it halfway thought the patch. At this point it looks good. I'll look it over more later.

subscribe

crell, i'd be into updating the docs with better examples once they're in core but until then real code that can be examined is better than code that might be added later. we can update the example once something better is in core.

i'm also wondering why the slot's default is optional. it seems like every slot need to have a default handler. if that's not the case how is a missing default handled?

hook_slot_info() needs to document that the default handler must be defined or else bad things may happen. meaning that the module implementin hook_slot_info() should implement hook_handler_info() and define a default handler.

re-rolled with some @see tags in the docs and fixed the conflict with system_update_7019().

whoops forgot the new files.

Rerolled against HEAD. Modified the hook_slot_info comment on default_handler.

I just wanted to weigh in and say that I'm very happy with the current documentation and API exposed to callers. I won't really speak to the internals.

handler_get_attached_handler() needs to call drupal_function_exists('handler_load') the same way that handler_attach() does:

/**
 * Get the handler object for the handler associated to this slot/target.
 *
 * @param $slot_id
 *   The slot for which we want to look up a handler.
 * @param $target
 *   The target for which we want to look up a handler.
 * @return
 *   A loaded handler object.
 */
function handler_get_attached_handler($slot_id, $target = 'default') {
  // Lazy-load the utility function.
  if (drupal_function_exists('handler_load')) {
    $handler_id = db_query_range("SELECT handler FROM {handler_attachments} WHERE slot = :slot_1 AND target = :target
        UNION SELECT handler FROM {handler_attachments} WHERE slot = :slot_2 AND target = 'default'", array(
        ':slot_1' => $slot_id,
        ':slot_2' => $slot_id,
        ':target' => $target,
      ), 0, 1)->fetchField();

    return handler_load($slot_id, $handler_id);
  }
}

Also we need to rename all the @params from $slot_id to $slot as pwolanin suggested in #78.

We can fix minor things like that later, I desperately would like to see in depth reviews and opinion from Dries/webchick on slot/subsystem.

I still do not believe that we need either hook_slot_info(), nor hook_handler_info(). We already have a function, class and interface registry, we don't need two new ones. Simply adding some meta-data to the definitions will easily do the trick. Consider:

/**
 * Fancy string.
 * 
 * Do fancy stuff to strings.
 *
 * @defaultHandler default
 * @reuse FALSE
 */
interface FancystringInterface extends HandlerInterface {

}

and

/**
 * No string mutation.
 *
 * This handler doesn't do anything to the string. It passes through unaltered.
 */
class FancystringDefault extends FancystringBase implements FancystringInterface {
  public function getString() {
    return $this->string;
  }
}

@Crell: I don't believe we need an UI *at all*.

Having an UI means relying on the database for handler routing. We don't need that. The bootstrap process is already less robust because of the registry (don't get me wrong: I like the registry, but nonetheless, it makes Drupal rely on data it needs to fetch from the database). We don't need to make it even less robust by relying on even more database data.

The debate is not if we need to move to a cleaner OOP pattern for pluggable subsystems. Of course we need to.

The key of this debate is on how to implement a robust routing system for those pluggable subsystem. I don't believe that the patch, in its current form, implement such a robust routing system. I suggested in #362747: Create a call routing system (fix broken dynamic includes) to define the routing table in settings.php. I still believe that's the way to go.

I fixed the bug in handler_get_attached_handler() calling handler_load() before including it.

Corrected the documentation on the @params for $slot_id and $handler_id.

I inlined handler_slot_defaults() and handler_handler_defaults() since they were only called once and didn't seem to add much.

Damien Tournoud, I really don't see the win in forcing the metadata into docblock comments. You end up needing some mechanism for querying it and I can't see it being nearly as straight forward and reusing the hook infrastructure we've already got. I also think that we do need the ability to provide a UI for selecting handlers. If someone wants to switch image toolkits it seems silly to force them to edit the settings.php file to do that.

subscribe

Is this still being worked on? If so, where does it stand?

Not to bikeshed, but I'd prefer a term other than "plugins" because in the general software world, "plugins" and Drupal modules basically occupy the same territory. I'd hate to confuse new users and developers. I personally like the term "handlers"; whether it's what Views considers handlers is largely irrelevant to me.

Here are some initial observations/questions I have.

Why isn't the config object stored in the variables table? When you only have the default handler you can't have config options. I know this is for performance and I don't know that I want config objects in the variables table. But, it's a limitation that isn't noted and really shouldn't be there.

For early running systems like cache and path this runs registry_rebuild() and plugins_rebuild(). This seems terribly inefficient. Not yet sure of another way to approach this.

What about configuring plugins in settings.php?

Some nitpicking...

Why is the ['math']['add'] plugn defined for simpletest? It doesn't look to be in use.

In PluginConfigurationBase why does the options() method return array() and not the variable $options? I'm guessing this is an oops

In plugin() why is there the line

  if (FALSE && $record = variable_get('plugin_default_' . $slot_id, array())) {

A hold over from debugging?

Looks really cool. As Crell noted, this would provide an excellent background layer for Field storage engines. I'm even wondering about widgets and formatters, although that might be a little more far-fetched.

How does the patch handle the case when an attached plugin is no longer available ? e.g the module that provides the plugin has been disabled ?
Just asking, because this is one of the nastiest things CCK and Views, for instance, have to handle with their own 'plugins'

Hm, yes, the behavior for "class not found" might depend on use cases. So probably the plugin API just needs to provide convenient DX to let the client code act according to its needs: fall back to default, block functionality in whatever way makes sense, or fail with a bang.

If a module providing a plugin is disabled, the corresponding rows in {plugin_attachments} remain, but those in {plugin_info} disappear on next plugin_rebuild(), right ? So it might be as simple as making sure plugin_load() returns NULL (which seems to be the case already)

Then client code can do:

$plugin = plugin_get_attached_plugin($slot_id, $target);
if (!$plugin) {
  $plugin = plugin_get_attached_plugin($slot_id, 'default');
  // or
  $disable_feature = TRUE;
  // or
  throw new Exception();
}

or maybe the exception should be raised by plugin_get_attached_plugin() and caught (or not) in client code ?

Then there's the case of 'module providing plugin is uninstalled' - should the attachments be cleared then ?

BTW, shouldn't plugin_plugin_rebuild() start by clearing all existing rows, instead of always inserting ?

The variables system is half-initialized during DRUPAL_BOOTSTRAP_CONFIGURATION; it has the variables configured in settings.php. I think it's reasonable to expect users who sub out the cache layer to edit settings.php; we do now. (Or am I missing that this is a complex value?)

Another option is to specifically write out a file storing the cache plugin configuration. It would minimize the special case-ishness.

@Crell - having to set the cache in settings.php (At least to utilize the aggressive cache) seems totally fine.

Note that DRUPAL_BOOTSTRAP_EARLY_PAGE_CACHE has nothing to do with aggressive caching, but with (as the name implies) early caching. This one is pre-database, so obviously pre-handlers.

*But*, we know that there are links between the database system, the registry and the variable system, that *will* prevent the plugins approach from working for the cache subsystem. For an example of such a cross-dependency, see #344088: cache.inc cannot be fully converted to dbtng.

@Crell - forward, please!

Regarding recent discussions on the caching layer. I agree that it is perfectly acceptable to special case this, especially for the early page cache, but potentially for all of it if that is how it needs to be. Most of the high value cache swaps are early page. I can't think of any cache backend swaps users might want to do that adding a line to settings.php would be any kind of blocker.

+  /**
+   * Creates the configuration form for this configuration object.
+   *
+   * Unlike a normal form function, you should not return $form from
+   * this method.  Simply alter $form as-is.
+   *
+   * @param $form
+   *   The form object to which to add our fields.
+   * @param $form_state
+   *   The state for this form.
+   */
+  public function optionsForm(&$form, &$form_state);

The fact that this is a form alter is a pretty odd pattern, compared to (for example) block forms. What about the case of a plugin being used in to different contexts (targets?), such as imagemagik being used with imagecache but also with a JS imagefield editing/cropping widget? Module X wants the plugin forms nicely wrapped in a collapsible, but module Y doesn't - or one wants the settings above their other form elements, and one below? Either the plugin needs to make some risky assumptions about what it should be doing, or else the plugin consumer module needs to do a bunch of hacking to fix up the form elements afterwards (also seems error prone).

I would suggest that this be a normal "return $form" function. We could still offer a separate altering step, however I think that could still have trouble - if we are serious about plugins being abstract enough for different use cases they should define a proper interface for the kind of effects they would like to have (e.g. method supress() { return array('suppress_image_manual_size'); } - could be a standard interface or perhaps just leave it up to plugins to extend as needed.

+ * @param $target
+ *   The target inside the subsytem the plugin belongs to.

This description is obtuse to say the least :)
The code could really use some examples to help people reading the code frame the terminology a bit before they get into abstract definitions.

+  $slots = module_invoke_all('slot_info');

We have just moved install profiles from hook_*_info to .info files (the last remaining instance). I think it is worth considering if we should use hook_*_info or .info files here. Obviously .info files adds complexity of one sort, but the standardization reduces complexity of another sort (pattern sprawl). Would plugins ever need dependencies (e.g. on a module that provides a "back-back-end" API to both a specific plugin, but also more generally to diverse non-plugin users).
I am undecided here.

I talked to merlin about the reason for the signature optionsForm is based on and the reasoning isn't actually surprising at all. The return $form style ends up giving you either an nasty mess of slow and tricky array_merge's or a #tree mess. Its somewhat of a limitation of the FAPI that I've also run into and its not one that we're going to solve in FAPI in 7 I don't think.

A side note, the reason Views doesn't have any issues with this setup is it uses smaller more contextual forms and Javascript magic. The ramifications of that may be something we should consider(may be a bikeshed to the overall issue here?), though I think the model is fairly tried and true in Views.

No such thing as a critical task.

Subscribing.

Some notes:

In the first patch attached, you are still copying superglobals (such as $_GET, $_POST and others), you really should use them directly (either using a reference, either just manipulating it).

In the patch provided in #106, you define a plugin configuration interface. I think this is basically an error. you should:

1. Ask the configuration initiative to build a mapping layer, reusable for everything.

2. Creating a basic "options" interface, without UI, using only get() method (and maybe implementing Iterator interface could be a plus), this is the configuration initiative Work.

3. Extend this options interface as "UX providing options" for forms, that would remain optional.

4. Use the first basic options interface everywhere in code, use if ($options instanceof MySuperConfigurableInterface) {} to determine weither or not you should display a form in all UX.

The idea behind using a mapping layer (for which the basic options implementation would be the only thing you have to worry about) would be to make everything exportable as a hierarchical tree of scalar values.

I did this kind of implementation there which is basically a derivative fork of Zend_Config.

EDIT: By the way, I like neclimdul's plugin code in its sandbox.

sub

Marking as duplicate of #1497366: Introduce Plugin System to Core.