Create a call routing system (fix broken dynamic includes)

subscribe

So you're reimplementing handlers in a crappier way, without using objects for it, when this sort of behavior the "killer app" for OOP. I do not see what you're trying to accomplish here that cannot be done better using native PHP syntax. 90% of what is needed is already written in the handler module in contrib.

subscribing.

I wasn't able to quite grok this from the description in the initial post, so here's a summary of chx and my talk in #drupal:

What chx proposes here is a unified way to handle any type of "there ought to be one thing that's responsible for this piece of functionality here." For example, which algorithm we're going to use to hash passwords, which SMTP library we're going to use to send e-mails, and which storage mechanism we're going to use with a given field. The initial approach done in the Fields API...

module_invoke(variable_get('field_storage_module', 'field_sql_storage'), $op, $arg1, $arg2);

...is not ideal, because then *all* fields will only use *one* storage mechanism. So the system he proposes makes it possible for any module to claim ownership on a per-password/mail/field basis... lowest weight wins.

So you might have two modules, amazon.module and sql.module, each of which implement hook_field_storage():

amazon.module:

function amazon_field_storage($field) {
  if ($field->type == 'asin') {
    return TRUE; // I want to intercept this one, because I want to retrieve things from a web service, rather than SQL.
  }
}

sql.module:

function sql_field_storage($field) {
  return TRUE; // I can handle any fields, so I'm always true.
}

Assuming both modules have a default weight of zero, because 'a' in amazon module comes before 's' in sql module, using the suggested implementation of module_get_module() in chx's initial post, here's what would happen with a 'asin' field:

  $field = make_a_field('asin');
  // $module ends up being 'amazon' rather than 'sql' because of the field type.
  $module = module_get_module('field_storage', $field);
  // Call amazon_insert($field);
  module_invoke($module, 'insert', $field);

And here's what would happen with some other kind of field:

  $field = make_a_field('text');
  // $module ends up being 'sql' because no other modules answered the call.
  $module = module_get_module('field_storage', $field);
  // Call sql_insert($field);
  module_invoke($module, 'insert', $field);

The pros of this approach include:
a) Module developers already understand how to implement hooks.
b) Weights are a common construct in Drupal, so explaining this to people is fairly easy to understand.
c) It's only a few lines of code to implement.
d) It leverages existing Drupal knowledge/constructs.

The cons of this approach include:
a) These aren't actually hooks; unlike hooks, where multiple modules can have their shot at whatever page event is going on, in this case only one wins. So it's going to be confusing for developers if we call them that (another one of those dreaded Drupal WTF? "most of the time ... except ..." things to explain...)
b) Weights are a fairly brittle means of establishing precedence, so I forsee this either breaking down when more complex interactions are required, and/or causing really hard-to-track-down bugs.
c) The precedence of weights is inverted here compared to what usually happens. Normally, the highest weighted module is the last one to take the shot (ex: form_alter, nodeapi, etc.) but in this case it's the lowest weighted module. The only other hook I can think of that behaves like that is hook_file_download() and ugh...

Personally, though, as much as I love you and all Larry, I do agree with chx that putting off further improvements to various subsystems waiting for handlers in core to materialize is not something I'm too keen on continuing to do, esp. given how long the DBTNG todo list still is. Whether this or another similar system is a less optimal solution than the handlers panacea we could debate for many days, but the sooner we get all of these systems behaving consistently in one way or another, the sooner we pave the road for the more advanced stuff you want to do.

As to whether or chx's proposal is the "right" solution, though, I'm not sure. Needs some more discussion.

I'm really against delegating routing decisions to modules. The decision to use a given field storage for a given field should be the call of the website builder. This site-wide decision would be best placed in the settings.php file. I suggest we implement a mixed approach like this:

Static routing table, using an array:

$routes['field_storage'] = array(
  'my_special_field' => 'field_couchdb_storage',
  '#default' => 'field_sql_storage'
);

Dynamic routing table, using a function callback:

$routes['field_storage'] = 'my_field_storage_routing'; // This is a callback function.

function my_field_storage_routing($field) {
  if ($field->type == 'my_special_field') {
    return 'field_couchdb_storage';
  }
  else {
    return 'field_sql_storage';
  }
}

@Damien Tournoud delegating the site config to the settings.php file will be a usability problem for a lot of users. It makes configuring the site more difficult.... like back in the pre-installer days. This might work fine for the top 10% of devs but it's much more difficult for the bottom 90%.

I'd like to see some more examples on the expected use/config. Something that shows configuration and usage in cases without module_invoke. In ways that don't require hooks but work for callbacks and classes/objects.

@mfer - anyone using a non-standard sub-system is probably in the top 10% I'd imagine...

That being said, I don't really like the idea of having this fixed in code in settings.php, since a module that ships with a replacement sub-system needs to be able to set up the appropriate routing on hook_install or hook_enable. Perhaps just another table like the variables table but which is not cached and simply fetched into memory if needed?

In any case, this issue of how to resolve the situation of more than one module wanting to have its system used tricky.

That being said, I don't really like the idea of having this fixed in code in settings.php, since a module that ships with a replacement sub-system needs to be able to set up the appropriate routing on hook_install or hook_enable. Perhaps just another table like the variables table but which is not cached and simply fetched into memory if needed?

In any case, this issue of how to resolve the situation of more than one module wanting to have its system used tricky.

It's not because I enable the memcache module that I suddently want all the cache to be delegated to memcache. That's exactly my point above: we shouldn't delegate that decision to modules.

Plus, there is generally a tricky path to follow to switch from an implementation to the other. For example, take chx' example of the field storage: there is probably no full-proof, fully automated path between one type of storage and another. The switch needs to be a conscious, reasonably studied decision. The best place for this setting is thus without any doubt in settings.php.

Why couldn't the decision on what to use be set by a module? Sure, for most things I don't think a module being enabled is the way to choose. But, that's just one way it could be done. There are other ways.

For example, take the smtp patch. Could a selector on an administration page be used to select which system is used? Say, something like what the messaging and notifications modules do for selecting which of the pluggable systems is used? This is handled through a module with a UI but not when a module is enabled.

@webchick, I totally get the "better something now than just a suggestion for later" argument. Patch with docs and unit tests for handlers is here: #363787: Plugins: Swappable subsystems for core. :-)

Honestly, I don't think this is the right approach at all, mostly for the Cons that webchick lists. The Pros listed all boil down to "it looks like the hammers we already have". That would be fine if we are dealing with nails, but we're not. Single-implementation plugins are a vastly different animal from many-to-many visitor or observer behavior, which is what hooks are. Hooks simply do not work for this problem space; that's not what they are good at. We don't really have anything in Drupal right now that does a good job of doing this sort of task, so any proper solution is going to look "un-Drupalish" at first. That's why we need a single, standardized solution so that what "Drupalish" means expands to include a solution to a new problemspace.

Further commentary in the other thread.

I'm wondering if this way of solving this problem is another drupalism or if it's a commonly used pattern. In introducing something new it would be good to keep the developer experience as best as possible.

I really like chx's new approach.

I wonder whether some drupal_function_exists() magic could eliminate those hard-coded alternative-subsystem-arbitration-invokers as well?

+  if ($function = variable_function('user', 'hash_password', $password, $count_log2)) {
+    return $function($password, $count_log2);
+  }

If not - after all, I think we're going to allow to replace subsystems with this method (be it core or contrib), so I would prefer a function with a "drupal_" prefix, e.g. "drupal_function" or even better "drupal_subsystem" (and likewise use $conf['subsystem']). That's minor though.

I like this. The settings.php syntax is very simple for swapping out one system entirely for another. While the example for multiple targets is a little more complex - it allows it to happen with just a few lines of code - and that's an advanced use case anyway, one which can be accompanied by additional documentation in the handbook. So we get an unbroken registry, multiple targets, and an easy to use syntax in a very small patch.

I have similar niggles to sun about the function naming, but that shouldn't stop this getting some more reviews.

I can't confirm the testing bot results. Patched HEAD with #22 and it installed and ran tests just fine. :/

This is a very interesting approach, and on initial review I prefer it over the proposed OO handlers implementation. Easy to use, easy to adjust in settings.php, easy to document. Definitely would love to have drupal_http_request() using this as well! :)

Does testing slave #4 have some known problems?

Summary of irc discussion which went into the above patch:

I consider this closer to drupal_function_exists() than variable_get(), so drupal_function_get() was chx's idea. I originally liked sun's drupal_subsystem, but this allows us to make just drupal_lookup_path() pluggable as opposed to the whole of path.inc, so it wouldn't be accurate.

The call_user_func_array() here is only used if you use the dynamic key in $conf['functions'] - which chx reckoned will be almost never - and if you know which tables are which, wouldn't even be required for different cache backends per table.

Hardly any existing code changes in the patch - only the session.inc stuff moving out of bootstrap, which is a straight cut and paste. Otherwise it's just find and replace for the existing pluggable systems.

Once again, it's very easy to grok, it's very few lines of code, and it gives us plenty of flexibility.

My initial critiques of this system:

1) It ends up loading the default code even when some plugin or handler is being used.
2) As a result, the default handler doesn't provide a guide to writing new handlers
3) There is very little visibility into the plugins. Now, because many of these MUST be set in settings.php as they will be needed before there is a database, that is perhaps not such a bad thing.

Now, doing it this way makes it a fairly light system, which is fine, but unfortunately I think that means it will forever be limited to only doing these tasks and does not create a generic system.

If a module want's to define it's own system, for example a different way to do password hashing, and provide an interface to choose this different plugin how would it go about making an interface that lets a user switch to it?

In this patch their hook_submit patch would have to load the functions array with variable_get, update it, the send it back with variable_set. This can/will lead to modules touching the config of other modules plugins.

@mfer, either we can ask module users to add something to settings.php - as happens currently. Or for those cases we can consider using something heavier than this - like the handlers patch.

However - this allows us to route stuff without hitting the database - which is ideal for caching and sessions - and we're never going to have a solution to that which doesn't involve settings.php or something similar if we want to get to the point of being able to serve cached page views without any database access.

So to me, considering our dynamic includes are completely broken in HEAD at the moment, and this adds a tiny amount of code in order to fix that. I think it's pretty much ready to go in - and then for things which might need a more flexible system, with configuration or whatever, it'll be easy to convert those subsystems to handlers later on a case by case basis. Additionally, I'm pretty sure the handlers patch doesn't allow for multiple caching back-ends in its current state - which we get here very nicely.

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.

What? Since when have handlers as I've proposed them not supported multiple cache backends? Every version ever posted to d.o or CVS has supported it. Where's this FUD coming from?