Impossible to extend the FileTransfer class system in contrib

+++ includes/authorize.inc	27 Oct 2009 23:30:41 -0000
@@ -116,6 +115,62 @@ function authorize_filetransfer_form($fo
+  // Invoke the callback to get the settings form.
+  $form = call_user_func($info['settings_form']);

I have yet to see a use-case where cuf() actually makes sense. Instead of cuf(), we can do the following here (like everywhere else in core):

$form = array();
$function = $info['settings_form'];
if (function_exists($function)) {
  $form = $function($form, $form_state);
}

+++ includes/authorize.inc	27 Oct 2009 23:30:41 -0000
@@ -116,6 +115,62 @@ function authorize_filetransfer_form($fo
+function _authorize_filetransfer_connection_settings_set_defaults(&$element, $key, array $defaults) {
+  // If we're operating on a form element which isn't a fieldset, and we have
+  // a default setting saved, stash it in #default_value.
+  if (!empty($key) && isset($defaults[$key]) && isset($element['#type']) && $element['#type'] != 'fieldset') {
+    $element['#default_value'] = $defaults[$key];
+  }
+  // Now, we walk through all the child elements, and recursively invoke
+  // ourself on each one. Since the $defaults settings array can be nested
+  // (because of #tree, any values inside fieldsets will be nested), if
+  // there's a subarray of settings for the form key we're currently
+  // processing, pass in that subarray to the recursive call. Otherwise, just
+  // pass on the whole $defaults array.
+  foreach (element_children($element) as $child_key) {
+    _authorize_filetransfer_connection_settings_set_defaults($element[$child_key], $child_key, ((isset($defaults[$key]) && is_array($defaults[$key])) ? $defaults[$key] : $defaults));
+  }
+}

Hmmmm... sorry, I forgot the reason - why do we fork form.inc, resp. form_builder() again?

This review is powered by Dreditor.

So dww clarified that this function is basically what http://api.drupal.org/api/function/_system_settings_form_automatic_defau... is doing, however, not 1:1 using system variables, but rather expanding a serialized array (in a system variable) into #default_values in a form.

I don't like the special-casing, but since we're past API freeze and this is non-critical, I guess we need to live with a custom implementation in authorize.inc. :-/

The UI guideline is to not wrap the main interaction of a form into a fieldset, so very good fix there. :)

OK so _authorize_filetransfer_connection_settings_set_defaults() made me wince a bit, but fully agreed that it's self-contained and we don't have a lot of options at this point. Also +1 for less random crap in system.module. Looks like sun's review is fixed / no longer applies. Comments are verbose. RTBC.

My reason for not wanting to put Drupal form definitions into a class is that classes should do something, not everything. Just because you can add it, doesn't mean you should. The FileTransfer classes have nothing to do with a form to configure them. I don't mind the approach in the patch, but I think there are other things.

+++ modules/system/system.api.php	30 Oct 2009 01:01:31 -0000
@@ -2665,5 +2665,72 @@ function hook_url_outbound_alter(&$path,
+function hook_filestransfer_info() {
+  $info['sftp'] = array(
+    'title' => t('SFTP (Secure FTP)'),
+    'class' => 'FileTransferSFTP',
+    'settings form' => 'sftp_filetransfer_settings_form',
+    'settings form file' => 'sftp.filetransfer_settings.inc',
+    'weight' => 10,
+  );
+  return $info;
+}

So the "kitten killing" is the extra delcarations for the settings form. I personally don't think this matters very much at all since these things are almost never declared, and now an _alter could change it without changing the class.

Anyway, what about something more like this:

Why so verbose? False stability / security

+++ modules/system/system.api.php	30 Oct 2009 01:01:31 -0000
@@ -2665,5 +2665,72 @@ function hook_url_outbound_alter(&$path,
+function hook_filestransfer_info() {
+  $info['sftp'] = array(
+    'title' => t('SFTP (Secure FTP)'),
+    'factory' => array('system_get_ftp_filetransfer', 'sftp'),
+    'settings form' => 'sftp_filetransfer_settings_form',
+    'file' => 'sftp.filetransfer_settings.inc',
+    'weight' => 10,
+  );
+  return $info;
+}

So authorize.php would load the one file in 'file' (this is the same as hook_theme and hook_menu. That file would contain a function to return a FileTransfer class when the 'factory' is called. factory would pass in the $settings array which currently goes to the factories in the classes themselves.

This lets the module implement the instantiation however it needs to, and keeps the settings form in the module where it belongs.

Should I roll this?

This review is powered by Dreditor.

Not critical. Please read and understand Priority levels of Issues, thanks.

I agree with everybody that setting the form defaults in authorize.inc instead of extending Form API is most unfortunate but also the only reasonable choice at this point in the cycle.

I have to admit I am not a huge fan of this much indirection in the class instantiation. I would rather see the settings form live with the class (as Derek suggested a year ago) than have a .inc that defines a settings form and a factory function which in turn includes its own .inc and instantiates its own class. We're building this stuff for Drupal, not a final exam in OOP, and it's going to be a confusing API to implement. But that is a matter of opinion only; since there's disagreement on it I am happy to defer. Even if this API is confusing, it's workable, which is a lot better than the current state of HEAD. So my vote is to go ahead with it.

Marking CNW for the following nitpicks:

+++ modules/simpletest/tests/system_test.module	7 Oct 2010 18:57:50 -0000
@@ -311,3 +317,56 @@ function _system_test_second_shutdown_fu
+ * Implements hook_filestransfer_info().

s/filestransfer/filetransfer

+++ modules/system/system.filetransfer.inc	7 Oct 2010 18:57:50 -0000
@@ -0,0 +1,81 @@
+ * Returns the form to configure the FileTransfer class for FTP

This docblock and the next two need periods at the end of their sentences.

Powered by Dreditor.

Seems fine to me.

How is the settings form now part of the class?

system_filetransfer_settings_form_ftp() seems to be outside of the class, but doesn't matter to me either way.

Honestly, if we shipped it to not be extendable, I wouldn't really care because I see very few cases of people using SMB to install mods. Still... looks like it's more verbosely documented and clearer.

This is a pretty enormous patch, and I confess I don't quite understand what all is happening here. And it changes APIs.

However, the APIs that it's touching aren't affecting any contributed modules at the moment, since this patch allows there to even be contributed modules that extend the FileTransfer methods.

So I think I'm okay with this going in, under the auspices of "blatant API oversight in new Drupal 7 feature". But I'll leave RTBC for 24 hours in case Dries believes otherwise.

Ok, dww was kind enough to spend a half hour or so going through this patch with me.

Firmly wearing my D7 core maintainer hat, the following is true:

- There's nothing about fixing this bug that requires hook_filetransfer_backends() to hook_filetransfer_info(). This should move to D8.
- There's nothing about fixing this bug that requires renaming $_SESSION['authorize_filetransfer_backends'] to $_SESSION['authorize_filetransfer_info']. This should move to D8.
- There's nothing about fixing this bug that requires moving system_filetransfer_backend_form_ssh() et al from system.module to system.filetransfer.inc. This should move to D8.

Then if I analyze the actual changes that are part of the patch, this whole 'factory' thing, while it might make sense to folks with a strong OO background, creates confusion and inconsistency with the rest of Drupal.

Everywhere else that we use an _info()-style hook (hook_theme(), hook_menu() et al), the 'file' index means "this is where to find the code for this thing." Here, it means "this is where to find a dummy wrapper function that includes the code that you actually want to call." Confusing as heck.

It seems to me like we could solve this problem very easily, and with a much smaller patch, by doing the following:

1. Move the settings form methods to the actual class definition. I read Jacob Singh's #12, and I think that would be just peachy if we were rolling this patch a year and a half ago and had ample time to tweak things, but we are rolling it now. For D7, I see no reason why we can't just couple these things together. Especially since, as dww points out, people don't have to call the settingsForm method, and since, as Jacob points out, most people won't override these from contrib anyway. In D8, if Drupal ends up going a lot more OO in its design, it probably totally makes sense to revisit this in context of what the entire system is doing, but in D7 a one-off here is just a total WTF for developers.

2. That would involve getting rid of the 'factory' and 'settings form' indexes in hook_filetransfer_backends() in favour of just 'class' and 'file'. Class is the name of the class, and 'file' is the file to include to get that class definition. Same as hook_entity_info(), same as other places in core.

3. Tests + documentation.

And that's it. That seems like it accomplishes the simplest thing that could possibly work to solve this semi-critical bug and leave room for contrib to improve about core's offerings here.

Looks like webchick's directions entirely stopped work on this issue. Can we find a compromise? FWIW, I agree with dww that API changes in this subsystem aren't really API changes - I'd recommend to respect do-ocracy here. dww is more or less the only one who actually works on all of these update manager issues. (Thanks for that!)

It seems to me that this is the one of the most critical bugs for the lifecycle of D7. I'll see if I can step into this code tonight to see if I can make a meaningful contribution.

I'm not owning "webchick's directions entirely stopped work on this issue". I spent a great deal of effort explaining why the patch in #26 is uncommittable (dww agreed), and laid out very explicit instructions on how to make it so. That no one stepped up to work on this is regrettable, but certainly not my doing.

There's still time though, but not much. Tick, tick, tick.

I'll see if I can do it this afternoon.

Oh wow.
Please forgive the intrusion, but I could not help but step out of my normal lurker role
I learned more about open source in this one thread than any conference I ever went to.
Do you really sacrifice so much just to meet an imaginary deadline? The main motivator here is passion and these rules seem more of a reflection of the corporate. Code may be fundamentally math, but here I see the ART. Do we ignore the best design because of misinterpretation of true object (or even aspect) orientation?
I am sorry for the rant - I respect your hard work and am not really part of the community, I would like to see the best D7. I do not want to wait years for such an important feature.

Status update: I have a patch for this halfway done, but it's Thanksgiving and I may not get back to it for a day or two.

OpenDomain, I can't tell from your comment which design you think is best, but there's disagreement on that among the participants in the thread. I don't think it's due to anyone's misunderstanding of software design principles. We just disagree on our priorities and audience. When that happens, we look to a committer to provide feedback. We got our feedback, and now we're acting on it. It seems to me like good teamwork, not corporate dysfunction.

And yeah, we have a deadline. If we didn't, we'd never release. I don't want to go down in history next to Duke Nukem Forever. :)

anxiously awaiting ksenzee's take on this patch.

I'm setting some time aside tonight to be able to test this issue if a patch becomes ready tonight.

Here it is. Sorry that took so long. I went ahead and renamed hook_filetransfer_backends() to hook_filetransfer_info(), partly because I was starting from the existing patch and that was easier than changing it back, but also partly because if there did exist an implementation of hook_filetransfer_backends(), the rename would be an obvious hint that it has to be rewritten. I did ask webchick first to make sure she wouldn't literally skewer me over it.

woohoo! Now pardon me for asking, how should i test this?

Okay, so this code already went through multiple iterations and reviews, and the only thing that's holding back dww from RTBC-ing it is the call_user_func_array VS eval question.

I too vote for call_user_func_array(). Easier to understand, looks cleaner to me, + no reason to even explain why I hate eval :) Existing approach > evil trick.

Peeked at the patch, didn't see anything strange, but I'm not really familiar with the update manager.

Let's get this to RC1 :)

In general call_user_func_array() is faster than eval()

Straight function call: 0.392249107361 secs
Dynamic function call: 0.671601057053 secs
call_user_func function call: 1.45473217964 secs
call_user_func_array function call: 1.61347579956 secs
eval function call: 3.61229491234 secs

Straight static method call: 0.701809167862 secs
call_user_func static method call: 2.67288184166 secs
call_user_func_array static method call: 2.81385302544 secs
eval static method call: 4.31622505188 secs

Straight method call: 0.700944185257 secs
call_user_func method call: 2.17300820351 secs
call_user_func_array method call: 2.32969498634 secs
eval method call: 4.98666906357 secs

Code test:

class myTest
{
    public static function test()
    {
        return true;
    }

    public function testMe()
    {
        return true;
    }
}

function testMe()
{
    return true;
}

$o = new myTest();

$function = 'testMe';

echo '<br />Straight function call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    testMe();
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />Dynamic function call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    $function();
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />call_user_func function call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    call_user_func($function);
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />call_user_func_array function call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    call_user_func_array($function, null);
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />eval function call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    eval('testMe();');
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br /><br />Straight static method call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    myTest::test();
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />call_user_func static method call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    call_user_func(array('myTest', 'test'));
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />call_user_func_array static method call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    call_user_func_array(array('myTest', 'test'), null);
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />eval static method call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    eval('myTest::test();');
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br /><br />Straight method call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    $o->testMe();
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />call_user_func method call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    call_user_func(array($o, 'testMe'));
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />call_user_func_array method call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    call_user_func_array(array($o, 'testMe'), null);
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

echo '<br />eval method call: ';
$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    eval('$o->testMe();');
}
$end = microtime(true);
$elapsed = $end - $start;
echo $elapsed, ' secs', "\n";

Agreed that the first patch in #50 is the one to commit.

Ok. I think this is a necessary change so we can continue to iterate on the Update Manager in contrib through the D7 lifecycle.

Committed #51a to HEAD. Down with eval(). :P

Randy, this is tagged "API change" because it is, but there's absolutely no reason to announce this. This only affects theoretical contrib modules that couldn't exist before this patch. :P