This is one of four issues to get Plugin Manager in core. The Plugin Manager module in contributions can be found at http://drupal.org/project/plugin_manager. It would be a huge boost to usability if we didn't have to mess around with our file system before we do updates or new module/theme installations. The reason I've separated this into four issues is because otherwise, this will quite quickly turn into even more of a monster patch. I will be updating my patches every Tuesday and Saturday, so getting reviews between each of these regular updates would be awesome :). The four issues I've created are outlined as below:

  1. #395472: Plugin Manager in Core: Part 1 (backend)
  2. #395474: Plugin Manager in Core: Part 2 (integration with update status)
  3. #395478: Plugin Manager in Core: Part 3 (integration with installation system)
  4. #395480: Plugin Manager in Core: Part 4 (installation profiles)

For more details on the other issues, click the above links.

This issue is about getting the backend/API for plugin manager in core. This does not involve adding a module to core, as far as I can tell; I think this would be better handled by adding a new .inc file, perhaps named plugins.inc (or something more creative — couldn't think of anything better). This .inc file would be reusable for both integration with the update status module for "automatic" updates (!!!) and system module/theme installation. The majority of the code will be taken from the plugin manager module, so please credit JoshuaRogers on the commit. I will just be organizing it properly, looking over it for coding style errors, and in general making it core-worthy.

CommentFileSizeAuthor
#266 filetransfer.patch2.64 KBcatch
#262 filetransfer-262.patch2.86 KBjrchamp
#252 395472_add_setup.patch986 bytesdeekayen
#241 filetransfer.patch2.64 KBcatch
#232 filetransfer_test_remove_debug.patch456 bytesJacobSingh
#229 works_for_me.patch761 bytescatch
#224 395472-fix-test.patch2.52 KBDamien Tournoud
#214 remove_dd_calls.patch750 bytesBerdir
#206 filetransfer_cleanup_206.patch30.57 KBJacobSingh
#205 filetransfer_cleanup_205.patch30.57 KBJacobSingh
#192 filetransfer_cleanup_03.patch27 KBcwgordon7
#188 filetransfer_cleanup_42.patch25.17 KBJacobSingh
#185 filetransfer_cleanup.patch17.93 KBchx
#176 filetransfer_cleanup_5.patch19.33 KBJacobSingh
#174 filetransfer_cleanup_4_0.patch19.64 KBcwgordon7
#171 filetransfer_cleanup_4.patch19.58 KBJacobSingh
#169 filetransfer_cleanup_3.patch19.5 KBJacobSingh
#163 filetransfer_cleanup.patch17.32 KBchx
#162 filetransfer_cleanup.patch3.92 KBchx
#159 filetransfer_cleanup.patch17.51 KBchx
#157 filetransfer_cleanup.patch13.88 KBJacobSingh
#151 extend.patch907 bytesFrando
#145 plugin_manager_part_1.patch12.37 KBchx
#142 plugin_manager_part_1_20.patch22.29 KBcwgordon7
#140 plugin_manager_part_1_19.patch21.66 KBcwgordon7
#139 plugin_manager_part_1.patch20.31 KBchx
#121 plugin_manager_part_1_18.patch21.15 KBcwgordon7
#120 plugin_manager_part_1_17.patch20.36 KBcwgordon7
#118 plugin_manager_part_1_16.patch20.3 KBcwgordon7
#108 pm_base.patch23.68 KBchx
#107 pm_base.patch0 byteschx
#105 pm_base.patch23.07 KBchx
#104 pm_base.patch18.12 KBchx
#98 fake.tar_.gz268 bytesJacobSingh
#97 395472_98.diff38.09 KBJacobSingh
#90 plugin_manager-395472_90.patch13.19 KBJacobSingh
#81 plugin_manager_part_1_15.patch37.79 KBcwgordon7
#79 plugin_manager_part_1_14.patch37.79 KBcwgordon7
#76 plugin_manager_part_1_13.patch37.78 KBcwgordon7
#66 yeah.png32.78 KBtstoeckler
#62 not_quite.png82.79 KBtstoeckler
#61 plugin_manager_part_1_11.patch34 KBcwgordon7
#60 plugin_manager_part_1_10.patch32.94 KBcwgordon7
#59 plugin_manager_part_1_09.patch33.18 KBcwgordon7
#51 plugin_manager_part_1_08.patch33.4 KBcwgordon7
#45 395472-45.plugin_manager_part_1.patch34.57 KBdww
#44 395472-44.plugin_manager_part_1.patch34.6 KBdww
#43 plugin_manager_part_1_05.patch34.95 KBscor
#38 plugin_manager_part_1_04.patch34.97 KBcwgordon7
#37 plugin_manager_part_1_03.patch33.48 KBcwgordon7
#8 plugin_manager_part_1_02.patch29.73 KBcwgordon7
#1 plugin_manager_part_1_01.patch8.26 KBcwgordon7
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cwgordon7’s picture

Status: Active » Needs work
FileSize
8.26 KB

Quick first patch, nowhere near ready. Summary of work left to be done:

- Database-based caching system has yet to be implemented.
- The various backends have yet to be implemented.
- The backend location/inclusion mechanisms are yet to be worked out.
- Tests need to be written.

What I've done:

- A quick copy-and-paste from plugin_manager.module to plugins.inc with function name changes and code/comment cleanup.
- Removed the PEAR package usage - I'm not sure what the legal implications of that would be, so I don't want it in core. It should still be possible for contributed modules to allow its use.
- Renamed a constant provided by the update module.
- Created a base hook implementation by system.module.
- Changed the location of saving files - instead of to the file directory, to the temporary directory.

So I'm probably about 3% of the way there ;). But, suggestions/feedback is always welcome. :)

joshmiller’s picture

Looked at the code, I like where it's headed. I also like the idea of putting this functionality in core. Can we get it done in time is the real question.

Perhaps a better file name: contrib-automatic.inc

josh

cwgordon7’s picture

Yeah, the file name is kind of pathetic, I couldn't think of anything better, though. I don't really like contrib-automatic.inc either, since the only other .inc file with dashes is cache-install.inc, which is used as a special-case cache.inc.

joshmiller’s picture

I personally like the word "plugin" -- but that's just not in the Drupal lingo. "Contrib" seems to be the only Drupal-specific word that can mean modules and themes.

Just my two cents... oh, and I agree about the hyphenation. I just wanted to make sure the file name was at least somewhat informative.

cwgordon7’s picture

contributions.inc is another possibility. Since the original module was called "plugin manager," I thought that either plugins.inc or manager.inc would be the easiest to start with until we can get some general consensus. I went with the former option (plugins.inc) but I'm happy to change if we get general consensus for another name.

catch’s picture

We don't have a good word to describe both modules and themes. I personally like 'extensions'. manager.inc also nicely sidesteps the issue :)

cwgordon7’s picture

I like extensions.inc too, I think I'll use that for my next reroll.

cwgordon7’s picture

New patch, some big stuff left to do:

- Database-based caching system has yet to be implemented.
- Tests need to be written.

Stuff that I've done:

- The various backends have been implemented (though not tested).
- The backend location/inclusion mechanisms have been worked out - some stuff has been moved to bootstrap.inc so that the main .inc file does not need to be included on every page request.
- The file has been renamed to extensions.inc, and the new files for the backends are named extensions.ftp.inc and extensions.ssh.inc.
- I've documented the hook in system.api.php.

So, still not ready, so needs work, but at this point reviews would be highly appreciated so I can find out if there's anything wrong with the fundamental structure before I build more on top of it. :)

catch’s picture

Is common.inc no good for the stuff being moved to bootstrap?

chx’s picture

I do not like the ftp extension and the wrapper being tied together in a fallback like that. I rather would like to see http://drupal4hu.com/node/188#comments something like this happening.

chx’s picture

Also, it's awesome you are on this :) I presume it's because of the state of the patch -- but i do not see the backends actually called yet at all.

cwgordon7’s picture

@catch, I believe (though I may be mistaken) that common.inc is included at the very end of the bootstrap process, so it will not be available during installation for the very likely situation that we want to use the extensions subsystem to grab modules/themes during installation (see #395480: Plugin Manager in Core: Part 4 (installation profiles)).

@chx, I think I understand your concerns about putting the ftp extension and the fallback wrapper in the same backend, but I think it might be over complicating the issue by automatically choosing the backend to use. It is perfectly possible that I could connect to my server using ssh, for example, but do not have the appropriate privileges to do so, and need to use ftp. Would moving the ftp wrapper fallback into its own backend (and only displaying it as an option as a fallback) be a good solution?

@chx(2), I decided to try to get the backend in first before adding any sort of complicated interface awesomeness. I spoke briefly to webchick about this, and she said that it would be fine to split it off into multiple issues to avoid mega-patch-of-doom issues. This does not mean that in the final version of the patch, however, the functions will remain uncalled: they will be called by the test cases I have yet to write. (Side note: how am I going to test this? I can't guarantee that a test server will have ftp/ssh/etc. nor can I get the username/password during a test case. This would probably be an ideal place to use mock objects, but maybe I can get by with creating a fake module that provides its own test backend, and test ftp/ssh if they are available with certain default settings.)

moshe weitzman’s picture

Not sure if it would help, but we could provide a limited permissions ssh account with well known credentials at a specific location like testing.drupal.org. The test would use that ssh account by default.

Bojhan’s picture

From a usability/UX perspective there are still a few steps in using plugin_manager that need to be taken away, these are mainly in the installation process. For sake of simplicity I would also advise, not to call this plugin_manager in core but rather just module_manager - if it actually shows up anywhere (I hope it doesn't, if you introduce a new concept like exstenstions it might flow into drupal, which is bad for lots of reasons)

skilip’s picture

subscribe

Xano’s picture

I'd put this into Update.module. If you don't want updates, you disable Update.module and therefore the code to fetch those updates (plugin manager) should be disabled as well.

amariotti’s picture

I like this idea. That is one thing that another blogging CMS has that I wish Drupal did. It would make a lot of things easier. Great work!

dww’s picture

update.module was called "update.module" in part because of the desire to eventually handle more than just "status". I think putting code related to managing your updates should go in there. I haven't looked at any of the patch yet, just pointing to where the code should live.

Oh, and it looks like you kind of already understand this based on reading #395474: Plugin Manager in Core: Part 2 (integration with update status). I'm not exactly sure what the difference is between these two issues, in fact. You're just saying "let's add the thing currently called plugin_manager to core" here, and then you're saying "and let's make it interface with update.module" over there. Seems like this is really one issue: "move plugin_manager functionality into core's update.module".

dww’s picture

Skimming over the patch, it's obvious that most of this belongs in a (partly refactored) update.module. For example: extensions_get_release_history() has no business being a separate function. ;) You should either use the existing update.module function(s) for that, or refactor those to make them suitable for your needs. There's vast overlap here. That's not how we roll with our code in core. ;) Ditto define('EXTENSIONS_DEFAULT_URL') -- we've already got that constant, it's called 'UPDATE_DEFAULT_URL'.

I think all the plugin backend .inc files should just live in a sub-dir off modules/update, not in the core includes dir.

Side note: extensions_list_reload() -- EEEK. ;) You're parsing all of http://updates.drupal.org/release-history/project-list/all and caching it? Can you say evil memory bloat? :( Yikes. What is that function supposed to do? It's not called from anywhere in the patch, so I don't know why the plugin manager needs a "list" that needs to be "reloaded".

p.s. @cwgordon7: yay for working on this! Thanks!

Dave Reid’s picture

Agreed with dww, major kudos to cwgordon7 for taking this up! :D Yay!

I make a whole lotta sense that plugin_manager were to be integrated completely into update.module. Update sounds just simple, and effective. Users who usually don't want the normal update.module enabled probably wouldn't want any kind of plugin_manager.module functionality as well, so why not combine!?

Maybe we should move the component to update.module?

cwgordon7’s picture

Well, here's the problem with moving it into update.module: what about installing new modules? I think it would be a bit of a stretch to call installing new modules and themes "updates." The way I was envisioning it, there would be a subsystem to handle the low-level process of moving files around, which would then be used by update.module in updating modules/themes, and also by system.module in installing modules/themes. If this doesn't make sense, I'm happy to expand the scope of the update module to beyond even "automatic updates" to "quick updates & installations," but I thought it was most logical to separate the two. With that said, perhaps I was too quick to move certain update-specific stuff (namely the constant) away from update.module.

As for release-history/project-list/all - the original module (before I corrupted it in my initial port to core) was using it to list available modules. On second thought, though, it might be best to send people to drupal.org to browse for modules, and then send them back to their site to install them. Here's how that process might work:

1) Link on my Drupal website sends me to a special page on drupal.org (say, drupal.org/foo) with a get parameter giving the url of my website (say, example.com) so I end up going to a url that looks like http://drupal.org/foo?site=http://example.com/.

2) Special page stores my website by either setting a cookie containing it or by saving it in my session data.

3) I browse for modules/themes, and when I find one I want, I click on a button that says "Install on http://example.com/"

4) This button/link puts me back at http://example.com/admin/build/modules/install?module=example, which proceeds to be handled by my Drupal website.

I actually like this idea much better than my original idea of some sort of mini-module-browser built into every Drupal website - why not just reuse drupal.org's module/theme browsing system? I'm not sure how feasible this will turn out to be, I'll leave it up to the drupal.org infrastructure people to tell me whether this is workable or if I'm just crazy. ;)

About usability/module naming, the current patch (a) doesn't touch the user interface, and (b) doesn't create or rename any modules, so I think this concern is misplaced.

Also, thanks moshe for suggesting using some central location for ssh tests - if the testing servers could handle it, it would be very useful, but I'm not sure if the additional server load will be too much for such a marginal testing benefit. I'll again leave that up to the drupal.org infrastructure people. ;)

Thanks everyone for all your useful and encouraging comments, I'm sorry I didn't post any updated patches this Saturday / Tuesday (SAT's and out of town, respectively), but new and updated patch this Saturday at the latest! :)

Xano’s picture

Pro's and cons:
Browsing@d.o
+ No custom browser needed.
+ No need for saving all the data locally.
- D.o is slow.
- D.o needs to be maintained. This could possibly conflict with future versions of this module.

Browsing locally
+ Fast
+ One environment. Users aren't being confused because they need to switch between their own site and d.o.
- Custom browser is needed.
- All module, theme and theme engine data needs to be saved locally.

Saving all data locally may or may not cause extra data traffic. I think that most experienced users don't need this, because they go to project pages more or less directly. Fetching all data from d.o by cron causes extra traffic for these users. For newcomers that browse the repo relatively often, traffic may be decreased. Disclaimer: this is a rough estimate.

I am still in favour of putting all this functionality in Update.module, which we could possibly rename. All these features are based around a single idea and IMO that is why they should be grouped together.

@cwgordon7: Kudos for your enthusiasm! :D

skilip’s picture

I agree we should not want to be redirected to d.o. Fetching contrib data using cron would cause uneeded data traffic. What about caching the fetched data locally for a certain lifetime? For example: if an admin visits the contributions (I like either contributions or extentions for a name) page for the first time, all info is fetched from d.o. The data is then cached for let's say one day.

Oh, and what's up with Joshua?

dww’s picture

Lots of the effort of the drupal.org redesign is going into making it easier for people to find the modules and themes they want. I think trying to duplicate all of that effort directly inside drupal core itself by having core download a gigantic XML file with data about all modules and themes, parse that, and provide "native" tools to browse it seems like the wrong direction entirely. In fact, I'm not sure core should know how to install new modules for itself like this at all, but I'm willing to have an open mind about that. ;)

I think core should know how to upgrade itself and whatever is currently installed (never automatically, btw, only when a human says so -- given the wide range of care and ability that contrib maintainers display, I'd never trust core to upgrade itself to the latest releases that are available). If you want to add to what you've got, you should search on d.o for it and then either a) download it yourself, b) use drush, or c) use some other contrib module (plugin_manager D7?) that attempts to do some other fancy thing. I don't believe any solution to this problem has matured enough to try to stuff it into core yet.

That said, when you're adding a new module or theme to your site, you're "updating" your site. http://dictionary.reference.com/browse/update says:

Computers. to incorporate new or more accurate information in (a database, program, procedure, etc.).

Installing a new module is "incorporating new information" in your site. That's updating. So, I still have no problem with the functionality related to finding out what's available and installing it on your site all living in update.module itself, even if we're considering the case of installing things you don't already have.

Dave Reid’s picture

Agreed with dww. d.org module browsing has made huge progress already and will continue. It's probably best that we create more memory bottlenecks where we can't afford it.

Having an 'installer' plugin_manager left in contrib while allowing update.module to perform updates sounds good. Or maybe working on a simplified installer can work.nMaybe some kind of simplified installer interface, like 'go find the module you want on Drupal.org', paste link to the .tar.gz to download, and Drupal will install the module with MD5 checking.

skilip’s picture

A little calculation:

Lets say an average module contains 300 characters (the number of characters in an .info file) which means 300 bytes. Add all json format data to that, and we'll have appr. 350 bytes. Multiply that with appr. 4000 modules currently available, 1.400.000 bytes, which is 1.3 MB. If this is gzipped using cron everyday, you'll end up with appr. 200 KB.

What's more server consuming? Creating this (large) json-formatted, gzipped file once a day and offer this to be downloaded, or visiting drupal's project page tens of thousands times a day?

It's beyond my knowledge, but I guess the first idea is less server consuming.

Xano’s picture

In that case we'd have a simple browser in every Drupal installation for quick administration, containing only module's titles, descriptions and packages. For more information we could point people to the project's page at d.o.

dww’s picture

Note: I'm not trying to optimize for reducing traffic on d.o. d.o is a big beefy site, with scalable architecture and room to grow. I'm trying to optimize for not requiring at least 1.5 megs of RAM for drupal core to work at all (since it has to parse that entire file every once in a while) And of course, that's a very small estimate, since even if you have just the title and description (and the link to the project) in the big file, you also need release information, too (at least for anything you're trying to install). So, it's probably going to be closer to 2 megs. Plus, modules (and themes) on d.o are growing at an alarming rate. By the time D7 core is in the wild, there might be 5000+, and by the time D7 is EOL, it could be 7000-10000 or more, who knows.

I'm going to use more strong language now: it would be a disaster to give this task to Drupal core. Please don't do this, or I'm going to have oppose moving any of this code into core. At the very least, it's a discussion for a separate thread. Think of the kittens. ;) The whole point is doing this in bite-sized pieces. The first bite is having update.module know how to update your site. Let's start [sic] "simple" and build out from there. Even if that gets in to D7 with all the hurdles that task faces it will be a huge accomplishment and a nice usability win. And, if we do it right, it'll be easier in D7 to have contrib modules that worry about installing new things on your site in various ways.

Xano’s picture

What if myContrib2 7.x-1.x depends on myContrib1 7.x.1.x, while myContrib1 7.x-2.x has already been released? We must prevent the system from accidentally suggesting this kind of updates for dependencies.

skilip’s picture

@dww I haven't thought about the work involved for Drupal core to realize this. You made your point. You're also right about the kittens. I like to dream along about all cool features and possibilities but always get rudely awakened by all kinds of kittens crawling on my bed.

FOCUS!

cwgordon7’s picture

I agree with the comments above, we'll start small, leaving the task of installing new modules to drupal.org. My idea in #21 will remain my dream until we have a clear way of implementing that. I will also move the code to update.module, which may need a renaming.

I believe that the problem outlined in #30 represents a deeper flaw in the dependencies specification through .info files - we really want to be able to specify dependency upon a specific branch or version of a module as well. I'd encourage work on #211747: Allow specifying version information as part of module dependencies, especially when/if this patch is committed.

Again, updated patch coming this Saturday, thanks for all your input!

dww’s picture

Component: base system » update.module

I see no reason to rename update.module for this feature.

Xano’s picture

Not yet, but it might be needed if we add the possibility to install modules from scratch.

cwgordon7’s picture

By "rename," I meant simply the user-facing name: "update status" would no longer seem an adequate description. Of course, this can wait for one of the followup issues, and I see no problem with the internal name "update."

dww’s picture

Ahh, right. The "name" field in the .info file. Sure, that part makes sense. I guess "Update manager" would work.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
33.48 KB

Ok, here's a patch with all features now implemented, though I'd be very surprised if they weren't still buggy. I wrote automated tests, and managed to get them to pass on my WAMP setup, but that's no indication of whether or not they'll pass on other, more common configurations.

Several changes between this and the previous patch (I may have forgotten a few):

- Automated tests now included! The automated tests assume a set of default configurations, but can be overridden in settings.php.
- Everything has been moved to update.module, and the hook has been renamed hook_update_backend().
- FTP is now only offered if SSH is not available. I was initially concerned that the user might not have SSH credentials, but chx pointed out that the SSH extension is extremely rare, so if it's there, it is very safe to assume that the user has SSH credentials.
- After a lot of nasty debug work, all the backends now actually work! At least sometimes. At least on my WAMP stack.
- The internal database caching of the huge page of doom has been dropped. I'm working on the assumption that any module browsing that will occur will happen on drupal.org - it's too great a task for all Drupal installations to be able to offer built-in module/theme browsing.

There are probably more changes, but I'm forgetting them right now. I'll post later if I remember any big ones. But for now, this needs review, especially on different operating systems or server setups than the one I'm using (WAMP Server on Windows XP). I may, however, have difficulty debugging problems on different setups, so if you could track down any problems as far as possible, that would be much appreciated. Thanks!

cwgordon7’s picture

Updated patch, addressing several comments:

- Drewish noted that function starting with names like 'ftp' is a bad idea, they're now all prefixed with "update" as well.
- Chx wanted ftp extension and ftp wrapper separated into different backends, and now they are, and they each have their own test case now.

Testers still needed for linux/mac! To test, follow the following steps:

- Make sure the ftp extension for php is installed. If it isn't, see http://php.net/manual/en/ftp.installation.php for further instructions.
- Make sure allow_url_fopen is enabled in php.ini.
- Install the ssh extension for php; see http://php.net/manual/en/ssh2.installation.php for further instructions.
- Go into your drupal 7 install's settings.php file, and override the following variables in global $conf:

* $conf['ftp_username'], $conf['ftp_password'], $conf['ftp_host'], and $conf['ftp_root'] should correspond to your ftp username, ftp password, ftp host, and the path from the root ftp directory to your Drupal installation, respectively.
* $conf['ssh_username'], $conf['ssh_password'], $conf['ssh_host'], and $conf['ssh_root'] should correspond to your ssh username, ssh password, ssh host, and the path from the root ssh directory to your Drupal installation, respectively.

- Enable the SimpleTest module and run the tests! Ideally, there will be 14 passes. ;)

Note that if you're unable to set up an ftp or ssh environment, you'll still be able to test the other backends.

Also note that you do not need to be running ftp/ssh actually on the machine you're running tests on, so you can run tests on localhost even if you're writing files to a remote machine.

Thanks for the enthusiasm, everyone! :)

drewish’s picture

subscribing

Wim Leers’s picture

Subscribing.

dww’s picture

First of all, rock on! However,

"- Make sure allow_url_fopen is enabled in php.ini."

Eeek. Why do we require that? I was under the impression that any site that's even moderately security conscious turns this off due to all the potential for badness that can happen if fopen() can open arbitrary URLs. update.module itself doesn't care about this -- it just uses drupal_http_request() -- surely the new feature (and its automated tests) can do the same, no? I'm tempted to mark this "needs work" on that basis alone, but I don't want to discourage other reviews and tests which clearly still need to happen.

Also, I haven't looked at all, but putting your ssh password in settings.php is only needed for the automated tests, not the proper functioning if the feature itself, right? I pray that the code prompts you for your password (which is never cached or written anywhere) while you're trying to update code interactively, correct? If not, that's another giant "needs work". ;)

Thanks,
-Derek

cwgordon7’s picture

Sorry, these were just the instructions for testing. Basically, there are three backends, all with separate requirements for actually running:

- ssh, which requires the ssh extension
- ftp_extension, which requires the ftp extension
- ftp_wrapper, which requires allow_url_fopen

We check for these in the above order, which means if you're a security-conscious site, you probably want to be using ssh anyway, or if not ssh, then ftp_extension. Chx suggested that someone write an "ftp_socket" backend for this, which I would encourage anyone to do. I'm not an expert on allow_url_fopen, so if you think this option is too insecure to even offer, I'd be happy to remove that backend to be replaced by an "ftp_socket" backend.

And yes, don't worry, putting your ssh/ftp passwords in settings.php are only required for the automated tests. I couldn't think of a better way to get the settings during the test, so I asked for them in settings.php. I'm not sure, but I'm guessing that it'll be relatively easy for testing.drupal.org to change variables in global $conf before running the tests.

scor’s picture

I've tried this patch on Mac OS Leopard's built in FTP server, and it requires to specifiy the absolute path in $root but I believe this is a Mac OS quirk. Other than that, I found the FTP extension tests work from my Mac to the local FTP server and a distant regular FTP server on Linux.

For the FTP wrapper I had to remove is_writeable($target) in update.ftp_wrapper.inc since after a chat with cwgordon7 we found is_writable() is not supported over ftp stream. (patch rerolled attached).

I haven't tested this with the ssh extension.

dww’s picture

Rerolled for a few things:

A) hook_update_backend() is a hook invoked by update.module, and should be documented in update.api.php, not system.api.php. Fixed some typos in there while I was at it.

B) I see no reason for system.module to implement this hook on behalf of core. update.module should do so itself.

C) Added newlines at the end of the newly added files (minor pet peeve).

Still haven't actually tested this or played with it yet, nor did a careful review of the underlying code in terms of a security audit, etc. But, it's getting closer. ;)

dww’s picture

One more missing newline. ;) (sorry).

cwgordon7’s picture

Awesome, thanks dww! I agree with the logic behind changes A and B, and don't really care about C, but if it makes dww happy, then great. ;)

Side note, are the functions in api.php files supposed to be in any specific order? I thought that ordering them alphabetically made the most sense, but it would be nice to know if there are any conventions for this.

Scor gave this a quick review on mac, mentioned above in #43; if someone could give this a thorough test on linux, that would be awesome! Thanks guys. :)

Berdir’s picture

Looks interesting, a few ideas/questions/opinions, in no particular order

1. I'd love to have such a feature in core. However, to be in core, it imho needs to work on as many hosts as possible, and if it doesn't, it should state clearly (= understandable for a normal user), why not.

2. It would be great if other ssh2 authentication mechanism beneath pasword would be supported, for example private/public key. The problem with password is that it can be disabled by the SSH server (Note: Most (if not all) ssh clients like PuTTy use the "keyboard-interactive" authentication mode, which is not the same as "password" used by ssh2).

3. A more flexible fallback system. Also related to the point above. For example, if the ssh2 module is available, but the configured Server does not support Password-Authentication, it would output a dsm message try the next system. One way to do this could be another special function, for example update_{backend}_test(), and the backend is only used if that function returns true.

4. Using shell command should be avoided imho, they might not be available on some OS or with different params. But I know that's not easy, especially with tar. (delete/mkdir and so on is possible, see below). The plugin_manager in core advices to download the Archive_Tar module from

5. It's probably better to use the sftp subsystem instead of scp, if available, as it is more reliable*. It also provides functions like unlink, mkdir and can also be used as a stream wrapper, for example: ""ssh2.sftp://$sftp$remote_file", where $sftp is the resource returned by ssh2_sftp().

6. In reference to 1, it might be better to let the update_{backend}_.. functions handle the configuration (username/password, root directoy, ..) themself, because some backends might use other authentication params than username/password and so on. Or provide something like a $conf array.

7. I'm wondering how long it will take until someone writes a local-backend, which directly writes the files and requires to have them write-able by php :p

I can try to implement some of my points in the ssh2 backend, I've used that pretty much some time ago.

reliable*: See comment 1 at http://kevin.vanzonneveld.net/techblog/article/make_ssh_connections_with..., Sara Golemon is the author of libssh2 and wrote the ssh2 extension originally.

cwgordon7’s picture

Status: Needs review » Needs work

First of all, thanks for the comments! My responses:

1. Completely agreed. I'd love to have this support as many servers as possible; however, at the moment, I feel what we have in the current patch is a very good start. As for explaining clearly why it doesn't work, that's for one of the other three issues, since this issue doesn't deal with the user interface at all.

2. It does sound like it would be cool to support other ssh authentication mechanisms. I am very wary, however, of scope creep on this issue; this issue has the potential to grow into a monster patch of doom, and I'm trying very hard to keep it down to a workable level. I'd definitely encourage you, though, to open up a followup issue for it, it's definitely something I'd love to have in core.

3. I'm also not too comfortable with the fallback system - what if ssh extension is available but the user doesn't have ssh credentials for the server? However, chx seemed convinced that the ssh extension was rare enough that if it was present, we could be reasonably sure that it was there for this purpose. I could be easily convinced that we should just eliminate the automatic fallback system. ;)

4. Right, the plugin_manager module in contrib advocates the use of a PEAR package; I made a decision early on in #395472-1: Plugin Manager in Core: Part 1 (backend) not to include the PEAR package in the port to core. This is in part because of licensing issues; I'm not sure what all the implications would be.

5. That sounds interesting... are you suggesting we replace the current scp implementation with an sftp implementation, or simply add an sftp implementation in later? I just ported the implementation in plugin_manager over, I don't really deeply understand the differences between sftp and scp. ;)

6. Yeah, that sounds like a good idea. I'll work on a new patch (setting back to cnw).

7. Hopefully not! It's written on the plugin manager project page as a frequently asked question, though... the best we can do is publicly state that such a backend would be insecure and will be unpublished on drupal.org. (Although, would it be so bad if we required a sudo password?)

Setting back to code needs work for the point in #6, new patch coming sometime in the next few hours.

pwolanin’s picture

For #5 it sounds like sftp instead of scp - there's no obvious reason you'd need both.

tstoeckler’s picture

Tested the patch on linux:
Applied cleanly and I ran update.module's tests. There were only 3 (1 for each of the backends), but they all passed.
I didn't set up any ftp server or ssh environment or anything, I wouldn't have a clue how to do that. I just followed the steps in #38 but I didn't edit my settings.php, because I didn't think that was necessary as I have allow_url_fopen enabled.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
33.4 KB

@#50 - yes, each of the tests will pass with 1 pass if you don't have anything set up - with the message "Could not connect to FTP server" or something like that. This is so tests will still pass without any manual configuration - but it doesn't mean anything. We still need a thorough test on a linux based operating system. Thanks for the effort, though.

@#49 / #47(5) - I'm really not sure of the implications of using sftp instead of scp, and I'm scared of touching it, since the original module used scp. The comment referenced in Berdir's post seems very dated, and I'm a bit skeptical of its current relevance.

Patch rerolled to incorporate the suggestion in #47(6). I'm marking it back to needs review for now because I'm not entirely sure if any of the issues brought up in #47 that I have not addressed need to be addressed before this is ready - but also, because I'd really like a thorough review on linux (should have 14 passes if all backends work properly).

Berdir’s picture

As I said, just some ideas I had while looking at your code, no need to implement that right now :)

2. As you already implemented 6, it is easy possible to do this in a follow-up patch. It would be hard if the API would be limited to username/password.

3. Did you already thought about the configuration UI, or how it should work for the user ? There could be two levels..
a) Only present the ssh2 configurations if the ssh2 extension is available. Maybe also additional tests, for example if the backend and the server have a common authentication mechanism and so on.
b) After the configuration settings were entered, there is another test which tries to connect to the server. If that works, the ssh2 backend is enabled and can be used to install/upgrade modules.

4. Yes there are legal issues and also technical ones (The pear package is pretty big, afaik, everything that is used by core should be already available, installing pear packages is not really easy :). Maybe there could also by multiple extract backends, so that someone in contrib could provide a archive_tar integration. There are also different compression extensions available in php, see http://ch2.php.net/manual/en/refs.compression.php. I heard someone was talking about providing zip downloads on d.o, that could make that easier.

5. Not sure. while my link is old, ssh2 development isn't very active. The 0.11 release in dez 08 was the first since 2005. I have also no experience with the 0.11 release, I've only used 0.10. The problem with sftp is that it is optional, while scp should be always available.

tstoeckler’s picture

Sorry in advance, but as still no Linux tester has reported back I feel important enough to bug you guys with my newbie issues:

I have followed all the steps in #38 to test this patch, except for installing the SSH thing. I have installed FTP and I know it works, because I can access my local webserver through FileZilla. Then I edited my settings.php file according to your description (at least that's what I thought), but I still get only 3 tests passing. Now if I've understood you correctly, the backends should function independently, that means the FTP thing (is it the extension or the wrapper, I can't tell?) should still work, even if the SSH thing doesn't. That's where I'm hung up.
The end of my settings.php (I just added it to the very end) reads:

$conf['ftp_username'] = myusername;
$conf['ftp_password'] = mypassword;
$conf['ftp_host'] = localhost;
$conf['ftp_root'] = www/htdocs/drupal7;

Now, again, I KNOW the credentials are correct because they work in my FTP client.
I would really like to help, so any direction would be of great help. Thanks!

cwgordon7’s picture

@#53: Thanks for testing this! Several questions:

1. What are the exact passes you're getting? Could you paste them here?
2. Has your cache been cleared since you applied the patch? A stale registry could lead to drupal_function_exists() returning false in some cases that could disrupt the test.
3. If all else fails, have you tried using an absolute path to ftp_root? I can't see why this would help, but it helped scor get it to work on a mac... maybe this ftp_root thing needs more thought...

tstoeckler’s picture

Ahh, I never noticed that SimpleTest is so verbose.
It actually reads:

FTP extension update functionality
...
1 pass, 0 fails, and 0 exceptions
No FTP server was found at the given location.

FTP wrapper update functionality
...
1 pass, 0 fails, and 0 exceptions
The supplied username/password combination was not accepted, or no local ftp server is running.

SSH update functionality
...
1 pass, 0 fails, and 0 exceptions
PHP cannot open SSH connections.

So I guess he recognizes the FTP wrapper at least...
I'll play around with it a little more...

tstoeckler’s picture

Short update: Using an absolute path (srv/www/htdocs/drupal7, right?) didn't help. I noticed when reading through update.test, that I probably have to put the values after the = in '' (those little apostrophe things). I did that, but it didn't help, it reads the same thing as posted above. I also noticed that, even though the description says, that the default root directory is assumed to be "drupal" when the test reads "NONE" at that point, so I thought maybe the test appends "drupal" to every path, so I tried that (I renamed my root dir to drupal and set the FTP server in the dir above) but it didn't help either.

scor’s picture

@tstoeckler: in settings.php, make sure you use single (') or double (") quotes around the values in $conf. PHP should complain otherwise.

tstoeckler’s picture

See #56: Tried that. Maybe "double quotes" will help. I'll see. Not today, though...

cwgordon7’s picture

I'm disappointed that this has sat at code needs review for almost two weeks without action now. I'd really like to get this in core as soon as possible so that I can work on the interface ssues & install profiles (etc.) before code freeze.

I borrowed my sister's laptop to test on kubuntu. It seems to work fine - I'm not sure what problem tstoeckler is having. I set up an ftp server locally using vsftpd, modified settings.php, and everything seemed to just work™. @tstoeckler - one thing you can do to try to debug is go into to modules/update/update.ftp_wrapper.inc and add the following line at line 28: file_put_contents(DRUPAL_ROOT . '/debug.txt', $ftp_base_dir);. Then, after running the tests, look at the contents of debug.txt (in your drupal root directory); it should read something like ftp://username:password@server/. If anything is obviously wrong with the ftp server credentials, you need to change what you've specified in settings.php for the tests. Otherwise, make sure the string works for ftp if you type it into your browser - firefox, for example, supports ftp. If it does not work, the ftp credentials are either incorrect or the ftp server is not running. If it does work, and the tests still fail, then something is wrong with this patch. Also note that double quotes ("") shouldn't be any different from single quotes ('') - see the PHP documentation for more information. If you need help debugging, I'm generally available (cwgordon7) on the #drupal irc channel on irc.freenode.net - just ping me.

Patch attached contains 2 minor changes: (1) There was a space missing in update.ssh.inc, (2) Still list ftp, if available, as an option even if ssh is available - the user interface should really handle any automatic selection like that, the backend still should know about all the available backends.

I've now tested this on linux and windows, and scor has tested on mac - if someone could just give this one more thorough code review, I feel it could be rtbc.

Thanks everyone!

cwgordon7’s picture

Berdir pointed out that we should be suppressing PHP errors where we're outputting our own with drupal_set_message() errors instead. The attached patch revises the previous patch with a bunch of @ error suppressions added. I also noticed that we have 5 passes in each of the ftp tests, but only 4 passes in the ssh, I updated it for the sake of consistency.

cwgordon7’s picture

Further suppression of PHP errors, and more nice display of errors through drupal_set_message().

tstoeckler’s picture

FileSize
82.79 KB

After killing my Linux install (twice) I actually managed to set up a webserver and after applying the patch and running the tests a gazillion times, it magically works. Well not quite. But it seems the "FTP wrapper" and the SSH thing somehow works. I'm getting fails, but as they are all related to some access issues on my webserver I'm not sure, that this speaks against the patch (note: I chmodded my whole drupal directory to 777, nothing changed).
cwgordon7 I attached a screenshot of my test results page, you'll know what that means better than I do:

EDIT: I noticed that one test says "Module removed successfully" but is marked as FAIL. I looked in sites/all/modules and the (coder) module is actually not removed.

tstoeckler’s picture

Possibly related, just posting in case this means that the patch is perfectly OK (pretty, pretty please?!):
After getting the screen above, I get following error a bunch of times:

Warning: opendir(sites/all/modules/coder): failed to open dir: Permission denied in file_scan_directory() (line 1422 of /srv/www/htdocs/drupal7/includes/file.inc).

And I can't access sites/build/testing anymore (WSOD) until I remove coder module manually. Then everything is fine again.
Oh, and I have clean URLs disabled.

cwgordon7’s picture

Status: Needs review » Needs work

Right. For whatever reason, it seems like the ftp_wrapper backend is failing to remove the module on your installation; all the other errors seem related to that. Also interesting, for whatever reason there's a permission problem for the uploaded module (in fact, that's probably the reason it can't be removed). I just retested on my kubuntu setup, and I can't reproduce either of the problems.

A few questions:

1) Does it work with the ftp_extension backend? Or can that not remove the uploaded module as well?

2) What are the file system permissions on the uploaded module? (sites/all/modules/coder)?

3) Is your ftp server allowing files and folders to be removed?

4) Is the "coder" folder (or any of its sub-folders) empty? (have any files been deleted from the coder module file system?)

Thanks again for taking the time to test this. Since I'm unable to reproduce, I have to ask these annoying questions to try to figure out what's wrong. I hope you have the patience to bear with me while I stumble through trying to determine the problem using binary elimination. Thank you. :)

tstoeckler’s picture

1) the ftp_extension doesn't work at all, I'll have to look into that (As is noted, mod_rewrite also doesn't work, so that is 99% my problem)

2) I'm pretty sure I 777ed coder module as well, but I'll retry that later.

3) That sounds like it could be the answer, I will look into my config and search the internet for that

4) No coder module is not empty, it seems as though all files are there (although I didn't check for every single one)

Concerning 2) and 4): After running the tests and therefore coder module being in my sites/all/modules, (as noted above) my admin/build/testing is a WSOD, while all of the other admin pages are fine. admin/build/modules probably broken too, but didn't check that.

Just one more note, probably worthless, but hey: I tried your debugging thing and that didn't work either (it actually showed up as a failed test) so I guess the webserver didn't have permissions to write that file (even though I 777ed it).

All in all, my server is weird, and I hope that's the problem. I'll fiddle around with it either later today or tomorrow and report back.

Btw cwgordon7: Thanks for being so helpful, I really wish I were a Linux/PHP guru, but I'm really struggling and I would be absolutely lost without your great advice.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
32.78 KB

The information highway truely is a blessing: I now know what "chroot" and "umask" means, and more importantly, I got the patch to work (see screenshot below).

Now I only tested one of the backends, but as you and others already tested the different backends for consistency, I actually dare to set this RTBC. Please revert if you feel this is inadequate, I am just so infamously proud of myself for working that whole thing out, that I feel nothing can stop this patch now (if even I can't).

xmacinfo’s picture

Can a security audit be done before committing this patch? Or is it better to do the audit only when the patch is committed?

dww’s picture

Status: Reviewed & tested by the community » Needs review

Indeed, this is far from RTBC. I still haven't actually looked closely at the code (and as the maintainer of update.module, I should really give this the thumbs up before it lands). No one else from the security team has looked closely at this, either. I'm glad to hear that folks have managed to see each of the backends work (though I'll point out that the various troubles people were having getting them working and debugging the problems tell me that the usability and error reporting for the backends isn't necessarily great). Deep reviews of the code, the APIs it introduces, the security implications, and the sanity of the integration with update.module are all welcome and necessary before this is really RTBC. Thanks.

catch’s picture

There's no UI for the backends yet, only unit tests, so I don't think usability and helpful error reporting is an option yet. However this does need some proper review. afaik the general approach has already been thoroughly vetted in plugin manager from a security standpoint, but obviously this specific code hasn't.

Berdir’s picture

There are a few issues with the current approach, I already talked a bit about it with cwgordon7

- It will currently only work on hosts which have a "tar" command that supports the used arguments. We should atleast make that switchable too, so that a contrib module could provide integration with Archive_Tar or other solutions for Windows etc.

- While it is true that it is not related directly to usability etc., a UI built on top of this can only work the information given by this. And for example, it does currently not test if a directly is writeable or does exist before it tries to copy a file into it (atleast for the SSH backend, I haven't looked at the others). And again, it assumes that commands like rm and mkdir exist. Imho, we need to improve the error handling, Exceptions might be a possibility, a UI could catch these and provide nice error messages with help and links and whatever. But to do that, it needs to know what actually failed...

Dries’s picture

This is starting to look really good. Nonetheless, a bunch of feedback:

- I agree that browsing should be done on drupal.org.

- I think it is OK to support tar only for the time being, and add support for other formats later. Let's keep this simple for now.

- We're missing a paragraph of text that describes the bigger picture/architecture of the update system. We need to explain how a backend works at a higher level, and why it was designed to work like this. Instead, we dive right into the hooks without bringing the developer up to speed on the overall design and architecture. This shouldn't be long -- it can be to the point.

- "These backends are handled by system.module." This seems no longer accurate.

- update.api.php implements an example hook "hook_update_backend()" that doesn't seem to match the documentation. The documentation talks about update_NAME_add_extension, update_NAME_remove_extension, update_NAME_settings_form but not hook_update_backend. I was confused by that -- it looks like the example is incomplete.

- Instead of using update.ftp_extension.inc I think we can use update.ftp.inc. It's cleaner because we don't end up mixing dots and underscores in filenames. Ditto for the other backends. Either way, we use dashes, not underscores for most file names other than modules.

- update_ftp_extension_add_extension could become update_ftp_add_extension.

- From an API point of view I was surprised to see that update_ftp_extension_add_extension actually opened the FTP connection. The API seemed to suggest that it merely registers a backend, rather than trying to connect to a server. It made sense once I read the code. Renaming the hook from _add_xxx to _upload_xxx would make it more intuitive.

- In documentation and user output (i.e. dsm()), 'drupal' is written as 'Drupal'.

- update_extraction should be update-extraction. Ditto for update_cache, etc.

-

+function update_remove_extension($backend, $files, $settings) {
+  $function = 'update_' . $backend . '_remove_extension';

In general I like it better when we glue to backend at the end of the name. So the generic function is update_remove_extension() and the backend specific one is update_remove_extension_BACKEND. It flows more hierarchically and therefore more natural to me. Eg. instead of update_ssh_add_extension, use update_add_extension_ssh.

- I'll see if, at Acquia, we can build a version of the Acquia Stack Installer (DAMP) with all of these extensions enabled. It might help local testing.

Keep up the good work -- this is exciting. I'm going to do a deeper review later on.

cwgordon7’s picture

Assigned: cwgordon7 » Unassigned
Status: Needs review » Needs work

Thanks everyone for the thorough reviews.

I am going to be very busy for the next few weeks, so if anyone else wants to pick this up, please feel free.

If no one picks this up, I'll get back to this after my exams are over.

dww’s picture

@Dries: re: "I'm going to do a deeper review later on.":

No offense, but before you do any deeper reviews of this, can I get a confirmation of the proposed plan at #220592-10: Core cache API breaks update.module: fetches data way too often, kills site performance, etc first? ;) That's a lot more important in the short term. Thanks.

cwgordon7’s picture

Assigned: Unassigned » cwgordon7

Updated patch coming hopefully sometime this weekend. :)

Dries’s picture

Issue tags: +Favorite-of-Dries

Tracking.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
37.78 KB

Ok, I think I've addressed all the points made in #70 and #71.

#70:
- I've made the untarring process pluggable.
- I think I've improved the error handling in the SSH backend. Please let me know if there's anything else I should be checking for, but keep in mind that it shouldn't be difficult to add more sophisticated error checking later (though I think the current error handling is pretty slick itself).

#71:
- Agreed, browsing on drupal.org. :)
- I made the untarring process pluggable, but I only kept the simple tar command in core. We can add more later if we deem it necessary.
- I added a small paragraph to the documentation of hook_update_backend().
- I fixed the reference to system.module to refer instead to update.module.
- Perhaps the way I lay out my documentation was confusing - the functionality I described in the bulleted list in the documentation of hook_update_backend() refers to what the callbacks should be doing, not what the hook does. I wasn't quite sure where to document those, as they are not proper hooks, so I thought that documenting them in hook_update_backend() seemed best. I've changed the documentation a bit to make it clear that it references the callbacks, not the actual hook. Please let me know if there's a better place to put that documentation that I'm unaware of.
- I removed the underscores from the file names, and replaced them with dashes, but I was forced to keep it as update.ftp-extension.inc in order to distinguish it from update.ftp-wrapper.inc.
- Similarly, I kept the original function name to keep it distinguished from the ftp wrapper extension.
- I think my original documentation was bad, which was the source of the confusion here. Hook_update_backend() is what merely registers the backend; the add/remove callbacks are what actually add and remove extensions (modules and themes). I hope the newer documentation clarifies this properly and this makes sense to everyone. I'm happy to change the name from add to upload if we decide it's better for developer experience, but I thought it made more sense to keep it as add/remove, which are more obvious opposites.
- I changed drupal to Drupal in a variety of places, thanks.
- I changed the folder names to use dashes, not underscores, thanks.
- I changed the ordering of the function names. I didn't really think it mattered much, but if it makes more sense to others, then sure.

A new updated patch is attached. I feel this is slowly morphing into the gigantic patch I was trying hard to avoid. ;) Thanks everyone for the reviews!

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

The PHP syntax error is:

Fatal error: Cannot redeclare update_remove_extension_ftp_extension() (previously declared in modules/update/update.ftp-extension.inc:81) in modules/update/update.ftp-extension.inc on line 155
Errors parsing modules/update/update.ftp-extension.inc

Just looked over the patch, looks really good now, I'll test it soon.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
37.79 KB

Whoops, sorry about that PHP syntax error. Hopefully it's fixed now.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

Forgot to change the .info file file name references when I changed the file names. Should be passing for real this time.

webchick’s picture

Status: Needs work » Needs review
dmitrig01’s picture

Status: Needs review » Needs work

Inconsitencies:
In hook_update_backend it auto-adds update_*_NAME In hook_update_untar it doesn't.
What about having hook_update_backend declare NAME and the pattern be [moduleName]_update_[name]_[*] or just omit the? The pattern this uses isn't used anywhere else in core, and this is at least slightly more familiar.

dmitrig01’s picture

Also in the docs be more discreet about what $files and $settings are.

dmitrig01’s picture

One more thing: Offer all options (ftp, ftp wrapper and SSH). a ncie interface would be: You're upload is with SSH (change) or somehing - suggesting a nice default but letting you change it. if you can change from ssh to ftp you sholud be able to choose which FTP.

chx’s picture

First of all: fantastic work.

One thing I am wondering about is overwrites. I am fairly confident that ftp_put overwrites files (there is a comment indicating this on php.net, too) . Same for copy with the wrapper. This IMO needs to be mentioned in the docs.

t('The supplied username/password combination was not accepted, or no ftp server is running.') this is just guessing. Say something like "Could not upload with the supplied username and password". There can be a lot of reasons for that -- ftp wrapper disabled in the first place, the webserver cant connect to the ftp server etc (this message is there more than once)

In the wrapper rmdir, IMO we should use a RecursiveIterator for brevity. That might remove the helper altogether.

// If we the FTP path is not valid, abort. say what?

Add a todo please to $result = drupal_http_request($path); that we need something better here. You are storing a downloaded tarball in RAM. That's not the best idea. We need to change d_h_r to support saving the results in a file, to store the blocks as it reads em into the file (and there is an issue to make d_h_r pluggable so we can use curl whenever its there). Not this issue, but definitely needs to be done before release.

array_key_exists($method, $available_untar_methods)) unless you expect $available_untar_methods[$method] to be NULL, change this to isset.

if (!empty($file_list)) return $file_list; return FALSE; . Why not just return $file_list? The empty list nicely casts to FALSE...

Similar is return !empty($backends) && !empty($untar_methods); just return $backends && $untar_methods; will be the exact same. (!empty and (bool) always is the same and && gives you the bool cast).

mkdir --version does that work on Windows? Why are we testing in the first place? Later on when we try to make dir we check anyways.

same for rm.

I would rather not hardcode a dev tarball in Drupal 7 release. That makes me go uuuuuh -- can we get a tagged, therefore nonchanging tarball instead?

Thanks again.

dmitrig01’s picture

Sorry for scattering thoughts (this will be the last post) but here are more comments
- +function update_untar($method, $file, $directory) {
You should check if the method exists first in the $methods

<?
+/**
+ * @file
+ * The FTP Backend for the plugin manager.
+ */
+
+/**
+ * Install the supplied files to the appropriate locations.
+ *
+ * @param $files
+ * An array of files to be added by the specified backend.
+ * @param $settings
+ * An array of settings, which will vary among different backends, but may
+ * include data such as the username, password, and host.
?>

Its not gonna vary within the ftp backend. And "speicfied bakcend" is kinda vague since we know we're dealing with ftp

<?
+ drupal_set_message(t('No ftp server could be found.'), 'error');
+ return FALSE;
?>

This makes for not so pretty error handling. What about returning the error message so it can be displayed wherever the interface feels like it?

Please document the differences between backends)

Also you can pick a unstable release to downloiad

boombatower’s picture

subscribe

JacobSingh’s picture

I can't get this patch to apply... Getting rejects in update.module Isn't anyone else getting this?
I #hate #cvs

JacobSingh’s picture

Here is an updated one which merges and passes tests.

webchick’s picture

Status: Needs work » Needs review
JacobSingh’s picture

Status: Needs review » Needs work

Huge time sync today trying to put a RecursiveDirectoryIterator in as per chx's suggestion.

It was a bit dicey because PHP has the following bug:
http://pecl.php.net/bugs/bug.php?id=9340&edit=3

which erroneously throws a warning when it actually works fine

but on top of that... just weird stuff. Like php leaving one file in certain directories, but deleting the rest.
Just to test my sanity, I also ran it against a local dir (the same one) and it works fine. I also ran it against another ftp server (didn't work).
I'm going to have to file a PHP bug and get a drink....

Another thing about this module we need to fix. The "unit tests":

// Now, remove the same module.
      $files = array('sites/all/modules/coder/');
      $result = update_remove_extension('ftp_wrapper', $files, $settings);
      $this->assertTrue($result, t('Module removed successfully.'));

I'm sorry, but this doesn't test anything except that the function returned some string. If we're going to require tests for everything and anything, it will just encourage people to write ones which don't actually test anything.

A unit test as I understand it wouldn't connect to an ftp server. It would simply ensure that it could detect if you have FTP support, it would ensure that the basic workflow and conditionals are behaving as expected. Anything to do with connecting to a remote server is not a unit test anymore, because you are testing the whole stack. If we're going to run this type of system test, I'd say the test assertion should at least check to see if the function did its job by connecting manually and looking for the file.

I'd also recommend that instead of returning true / false from the various methods, we should be using proper error handling. This makes testing a lot easier as well because the exception stack is available to figure out what happened instead of some obtuse drupal message (which can be set at the exception handling level in the implementer).

Finally, I would like to propose a rather major shift from the current structure of this patch (which I'm sure will make cwgordon throw darts at my picture) :)

This module cries out for an OOP approach.

A Backend is an abstract class that includes such methods as
->putModule
->putTheme
->deleteTheme
etc

FTPBackend implments (or extends) Backend
has a constructor which takes username, password, host, path and port

FTPWrapper_FTPBackend extends FTPBackend
implements the getting and putting of things based on the private vars set in the parent's constructor

FTPExtension_FTPBackend extends FTPBackend
implemens the getting, putting, deleting, etc using the wrapper's methods.

This will make the system much easier to test, will reduce a lot of the code and IMO make it easier to organize.

I don't want to make such a suggestion and run, so I'm committing my Acquia "Gardening days" (2 every 3 weeks) to this cause.

How do people feel about re factoring what we've got into an object oriented fashion?

Best,
Jacob

Berdir’s picture

Hm, I like the OOP idea. It would also make sense to use Exceptions for the error handling then.

PS: Seems like your recent patch didn't contained the new files.

dww’s picture

A) Yes, patch #90 is missing a bunch of the new files added in previous patches, e.g. #81.

B) I like the OOP idea, too.

C) I quickly skimmed and noticed hook_update_untar(). That's all well and good, but we need to make this part pluggable, too. It's entirely likely that down the road d.o will also be providing .zip files, and there should be a way to specify which ones to fetch and how to unpack them.

D) Agreed that "unit tests" which just test for a string returned by a function are silly. ;)

It's great you're going to dedicate some of your "free" time to work on this, Jacob!

chx’s picture

OOP ? Well, yeah. Just ->putModule and ->putTheme are wrong, there is no need for two of these, they are the same put.

JacobSingh’s picture

I'm just about done with a first draft implementation supporting all the existing connection types.

Also, DirectoryIterators and FTP streams seems like a dream at this point (sadly):
http://bugs.php.net/bug.php?id=31774

I've wasted enough time on that, I'm going to just do the ugly way :(

(expect patch tomorrow).

Jacob

JacobSingh’s picture

FileSize
38.09 KB

Okay, this is not 100%, but chx has generously offered to start reviewing it even though I'm sure it needs a bit of work.

Note:

I went through major CVS pain getting this to work, so I actually did it through SVN and a little trickery, documented at http://drupal.org/patch/create#comment-1677658. If the patch is screwed up, sorry, I'll figure it out and re-roll.

A couple notes on the plugin manager in core project.

I think cwgordon has done a great job building momentum for this, and I think they way he's separated out the issues makes sense.

However, I think that building from the "bottom up" is not the right approach (it rarely is IMO). I'd prefer if we start at the interface level, and get the user experience and workflow down, at which point low level APIs this issue is about will be more clear. To that end, I suggest we get this patch in when it is cleaned up and reviewed so we can then jump straight into a implementing the interface on the update report screen and then revisit these apis once we know wtf we are building.

Some notable differences from the previous patch(es):

  • Renamed "backend" to "connection". I feel like this is still not ideal, but I think it is more illustrative of the object purpose than backend which for most people invokes an Admin's UI. Examples using the "Is a" test SSH is a Connection, FTP is a Connection, etc. More accurate might be Protocol, but it just sounded funny. I know this is a BikeShed discussion, and if someone really thinks backend is more illustrative, whatever, we can put it back.
  • I removed all the untar complications with untar methods, etc. I made a new function in file.inc called "untar" which provides this functionality. We will need to accommodate windows without GNUtar installed, etc at some point, but I'd like to progress on the interfaces first before we dwell on this. I also encourage us to look into existing tar libraries which can do this.

This patch provides the following:

  1. A hook invocation and implementation (hook_update_connections) that tells update the different ways it can connect to the server to move files


    This looks like this:
    
    /**
    * Implementation of hook_update_connections().
    */
    function update_update_connections() {
      $connections = array();
     
      // SSH2 lib connection is only available if the proper PHP extension is
      // installed.
      if (function_exists('ssh2_connect')) {
        $connections['ssh'] = array(
          'title' => t('SSH'),
          'class' => 'SSHConnection',
          'class_file_location' => drupal_get_path('module', 'update') . '/includes/Connection/SSHConnection.php',
        );
      }
      // FTP connection is only available if the proper PHP extension is installed or
      // allow_url_fopen is on.
      if (function_exists('ftp_connect') || ini_get('allow_url_fopen')) {
        $connections['ftp'] = array(
          'title' => t('FTP'),
          'class' => 'FTPConnection',
          'class_file_location' => drupal_get_path('module', 'update') . '/includes/Connection/FTPConnection.php',
        );
      }
      return $connections;
    }
    
  2. For each connection available, there is a Connection class.


    It must implement a few methods for low level file handling:

    copyFile($source, $destination) or copyDirectory($source, $destination)
    rmdir($directory)
    mkdir($directory)
    canCopyDirs()

    The Connection class itself exposes the public methods
    addModule and addTheme

    They handle the logic of whether to copy individual files up, creating directories along the way, or copying directories wholesale (scp).

    This is a template pattern for those up on their design pattern lingo. http://en.wikipedia.org/wiki/Template_method_pattern. It is a pattern which is strongly associated with an IOC approach.

  3. Exception Handling through a custom Excpetion class (ConnectionException)

Demo

A sample implementation of this which works (at least on my box)


I know this will be removed in a final patch for this update API stuff,
but I've included it so people can get a taste.

  1. Install coder
  2. edit the .info file, change 7.x.1 to 7.x.0
  3. your update report should say "not supported"
  4. open settings.php and add the following:
    $conf['update_connection_settings'] = array();
    $conf['update_connection_settings']['ftp']['password'] = 'password';
    $conf['update_connection_settings']['ftp']['username'] = 'jacob';
    $conf['update_connection_settings']['ftp']['host'] = 'localhost';
    $conf['update_preferred_connection'] = 'ftp';
    //you can use ssh too if you'd like
  5. Go to admin/update/download-and-install/coder and it should download the newest coder, remove the old one and replace it with the new one.
  6. Your Milage May Vary While developing, I wiped out my Drupal install a couple times till I got it right.
    Please do this on a safe place where your FTP / SSH user has limited access to write to your home folder, etc.

Testing

As noted above, I don't think a unit test (or any test) should be testing the
actual connection to the target server. Unit tests by definition should
only exercise the code, not ensure that it does what it says.

Since the Connection classes are now very low level interfaces, we can test the
logic separately. That is accomplished with the TestConnection class, and there is a full unit test there to ensure that the logic behind Connection handling functions without actually requiring a connection to a live server.

Next Steps

I'd like to get a review of the architecture and I'm pretty sure there is silly syntax pieces, style and whitespace violations, etc.
As I said, it's not perfect, and I'm trying to take Angie's advice and not be a perfectionist :)
If we can agree that the fundamental implementation of how the connection and untaring code is put together, I suggest we progress into mocking up an interface for updates. As we work on this, we may want to update the API in this patch.

Thanks!
Jacob

JacobSingh’s picture

FileSize
268 bytes

Opps, seems like my tarball didn't make it for the test-assets.

chx’s picture

I will try to find time today to massage this a little bit. The architecture I like.

dww’s picture

The patch removes a few of the recent commits to update module. Please reroll from current HEAD. Thanks!

Also, update_get_release_history() is all wrong. And, it's not even called anywhere in the patch. If anyone needs that info, call update_get_available().

Shouldn't class Connection be an Interface and called ConnectionInterface?

Thanks, this is already looking a lot better!

Cheers,
-Derek

JacobSingh’s picture

"The patch removes a few of the recent commits to update module. Please reroll from current HEAD. Thanks!"
- Sorry, I had a lot of CVS pain getting this together, I must have screwed that up. I wanted to get something up there because chx had time to review, even though I wasn't totally done.

Shouldn't class Connection be an Interface and called ConnectionInterface?

Yeah, I suppose you're right. In python, I would have the abstract functions be marked as such, and have the operating (public) methods be public. PHP doesn't allow this AFAIK. Now that I look, you're right. Should be

class Connection implements ConnectionInterface

Is the best way to go.

I'll look at this some more on Wednesday (my real Acquia community day), I've got to get some Solr work done :)

Thanks for the look,
J

netaustin’s picture

Very cool. Noticed only one thing--untar() seems to be meant to be pluggable, but the function itself only checks the command line "tar" program.

dww’s picture

Assigned: cwgordon7 » Unassigned

Right, untar() is just an initial work-in-progress (as Jacob explained in #97). It needs lots of lovin'. And, it (or something parallel to it) should consider supporting .zip files, too (as I explained in #94C).

Also, it looks like this is clearly a big group effort at this point -- seems a bit misleading to have this assigned to any one person...

chx’s picture

FileSize
18.12 KB

What about this?

chx’s picture

FileSize
23.07 KB

After a discussion with Jacob, I put back the lazy connection, implemented using __get. Also, added back the test, which might not be passing this time.

chx’s picture

About connection settings, because some clarification is needed. Core should be able to pick a sensible default (the order is: ssh2, ftp wrapper, ftp extension and finally ftp-over-sockets which we still need to copy from Joomla!, stay tuned) and at the same time, the user should have some way to override that default. I do not think core should provide an UI for that override, as it should be an extremely rare case. The 'title' is more a documentation than anything else.

chx’s picture

FileSize
0 bytes

Moved untar to system.

chx’s picture

FileSize
23.68 KB

Hey...

dww’s picture

@chx: Thanks, looking even better...

A) The plugable untar is a bit fubar:

A1) system_update_untar() returns an array with "SystemUntar" as the class name, yet that class is never defined. system.untar.inc defines DrupalUntar instead.

A2) I'd rather the names were more meaningful. If "SystemUntar" or "DrupalUntar" is really the "CLI untar" I'd like CLI in the class name, just like we do for the UpdateFooConnection classes. So, perhaps SystemCLIUntar (to help differentiate down the road from SystemLibTarUntar and whatever other untar "backends" we need...

A3) Why is this an "update" hook at all? If core needs plugable untar, and it's being implemented in system module, what's "update" got to do with it? Why isn't this just a (perhaps separate issue) for adding a pluggable untar system to core?

B) I repeat: Also, update_get_release_history() is all wrong. And, it's not even called anywhere in the patch. If anyone needs that info, call update_get_available(). Please don't post another version of this patch with that function. ;)

Thanks,
-Derek

Berdir’s picture

About connection settings, because some clarification is needed. Core should be able to pick a sensible default (the order is: ssh2, ftp wrapper, ftp extension and finally ftp-over-sockets which we still need to copy from Joomla!, stay tuned) and at the same time, the user should have some way to override that default. I do not think core should provide an UI for that override, as it should be an extremely rare case. The 'title' is more a documentation than anything else.

I'm not so sure about this. It is true for all the different FTP backends, we just use the best available solution, and that's it. However, it could be possible that the ssh2 extension is installed but you don't have SSH access to that server. So we need to explain the difference to the user and give him a choice which backend he want's to use. Anyway, that's nothing that needs to be done in that patch.

Patch looks good, I like how simple most things are now due to some OOP.

+ // @TODO do we want to remove the extension first? Maybe -- but usually files
+ // are overwriteable and the possible failures and race conditions are
+ // problematic.

I'd say yes, or how are we going to handle files that are moved/renamed/deleted inside a module? The problematic part is that we might remove something that will needs to be included. For example, if the registry points to a specific file that is moved.

+ * class TestConnction
+ */
+class UpdateTestConnection extends UpdateConnection {

Another "wrong" name...

JacobSingh’s picture

chx and I discussed the patch a lot. He's changed some small things which I feel make it less testable, but on the whole the Connection stuff is (as of #108) more or less the same, but has better naming conventions and cleaner PHP. I'd like to just go forward with it and switch to http://drupal.org/node/395474, getting that interface right.

The only major objection I have is the tar stuff. We talked about this too, and here's my take:

1). I don't see a use case for a pluggable tar system. Tar is a low level function, your system can either do it, or it can't. We have to provide two implementations which should have a common interface: Windows and Posix. There is no need for a hook system, or an Object Interface, certainly not a separate "update" concrete implementation which simply provides a default directory and creates it. My original patch actually does it the way I still think is best.

2). It doesn't belong in update. There are plenty of other times when a developer might want to untar something, and we should just provide this functionality in file.inc or perhaps in system as chx has done, but I don't really know what "system" is for. IIRC, system is really more about the "Drupal" system, calling hooks and admin pages, etc.

3). The PEAR Archive_Tar (which chx is okay using) is a very good cross platform implementation of handling tarring, untarring and introspecting tar files. If we can (license wise), I'd just go with this. If we want to create helper functions (Adapters) or a function to return an instance of Archive_Tar (Factory) we could do this (ala file_tar($file) returns Archive_Tar::factory($file)). Else, just let implementers instantiate it directly because it already has a factory.

4). Assuming we don't use Archive_tar and want to just get this working so we can focus on the important task of actually building interfaces and then revisit this issue after we get the UI down pat, I would create 3 functions:

file_untar($file, $directory);
_file_untar_win($file, $directory); //stub this for now
_file_untar_posix($file, $directory);

And just build in the CLI arguments or using the php native libs or whatever is required to make this happen and deal with the bigger Archive_Tar thing later.

I'm in favor of this approach, because it helps us get this code in, which I think everyone more or less agrees is a good architecture, and certainly good enough for now so we can start focusing on what we're building instead of how it will work.

Tomorrow, I'm going to start in on the update status upgrade UI with some planning and mockups, I would re-roll the patch but honestly, I'd do what I had in the first one in regards to the test, the template method and untar which are the last things to clean up, so until we get some agreement there, I don't want to put them back.

Nice work guys, I think we're really close. I hope we can put this one in the bank this week.

@dww: I'm going to put update_get_release_history() in every patch of mine on any issue, even non Drupal and PHP related and cc you from now on.

Best,
Jacob

Berdir’s picture

Maybe not as part of this patch, but I still think it should be more flexible than just a linux / windows switch.

- I don't think we can/will include Archive_Tar, and PEAR can be hard to install on some configurations, especially if you want to do it properly.
- There are more OS than just Linux/Windows, even though they might be the most common. Some Unix derivates don't provide the gnu tar binary, but instead one that needs to be combined with gzip.
- drupal.org might at some point provide other downloads than just tar.gz, .zip for example. This would need to be combined, so that untar would be become a more generic unpack that would be combined with download, so that it does download a supported archive format. Especially as there are several compression libraries available for php: http://ch2.php.net/manual/en/refs.compression.php, that could handle zip files. And then there is phar...
- open_basedir, disable_functions and/or other things might deny command line access

I'm not saying that we need to handle all that stuff, but we should make it possible (now or in a follow-up issue), that we can extend it later and provide additional solutions for those things.

joshmiller’s picture

Agree with Berdir on #112 regarding the untar methods. Shouldn't it be called "unpack" or something more generic?

Devil's advocate: I will bet you money Drupal will be offering tar.gz for the forseeable future and additionally offering zips for those people that request them.

dww’s picture

I agree on making untar/unpack extendable, though I think that should be another issue entirely. For now, file_untar() with lots of comments and TODO stubs is fine with me.

@joshmiller #113: I'd never take that bet... given that I basically control the keys to that castle, and that's the position I've been advocating for about a year now, that's almost certainly what's going to happen. ;)

Berdir’s picture

See #486558: Add Archive Tar to Drupal. This would solve all my points I think.

michaelfavia’s picture

Subscribing.

int’s picture

done

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
20.3 KB

Here's a reroll using Archive Tar. It also should address dww's points in #109. Sorry if I missed anything or if I messed up this patch, adding directories in CVS is a pain.

chx’s picture

Note that the only reason this passed the bot is that the new update.test is not added to update.info , the test needs serious love it's not updated to my version of connection yet.

cwgordon7’s picture

Here's an updated patch with some update tests, updated connections, removed dead code, and various coding standards fixes.

cwgordon7’s picture

Updated patch with more code comments and better string constructs as per chx's suggestion.

chx’s picture

Status: Needs review » Reviewed & tested by the community

It just feels good to be able to post this comment. That's some clean and documented code there...

dww’s picture

Status: Reviewed & tested by the community » Needs review

On another quick skim, this is pretty horrifying:

  if (!@ssh2_exec($this->connection, 'rm -Rf ' . escapeshellarg("$directory"))) {

Can we have no sanity checks on $directory? Like, it has to at *least* be a subdir of the site's webroot or something? Seems way too dangerous otherwise. Even if not from malice, programmer error (or one poorly written unit test) could completely wipe out an entire server like this. :(

dww’s picture

p.s. I'd also recommend that we require $directory to be an absolute path for this method.

JacobSingh’s picture

@dww: The rm -Rf fear.

Yes, I learned about the registry this way after I nuked my module directory a couple times while testing and could not get my site back no matter what I did.

If you see my original patch, I had the drupal root as a property of the connection for this reason. I didn't really like having it there, but that's what I did. Perhaps the solution is to specify a "jail" of sorts as a param in the connection class and make sure all operations are within this path. So implementation would look like:

$connection = new FtpConnection($host, $username....);
$connection->chroot = '/home/iswhere/the/heartis'

And then because we are using a template pattern (I would remove chx's change to call copyDirectory directly in implementing classes). We can check it in Connection::copyDirectory or whatever it once was. This avoids implementing it in every child class.

Naming conventions in the current patch

while also fixing many of my stupid bugs, and improving the code, chx changed a lot of the naming conventions, etc from my original patch, and while I don't think this is a show stopper, I'd like some rationale as to why we are naming files like this:

connection/update.connection.inc
UpdateConnection
ConnectionException

connection/update.ssh/inc
UpdateSSHConnection extends UpdateConnection

How could any (non-psychic) developer know how to find these classes or what their inheritance pattern is?

In my original patch, I used a standard which is used for pretty much every PHP framework out there, all Java Applications and is also a C++ convention. I believe chx's response on IRC was "Who does that!?"

See the following:

from http://www.possibility.com/Cpp/CppCodingStandard.html#cflayout C++

"Name your files after your classes. I didn't believe this one until I saw it. Why you name a file different than the class? How I am possibly supposed to know what's in the file otherwise? "

from http://framework.zend.com/manual/en/coding-standard.naming-conventions.h... Zend

Class names may only contain alphanumeric characters. Numbers are permitted in class names but are discouraged in most cases. Underscores are only permitted in place of the path separator; the filename "Zend/Db/Table.php" must map to the class name "Zend_Db_Table".

from http://java.sun.com/docs/codeconv/html/CodeConventions.doc2.html#3043: Java

Each Java source file contains a single public class or interface. When private classes and interfaces are associated with a public class, you can put them in the same source file as the public class. The public class should be the first class or interface in the file.

from http://pear.php.net/manual/en/standards.naming.php: PEAR (who we supposedly follow)

Classes should be given descriptive names. Avoid using abbreviations where possible. Class names should always begin with an uppercase letter. The PEAR class hierarchy is also reflected in the class name, each level of the hierarchy separated with a single underscore. Examples of good class names are:
Log Net_Finger HTML_Upload_Error

Ruby
Ruby is a little difference because of the namespace model using Module, but if you look at popular gems like net-ssh and net-scp, you'll see that they too separate class hierarchy into directories, and name the files after the classes. The Ruby standard is lowercase filenames, but the same as the classes. I personally think this is just confusing because if you use StudlyCaps in one place, and lower case in another you just have to adapt something nonsensical like a "_" or "." but whatever, it's still more logical.

.NET (C#)
Don't have a reference handy, but I know that it follows the guideline of classname == filename.

An acceptable answer to this question does not include "that's the Drupal way" or "I like it". There is strong rationale for the pattern implemented by almost every major framework and language in existence; equally compelling arguments should be provided for doing it the way we are.

Where does this stuff sit

I don't wee why this code should be called UpdateConnection or placed in update module actually. Previously, I had the DrupalRoot as a property of the connection to handle the "chroot" kinda issue. But I think we can move this out and just provide a rooted area instead. Then, I propose we move this whole code structure around connecting to servers to includes and just use my original convention of calling the base class Connection.

I could see this being of use to people working on the asset module, media_mover and other multimedia file harvesting / formatting mods.

I can re-roll back to some of the conventions I put in place earlier, but I'd like some consensus here, and especially weigh-in from webchick / dries.

chx’s picture

I am fine with naming the files after the class. However, your original patch had modules/update/includes/Connection/FTPConnection.php for a file. Ouch. Really, three levels of directories? Who does that :) ? That was my problem.

chx’s picture

About the escapeshellarg issue, we can make copyDirectory protected and then let addExtension handle the chrooting.

dww’s picture

Status: Needs review » Needs work

Re: location: This is a patch against modules/update since early on, it was trying to be a generic patch against system that duplicated a bunch of code and constants from update.module. I argued that was dumb, so we moved it. Meanwhile, the code's been basically re-written maybe twice in this issue, and the scope is changing, and now it's this generic Connection class for doing file operations on your Drupal site. So yeah, maybe it should move to includes as part of our file handling (component "file system"?).

Re: filenames: I'd also like to see sanity in layout and filenames before this is committed. Just about anything else can be easily changed after this initially lands, but moving/renaming files in CVS is a pain, so I'd like to see that as close to perfect as possible before this goes in.

Re: jail/chroot/protection: Instantiating a Connection class with a jail path sounds lovely. All operations should enforce that jail. The idea of just making the incredibly dangerous function a protected class member and having callers be smart is still terrifying to me. If we put a function in core that allows for the arbitrary execution of "rm -rf $some_path", I can promise you that someday we're going to live to really regret it.

JacobSingh’s picture

Status: Needs work » Needs review

Actually I had 4 :)

includes/Connection/FTPConnection/FTPWrapper.php

I didn't strictly follow the conventions used in PEAR, Zend, Java(sometimes) and to a large extend C++ (see links above) which would be:

Connection.php
class Conncetion

Connection/FTPConnection.php
class Connection_FTPConnection extends Connection

Connection/FTPConnection/FTPWrapper.php
class Connection_FTPConnection_FTPWrapper extends Connection_FTPConnection

the includes part is just because that seemed to be a newish Drupal convention to bury anything not directly Drupally (no hooks) inside includes.

The rationale is that if there is an incidence of class hierarchy, it should be reflected in the filesystem so that the hierarchy is apparent without having to open up the files. Furthermore, it serves as a namespace differentiator in languages / frameworks which do not have, or do not enforce namespaces.

I personally feel that it is a little overkill. Using Directories to show hierarchy makes it a lot easier for developers to trace, and makes the file organization logical. Naming the classes with underscores to denote hierarchy back to the root can make class names unwieldy. Java uses packages for namepsace declaration so this is format of Parent_Child_GrandChild is not generally used AFAIK. (PEAR does though).

So in the PEAR tree:

Archive/Tar.php
class Archive_Tar
Archive/Zip.php
class Archive_Zip

etc...

Best,
J

chx’s picture

I removed the drupal_root argument because it was a variable. Just how often do you expect it to change? We currently have return $this->copyDirectory($source, DRUPAL_ROOT . '/' . $path); . (needs a realpath or something) If we change copyDirectory to protected... then what else do we want? Change the SSH implementation to verify it is, indeed called with DRUPAL_ROOT? That's reasonable to ask.

chx’s picture

About file names, I knew OOP will get us into trouble. PEAR is everything (bloated, backwards compatible) that Drupal is not.

chx’s picture

Note that DBTNG sits in includes/database/mysql and such. includes/connection/ssh then? Its not too hard to find a file dealing with ssh in a directory called ssh.

Crell’s picture

Java, C++, and C# coding standards for file organization are not really relevant to us. Those are compiled languages. They don't need to worry about the code load/organization issues that we have in a shared-nothing scripting language like PHP. Java and PEAR and Zend use class==>directory-and-file because it makes their loader easy. We already have an easy loader based on the registry so that's a non-issue for us.

Rather, code should be organized in a way that is going to be load-friendly. That's why, for instance, in the DB layer the query builders are broken out into one big file for most of the builders but one separate file for the select builder, which is itself the largest. It's structured along code usage patterns so that we try to balance the code weight/number of files ratio. Putting every class or interface in its own file in the DB layer would have made every update query pull in 5 or so files instead of one, which is bad for performance.

I really don't see a reason to have the entire inheritance hierarchy in the class name, either. Sure there are cases where it can be descriptive, but if you want to know the parent class just look at the first line of the class. It says right there. And what about classes that implement interfaces? How do you flag that in the naming convention?

Make class names self-descriptive and organize based on usage patterns and obvious packages (like all of a given DB driver together in one directory, etc.), not on the class hierarchy. The reasons that some systems do that do not apply to Drupal.

JacobSingh’s picture

Java and PEAR and Zend use class==>directory-and-file because it makes their loader easy. We already have an easy loader based on the registry so that's a non-issue for us.

I don't think this is the case. It is for developer experience. Didn't PEAR had this style long before PHP had an autoloader? The autoloader is quite slow really.

Rather, code should be organized in a way that is going to be load-friendly.

Is Drupal really that efficient that loading 5 files vs. 1 is going to make a difference? Not with modules like views and functions like node_load, not to mention the theming layer. Drupal's biggest problem IMO is user friendliness and developer friendliness. Everyone has the same criticism of Drupal, hard to figure out WTF is going on, doesn't follow any other coding standards, I have to write a ton of SQL, no standardized APIs. The fact that there are 20 Drupal books is not necessarily a good thing. What makes Drupal slow is the DB interface layer and the fact that to show a list of nodes, we are typically running 3-4 queries per node. The registry is great, especially for figuring out what hooks which modules implement, but an opcode cache is the solution to the # of files in your source tree.

I really don't see a reason to have the entire inheritance hierarchy in the class name, either. Sure there are cases where it can be descriptive, but if you want to know the parent class just look at the first line of the class. It says right there. And what about classes that implement interfaces? How do you flag that in the naming convention?

I agree, I don't think that is needed. In fact, I said that in my post. It helps in a massive library like PEAR where namespace is important, but is not needed in Drupal. If you separate by directory, it is clear enough.

Make class names self-descriptive and organize based on usage patterns and obvious packages (like all of a given DB driver together in one directory, etc.), not on the class hierarchy. The reasons that some systems do that do not apply to Drupal.

I'm okay with having them all in one directory, as long as the classnames == filenames. However, as a pattern when you start having lots of classes and hierarchy in them, breaking down by directory makes maintenance and understanding easier.

About file names, I knew OOP will get us into trouble. PEAR is everything (bloated, backwards compatible) that Drupal is not.

No offense, but have you ever used PEAR in a production environment or tuned it to perform? I have, and like anything, performance is far more subtle than # of classes, class hierarchy, etc. Also, saying OOP will get us into trouble is like saying a GUI will get us into trouble. OOP vs non-OOP is not really a debate AFAICT. Everything is OOP, how you do it is relevant, not if. Try to name one prominent language or framework which is not OO in nature. Where is the proof PEAR is evil? It is open source, and it powers many relevant projects. It has flaws, but just because Drupal has a bad DB interface (which Crell is fixing), should I have said it is crap and disregarded it wholesale before I even started using it?

At least PEAR is trying to encourage code reuse and sharing, something the Drupal community is notoriously bad at.

Note that DBTNG sits in includes/database/mysql and such. includes/connection/ssh then? Its not too hard to find a file dealing with ssh in a directory called ssh.

No, in a typical file structure (I'm using typical because I challenge anyone to find an OOP framework of note NOT following the naming structure I explained above), a base class sits in a directory, i.e.

/Connection.php

child classes sit in a directory of the same name

/Connection/SSH.php for instance

if Connection_SSH has child classes, they sit in Connection/SSH/SSHChild.php

You don't just create a dir for every class.

I hope that helps to explain where I'm coming from. I know that ultimately, Dries or webchick is going to decide on this, and I don't want to waste more time on it, I've got other stuff I've got to do, and the filenames are not that important, I just want to bring to light that if almost everyone else is doing something one way, and we are doing it another, maybe we should consider that there is a chance we could be wrong (for once). :)

-J

Crell’s picture

It's been a while, but I don't think CakePHP followed that convention when I last looked at it. (Admittedly it was over 2 years ago.) It's a Rail-style framework so followed that organizational convention with "models", "views", and "controllers" directories.

Regarding code loading performance, that's being discussed to death over here: #345118: Performance: Split .module files by moving hooks. My point is that a literal mapping of class name->inheritance tree->directory structure is actually not that useful for PHP or Drupal, and is not a DX improvement.

catch’s picture

Jacob, don't be dissing node_load() ;)

I personally prefer shallow directory structure - trying to find field modules in modules/field/modules/module_name is bad enough, but equally that shouldn't hold this patch up.

dww’s picture

Status: Needs review » Needs work

We can keep debating the filenames if necessary, but this is definitely still "needs work" for the rm -rf badness I pointed out in #123...

In terms of filenames, I personally think the following makes the most sense:

/includes/
/includes/connection/
/includes/connection/connection.php
/includes/connection/ssh.php
/includes/connection/ftp.php
...

- this isn't necessarily part of update.module after all, so it should just be in the root /includes dir, not /modules/update/...

- no where else in core do we use CamelCase filenames. yes, maybe we should, but if so, we should be consistent. and we should only do a major reshuffling of files once we're no longer using CVS for core.

- this somewhat matches the db layer's file layout.

But, I don't care that strongly... I'm vastly more concerned about the security horrors of rm -rf. And I'd prefer we settled on filenames we can agree on (at least for the next year or so) before we commit. Thanks.

webchick’s picture

chx asked me to comment on the filename issue. The general rule is "Do as Drupal core does. Where Drupal core does not do something yet, do as the wider industry does." Hopefully this rule means that Drupal core always conforms to industry standards, but if not we can always revisit Drupal core decisions in the coding standards group.

What Drupal core does for libraries is:
- /includes for libraries that need to be used across modules.
- if it's a self-contained library, stick it in there directly as foo.inc.
- if it's a library consisting of many multiple files, stick it in a lower-cased sub-directory of includes.

I'm not opposed to changing our naming/folder structure conventions, but if we want to do that we should do that in another issue, since it's ultimately a standards-compliance issue, not a technical one. Though I will probably vote against it because it would create internal inconsistency and confusion about where to find stuff in Drupal. Unlike PEAR, which is OO throughout, we only have one or two OO libraries in core, so it would mean special rules for them that don't apply to everything else, creating a DX problem.

So basically, let's go with:

/includes/
/includes/connection/
/includes/connection/connection.inc
/includes/connection/ssh.inc
/includes/connection/ftp.inc
...
chx’s picture

FileSize
20.31 KB

files moved around and SSH got a protection against rm -rf. It still needs the hooks moved to system but I am out of time.

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
21.66 KB

I moved the hook to system.module. It's hard to add directories in CVS patches, but I'm sure the testing bot will tell me if I did something wrong.

dww’s picture

Status: Needs review » Needs work

A) rmdir() needs the DRUPAL_ROOT check in all implementations, not just ssh.

B) code style:

+        } elseif (is_dir($full_path)) {

Probably more, but it's 2:15am here and I really have no time for a thorough review...

cwgordon7’s picture

Status: Needs work » Needs review
FileSize
22.29 KB

Addressed the feedback in #140 - added the DRUPAL_ROOT check in all implementations and fixed the coding style error.

Dries’s picture

- getParams() should be getParameters().

- rmdir() and mkdir() seem inconsistent; I understand where the names come from but more consistent would deleteDirectory() and createDirectory()

- When reading the API it feels like we're mixing things at different -- there is a lot of stuff about "connections" (low level concept) but also about "extensions" (high level concept). It seems like we're trying to abstract things out, e.g. the files live in a generic "connections" directory instead of, say, a directory called "extension-manager". To make it more pure and more generic, addExtension() should live outside of the class, it seems. Looking at the code, that would be perfectly acceptable. It would make for a better networking component, IMO.

- I've been going back and forth on the name 'UpdateConnection' for a couple days now. The name is slightly odd. I think 'FileTransfer', 'FileTransport' or 'FileUpload' feels slightly more appropriate. It's really about file transfers with different transport mechanisms/layers (i.e. FTP, SSH). The word 'Connection' feels like a misnomer. For example, throw new ConnectionException("Unable to remove to directory @directory", NULL, array('@directory' => $directory)); -- really, it is not a connection error.

dww’s picture

Status: Needs review » Needs work

A) Yeah, it should have been renamed from "UpdateConnection" when it moved to includes.

B) I worry about calling this "FileUpload" since 1) this is about more than uploading files and 2) that's going to be confusing with upload.module (which is a completely different beast). However, I agree that something like "FileTransfer" would be better than "Connection".

C) I agree that addExtension() shouldn't be in this class at all. That can just be a function inside update.module over at #395474: Plugin Manager in Core: Part 2 (integration with update status).

D) If we're going to rename rmdir() and mkdir() to be more consistent (I shed a little tear, but whatever), it makes me wonder if we should add a deleteFile() method to be consistent, too. You can create, copy, and delete directories, but you can only copy files? It doesn't make sense to create a file without copying it, but it certainly seems like someone might need to use this class to remove a specific file from a directory, not the entire directory.

chx’s picture

Status: Needs work » Active
FileSize
12.37 KB

Nope, getParams should be removed, not called anywhere. Then params is gone then the constructor is gone. Really nice and short exception class is the result. I removed the test too it made little sense by now (see Jacob's comments above about the test) and with addExtension gone there is nothing left. So is test.inc is gone too. Did lots of renames as asked for. Added a deleteFile method. Removed the demo too.

catch’s picture

Status: Active » Needs review

Don't think this was supposed to be set to active.

JacobSingh’s picture

I like it!

I still disagree with the kinda arbitrary file naming, and the object hierarchy, but I'm willing to shut up because this looks clean and usable.

One small change we might want to consider:

Using a constant in a class which is not defined in the class is a bad idea generally (DRUPAL_ROOT). This makes the classes's functionality dependent on its caller which in this case is not needed. Also, this code is duplicated in a bunch of places. If we followed the template pattern I outlined in my first patch, it could be centralized... But I don't want to let that hold us up.

Also, I made a few changes to the system_retreive_file function which were hangovers from update and changed some var names.

I threw this into a new issue, because we can commit this independently of the plugin manager stuff, and maybe actually get started on implementing the plugin manager code with more manageable patches :)

I hope no one has an issue with that, I think we've established that this is good "library" type functionality and we can split it off. Nice to get smaller!

See: #499628: Create FileTransfer API to allow for uploading files to remote servers via SSH and FTP

Dries’s picture

Status: Needs review » Fixed

I committed #145 to CVS HEAD so we can move forward. Thanks for the hard work on this. I'm sure there is more to refine, but we can do that in follow-up patches.

JacobSingh’s picture

Dries, did you see the one I split out into another issue?

I'm all for closing this issue out. In fact, I'd like to direct attention to #395474: Plugin Manager in Core: Part 2 (integration with update status) and then revisit APIs once we get the UIs solid, but there is a couple of funny things in the system.module function (like referencing update-cache in a generic function) that I fixed in the other issue.

joshmiller’s picture

Can I just YES!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

This is so exciting... now everyone checkout Jacob's screencast in Part 2 of this patch:

#395474: Plugin Manager in Core: Part 2 (integration with update status)

Frando’s picture

Status: Fixed » Needs review
FileSize
907 bytes

I just read the patch that got committed, and the classes FileTransferFTPWrapper and FileTransferFTPExtension really should extend FileTransferFTP but don't. Attached patch fixes it.

chx’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed #151 to HEAD.

webchick’s picture

Status: Fixed » Needs work

Well this seemingly "simple" two line patch appears to have wreaked havoc with the testing bot.

Rolling this back in an attempt to fix it.

webchick’s picture

Status: Needs work » Needs review
JacobSingh’s picture

@frando:

I totally agree. See my earlier patch which chx has made mince meat out of since.

@webchick
I don't understand what you just committed. The last patch @ 145 is totally broken. Like, busted. See the other issue I created specifically to deal with the file transfer classes.

@chx:

A functional test is not useful here. I am really getting frustrated. My original patch works and is testable and had a test Everything since in the name of "improvement" is broken, and untestable, and now, curiously, committed.

-J

JacobSingh’s picture

FileSize
13.88 KB

The patch committed in #145 is broken in many ways and was probably never even run once by anyone.

Here is an updated version with a unit test. I do not claim this is done, I just have to go have a life now, and this is the best I could manage in the time I had today for this. I've tried the actual connection classes sparingly, I think SSH works, but haven't confirmed. This needs manual testing to eek out any little bugs in the SSH or FTP classes.

I will happily dig into it tomorrow. I suggest we return to a factory method to collapse the two FTP classes together. It just makes life simpler and the UI easier to build. I'm nearly done with part 2 - #395474: Plugin Manager in Core: Part 2 (integration with update status), but because this was so broken, I decided to start a patch first so I could complete the task.

AGAIN: This is not done. It is a work in progress but certainly less broken than what is in CVS now

The unit test should cover most logic. The unit test passes, it still needs to test Exceptions for the jail.

Here is a summary of some of the fixes:

Major
Exception handling was totally broken, passed wrong params in.

constructor was broken for SSH

static var in __get never used

__get called connect every time and assigned a var as a return value which was not implemented. This means it never oculd have worked.

system_retrieve_file had update-cache statically written into it.

Architechtural
Constructor in base class assumes the same settings as children (username, port, host, etc). If we have a dynamic hook, this is not a good idea.

Duplicated code for checking the jail

Extra / improvements

I added a hook to handle the settings form for the default backends. It's not done yet, but it's a start and does no harm. If you want to strip this from the patch, please go ahead, but I will include it in #395474: Plugin Manager in Core: Part 2 (integration with update status)

chx’s picture

I can see we will be long here. I will not comment "mincemeat". Please do not use _ methods when you want a helper method, it looks very weird. I thought I told you why I removed the canCopyDirectories method: first I was thinking "hm, why canCopyDirs is a function? it's extremely unlikely that this is dynamic, sounds like a class constant to me , more like" and then i wrote it that way, added a copyDirectory function and a comment saying "if you override this method change the constant too" and then I was "that's a major wtf. why the heck? let's see whether i can determine whether the copyDirectory is overridden and set a protected var". Now, here you run into a wall because method_exists does not tell you whether the actual class has defined the method, you need the reflection API for that.

Yes I can agree with moving all methods into a protected method, removeDirectoryJail and so on so that the parameter checking is centralized.

This:

  function setJail($jail) {
    $this->jail = $jail;
}

is the worst bloat that OOP offers and it gets into Drupal through my dead, cold body.

Exception param handling is WAY beyond this issue, you need to change _drupal_decode_exception and _drupal_log_error too so that those params actually can reach watchdog.

I am coding this, just wanted to post my review.

chx’s picture

FileSize
17.51 KB

I did not understand why you removed the common constructor just to reintroduce it copypasted everywhere so I have not done that. I have fixed the classes and provided a uniform jailing. The system module parts are yours.

webchick’s picture

Edited comment above to clarify that I tried to commit #151 and subsequently rolled it back.

JacobSingh’s picture

@chx: I notice no comment on all the totally broken pieces of the patch you sent that got committed.
It's really hard to want to work on core issues. I work for an hour and half; take totally broken, untested code and turn it around, actually test it, and actually write a unit test and a description of why I made every change and not even get one "Oh yeah, I guess I sent up some bugs" or "Sorry about that". I just get straight torn into with no desire to understand why somethings may be designed a certain way. </rant>

Anyway, to the issue at hand:

There is a good reason for doing things they way I did. If you ask, I'm happy to explain. I even mentioned in my comment why I removed the abstract class constructor.

If we offer a hook to define filetransfer classes, we assume they might want to behave differently. Implementing a constructor which assumes username, password, port and host is incorrect. If someone is using S3 or SSH with a key or any other option, this would not be desired. It is not duplicating code, it is providing exactly the amount of code which we are sure is relevant to given class. The base class doesn't know about this settings array. In fact, I think a factory (as I made in my original patch) is a much more appropriate place to pass in an array of whatever the filetransfer backend wants, and let a constructor be explicit. I see no reason not to instantiate one or the other FTP class in a factory, it's much cleaner.

Drupal has enough mystery meat "APIs" full of arrays with mystical key names. Put that logic in a factory where it belongs, not in a constructor. The constructor should take real arguments that are well defined and named. This provides validation and testability not to mention DX.

You can remove the mutator for the jail, I'm fine with that. I was assuming you might want to add some validation there. You don't need to pry it from my cold dead hands. :)

Now, here you run into a wall because method_exists does not tell you whether the actual class has defined the method, you need the reflection API for that.

Hmm... Are you sure about that? If you use a protected function like I did, I'm pretty sure it does work, but I didn't test it. Hiding the destructive ops in protected methods still seems like the best architecture to me. If you want to change the _ to something else, it's still best. _ is commonly used for private methods, but I also don't like it. removeDirectoryJailed is kinda silly sounding, but that's fine, I don't really feel like arguing this point as long as the architecture is sound.

This is the reason for a template pattern (see the wikipedia entry I linked 2 weeks ago) When you have to implement functionality irrespective of the child class, it can go in template methods.

Exception param handling is WAY beyond this issue, you need to change _drupal_decode_exception and _drupal_log_error too so that those params actually can reach watchdog.

My patch is better than what is in CVS which throws a fatal error! I think something needs to be in place. If Drupal needs i18n compliance, what I did is the only way to do it. I agree that Drupal should have a base error class which deals with this, but in the meantime, how do you propose to return localized errors with params?

Back into the frying pan friend.

I need a beer and a bed, and not necessarily in that order.

-J

chx’s picture

FileSize
3.92 KB

Some system doxygen are marked for deletion in the patch, i have restored them.

chx’s picture

FileSize
17.32 KB

Ops, crossposted with Jacob. [rant on] In general, I knew this is going to be hard and I am already tiring of fighting OOP purists. The only reason we use OOP is that sometimes it's more appropriate. Most of the time it's something that is incredibly hard to understand and bloated. I cant care less about "templates" and "factories" when most our programmers have never seen a class or an interface to begin with. It's hard enough to understand what we have now without adding even more code[/rant off]

My final classes are actually templates:

In object-oriented programming, first a class is created that provides the basic steps of an algorithm design. These steps are implemented using abstract methods. Later on, subclasses change the abstract methods to implement real actions. Thus the general algorithm is saved in one place but the concrete steps may be changed by the subclasses.

About the constructor, if you do not need the parent constructor dont call it. We have three implementations all using the same code, why not put it in a common function? If someone does not need it, override it, that's why we have OOP.

About factories, yes, core use them actually quite extensively but only when you do not know what class you want. We could add one in the base class much like systemQueue has it, for sure. This, however, has very little to do with constructors. I added it.

I have added some little code to the exception. For sure it's a followup patch.

JacobSingh’s picture

@chx: Looking good, I like, I like. I think we're getting very close. I'll do a full review and test that it is actually working later today.

A couple quick points from looking at the patch:

1. The constructor with the anonymous array of settings is a sticky point. When implementing part 2, I'm running into places where implementing classes are doing a lot of silly logic. Look at the factory pattern in my first patch with an open mind. If it doesn't get in now, I hope it will later as we see the benefit while implementing.

2. The Test will no longer work AFAICT. It was the process of writing this unit test which actually unearthed all the bugs I found in your patch that got committed. I know it looks trivial when it runs, like it must not be testing anything, but the nothing it is testing was pretty broken. Maybe it's okay, it's very lean and mean code now anyway. Should test the jailed behavior though.

3. Embeding Drupal API calls in the class
Most important! This is exactly why I didn't supply DRUPAL_ROOT in the class. If you include calls to Drupal functions / vars inside the class, it is not unit testable. It depends on the rest of the stack. THIS SHOULD BE AVOIDED! Orthogonality is the basis of sustainable architecture. Just because this class will most likely only be used in Drupal doesn't mean it should use resources from Drupal. Another benefit is that future versions of Drupal (or even past ones), never need to worry about changing Drupal APIs. The only "downside" is that implementers must provide the jail path. That's probably a good thing though, otherwise they would have bizzare defaults they didn't realize were being set for them.

This also extends to how we handle error handling:

My original patch (and latest one) works. This is what you've got currently:

class FileTransferException extends Exception {
  function __construct($message, $code, $arguments) {
    // @TODO: make this actually work. Add watchdog right here?
    parent::__construct($message, $code);
  }
 }

I don't think we should use t() because that assumes a certain behavior. Rather, just store the params in a variable, and allow the params and the message to be accessed separately (as in my patch). If an implementer who is handling the error wants to run it through t(), great. If they are using it from some standalone script, or from the installer and don't want to use t(), but have their own sprintf or str_replace, why not? It's clean code, it doesn't care.

JacobSingh’s picture

When trying to run the unit test:

Fatal error: Cannot override final method FileTransfer::createDirectory() in /Users/jacob/work/drupal/7.x/modules/system/filetransfer.test on line 105

Please try to submit patches with updated tests since we have a test now.

JacobSingh’s picture

Regression here:

in the constructor:

+    if (isset($this->hostname)) {
+      $this->hostname = $settings['hostname'];
+    }

Please try to actually run it once. I don't think this would have ever worked.

JacobSingh’s picture

regression:

protected final function checkPath($path) {
    if (realpath(substr($path, 0, strlen($this->jail))) !== $this->jail) {
      throw new FileTransferException('@directory is outside of the @jail', NULL, array('@directory' => $directory, '@jail' => $this->jail));
    }
  }
JacobSingh’s picture

There were a couple other regressions that happened from my patch to the latest update that I fixed. One important one is that the __get method was totally borken, would never have worked even if someone only tested once.

This patch address everything except the exception handling.

The unit test is up to date.

Still going to do some more work testing real connections manually.

I added the factory back, and integrated it into the FTP class. I removed all references to Drupal functions, still works fine.

Please just commit this after a review and some manual testing of real connections. What's in CVS now is totally broken. Part 2 is nearly done (will post something today), and I'd like to use some of these APIs. If Part 2 looks silly in the implementation, we can revisit then. I have to get back to "work".

-J

JacobSingh’s picture

JacobSingh’s picture

Actually, there are still many regressions in there from my last patch and comments.

@chx: please at least look at my patch:

<?php
// $Id: ssh.inc,v 1.1 2009/06/23 12:11:19 dries Exp $

/**
 * The SSH connection class for the update module.
 */
class FileTransferSSH extends FileTransfer {

  function __construct($jail, $username, $password, $hostname = "localhost", $port = 22) {
    parent::__construct($jail, $username, $password, $hostname, $port);
  }

function connect() {
    $connection = @ssh2_connect($setings['hostname'], $this->port);
    if (!$connection) {

  

That was still in there. I don't feel like fixing these things twice. I will though, and post another patch shortly.

-J

JacobSingh’s picture

FileSize
19.58 KB
cwgordon7’s picture

JacobSingh: I deeply admire your patience and perseverance. :) Thank you for working on this patch and also for part 2. I'm similarly near getting a working version of part 3 up, so fixing the underlying code is awesome. I also think you meant to post http://drupal.org/files/issues/filetransfer_cleanup_4_0.patch here but posted to http://drupal.org/node/395474 instead?

JacobSingh’s picture

Doh!

Yes, quite right.

cwgordon7’s picture

Uploaded here and unpublished at the other issue for clarity.

Dries’s picture

Thanks for the persistence, Jacob and chx. It's not always easy to collaborate, but it will ultimately result in better code. I'll make sure to commit a patch later today so we can keep moving forward -- even if it is still not perfect yet.

JacobSingh’s picture

FileSize
19.33 KB

Okay, one more go here.

This fixes the ->connect methods to actually set something.

btw, Just upgraded my first module using the API in this patch over at #395474: Plugin Manager in Core: Part 2 (integration with update status)!

-J

chx’s picture

I give up. Commit whatever, I give up.

chx’s picture

If you wonder why, the factory is back despite my pleas, removals and all. I seemingly cant stop the bloat and I wont repeat all that again. It does not even adhere to coding standards but who cares? It's OOP-theory-pure-correct and that's all that matters isnt it?

chx’s picture

Oh the irony! All that theoritizing and then the system settings form suddenly knows about the ports specific backends use and thats not even in the hook. It's just... there. But, I am not going to reroll , not until Jacob agrees that the factory is needless bloat.

Edit: my factory function followed a pattern we already agreed on... twice. Once for the system queue, second for the cache.inc. And, DBTNG is full with it too. Also, my connect method connected and returned the connection. Usually I would not care that much about the connect method changing $this->connection but once again, in this issue we are so heavy with theory, isnt it more theoretically correct that the method does not know what happens with the connection, it just connects and returns it?

JacobSingh’s picture

It works. I actually tested it. It's not full of bugs. I think that's a good start. I'll finish part two, and you'll see why it makes sense more I think. If not, we can change it. What is in CVS is broken beyond broken, as is every patch you've posted since. I'm sorry, but they plain don't work.

If you change your mind and decide to re-roll, please do the following:

1. Explain your changes.
2. Do not introduce regressions by working on an old patch, use the latest one.
3. Fix the unit test so that it passes or note why it doesn't.
4. Do an actual test instantiating at the very least the SSH class and make sure the operations function as promised.

Before you go on hating Object Oriented programming and design patterns. Try making a project with PEAR, or Java or Django. Read the Gang of Four. More code != worse code. There are valid reasons. Saying something is "bloat" is not a good reason to use / not use a design pattern. I think I go to great lengths to explain exactly why a certain pattern is implemented and increases orthogonality and testability, I don't just say "it's better" or "it is bloat / not bloat".

Berdir’s picture

Why is the latest patch again limited to $hostname/username/password? And why is that configuration form hardcoded? Why not move that to the class, where a sub-class can overwrite it as needed?

Not all protocolls are limited to username/password, the ssh2 extension supports private/public key login for example. Password based ssh login *only* works when the PasswordAuthentication configuration setting is set to on. Note that tools like PuTTy use keyboard-interactive, so that would still work in that case.

# Change to no to disable tunnelled clear text passwords
#PasswordAuthentication yes

In the company I worked, this was always off.

Edit: removed other point - misread that.

JacobSingh’s picture

I agree. I think you have a child class for the SSH class which is SSHWithKey or something, have the factory in the main SSH class decide which one to load depending on what settings come in.

That's why I don't want the base constructor to contain host, user and pass...

The forms are not hardcoded, they will be called from a hook. So there could be flexibility there. The class of course is hardcoded presently.

-J

Berdir’s picture

Another thing, regarding the factory..

First: I do not hate OOP, I used many PEAR packages (In fact, I am a package mantainer, but only a validate package ;)).

But, what do we gain from that factory, as it is implemented?

As an example, MDB2, you do not need to know the classname, you just pass in the connection string and it will return the matching driver:

$connection = MDB2::factory($dsn, $options);

However, in the patch, the factory is a static method that *needs* to be implemented in every sublcass, and we still have to know and call the correct class.

+  static function factory($jail, $settings) {
+    $settings['hostname'] = empty($settings['hostname']) ? 'localhost' : $settings['hostname'];
+    $settings['port'] = empty($settings['port']) ? 22 : $settings['port'];
+    return new FileTransferSSH($jail, $settings['username'], $settings['password'], $settings['hostname'], $settings['port']);
+  }

That's nothing that we can't do in __construct itself. It's a bit different for FTP, because of the two different implementations, that is correct. In DBTNG, we have functions that act similiar to a factory/singleton internally, but a bit hidden, see http://api.drupal.org/api/function/db_select/7 for example. I'm not sure what to pass in and how it would work.. It would help to see your code in #2 I think, because currently, you know more than we do.

+    if (ini_get('allow_url_fopen') == TRUE) {
+      return new FileTransferFTPWrapper($jail, $settings['username'], $settings['password'], $settings['hostname'], $settings['port']);
+    } elseif (function_exists('ftp_connect')) {
+      return new FileTransferFTPExtension($jail, $settings['username'], $settings['password'], $settings['hostname'], $settings['port']);
+    }

Don't we want to prefer the extension, if available?

+  function __construct($jail, $username, $password, $hostname = "localhost", $port = 21) {
+    parent::__construct($jail, $username, $password, $hostname, $port);
+  }

That is just not necessary. We just add default values that we set in the factory anyway. the parent constructor is called automatically, if there isn't one in the current class.

JacobSingh’s picture

@Berdir: Good catches. I agree the factory is a bit suspect, but makes sense for FTP and would make sense for SSH with/without key.

For me, the point is that passing an anonymous array of values to the constructor makes the class unclear and reduces testability. A factory of some sort will exist. We can put it in the system module, or in the class itself. I kinda like it here because if we change the __construct() signature, the factory which access it is in the same file.

re: the default values, I agree it's redundant, I wasn't sure about that because you could instantiate directly too. Sadly, PHP doesn't let you pass NULL values to arguments and get defaults :( I'm okay with removing the defaults from the constructor though.

for the record: I don't like the parent constructor. I don't think it should assume the common connection params, even if it means duplicating that code. It's not code which will change, and those are essentially private vars.

chx’s picture

FileSize
17.93 KB

First,

  protected function removeDirectoryJailed($directory) {
    if (!@ssh2_exec($this->connection, $command)) {

this wont fly, $command is not defined.

I hate OOP because it says more code !== worse code. I hate bloat. That's why I love Drupal. We are lean. We here picked OOP because it produced less code. Life was good. Did I make small mistakes? Yes. We can fix those? Sure. It's easy. Instead you decided to pick up a fight and copy the constructor and added some "factory" bloat. Can we stick to clean up/fix whats necessary and do that fight in a followup issue? It's not easy to pick the latest patch because you have added back something which we definitely disagree upon. Actually it's hard to pick any of your patches because there is so much to undo. You feel like you are repeatedly fixing regression I introduce and I feel like I clean up the code bloat you introduce. So that makes us even. I decided to continue with my patch.

JacobSingh’s picture

Welcome to the doo-cracy of Drupal.

cwgordon7’s picture

Let's try to stop having patch wars, people. You both have legitimate concerns about the patch, and you're both, to some extent, ignoring the other's concerns, I don't want to see us keep switching back and forth on minor issues such as whether we want a factory or not. Chx, not every line of strictly unnecessary code is bloat: if it improves readability/developer experience, then we should completely go for it. JacobSingh, at the same time let's consider whether each change we're making actually does improve developer experience. I am neither for nor against OOP, but would prefer to see cleaner, nicer code, and if that means OOP, then that's awesome. In order for collaboration to work here you both have to be willing to be a bit flexible. I don't think your styles are entirely irreconcilable here, and compromise can go a long way.

On a side note, if anyone wants me to roll out a patch for this, I can work on it tonight.

JacobSingh’s picture

Here is a patch which will work with #395474: Plugin Manager in Core: Part 2 (integration with update status)

I haven't tested FTP yet.

It also includes some other stuff in system for handling the settings forms. May not be appropriate in this patch... Anyway, they are going to go hand in hand.

Used 42 because I've lost count of patches.

chx’s picture

How can I unsubscribe from this issue?

cwgordon7’s picture

Reroll coming shortly. Also, wouldn't it be best to incorporate the setting forms stuff into the classes? Then we could do stuff like:

function settingsForm() {
$form = parent::settingsForm();
$form['port']['#default_value'] = 21;
return $form;
}

Since we already have the classes, it makes the most design sense to me to put the settings forms in the classes, so they're logically found in the place they are to be expected. Assuming no objections, I will incorporate that into my patch.

chx’s picture

Really my last post to this issue and any other plugin manager for that matter. I have dreamed up plugin manager for last summer's SoC, I have worked my ass out to get this issue in while keeping it at a bearable size and complexity. I have made a few small mistakes in the last before commit and my penance is the destroy of all my work by making this overcomplicated and bloated. Do it if you want, just please do not expect me to participate. I am sick of fighting this issue and so many others. I would prefer if I would need to fight core issues but apparently that's not an option. I will help Drupal by sysadmining automated testing v2, writing a few menu patches and more remotely by enhancing php.net. I am sick of core issues.

cwgordon7’s picture

Updated patch, FTP stuff is still completely broken. Some coding standards stuff fixed too, I didn't get a chance to move the form functions into the classes.

JacobSingh’s picture

@chx: I'm sorry you're so frustrated. I've become pretty frustrated as well. This is a larger problem in Drupal. There are only 2 commiters, so it's hard to get finality on any topic, especially when there is a lack of consensus. I don't think it's accurate to say you did all this work, and then someone destroyed your design. I could easily say the same thing. The architechture before my epic patch was very different, and I think most people like what I did more (you as well). I had certain ideas, some survived the war, some did not, but ultimately, we are better for it. It is accurate to say you've been extremely active and responsive and without you, this would not be possible.

I apologize where I may have been overly sensitive, or not sensitive enough. I also don't relish this way of working. Fortunately, once we get this done, we can just work in Contrib and people can easy install and upgrade our modules (joking). If you feel it is best to take a rest from this, cwgordon and I can get it in shape and committed along with ver 2, and then if we need to change a couple method names or use an abstract factory vs an array of params, etc, or include form definitions inside classes, it can be done.

Best,
Jacob

chx’s picture

/me takes a very deep breath and tries to concentrate on the merits of the patch without any prejudice.

  function __construct($jail, $username, $password, $hostname = "localhost", $port = 22) {
    parent::__construct($jail, $username, $password, $hostname, $port);
  }

if you are not doing anything in your constructor then overriding the parent is pointless. Also you can not change the defaults like that -- it will lead to an E_STRICT error. So let's simply leave these out.

I then.. can agree to the factory but only if we take the FTP coupling logic out. Please do not tie things together on this level. If we want to hide from the user, that's fine but please have them separate here.

chx’s picture

also,

   $settings['hostname'] = empty($settings['hostname']) ? 'localhost' : $settings['hostname'];
   $settings['port'] = empty($settings['port']) ? 22 : $settings['port'];

can be shortened to

  $settings += array('port' => 22, 'hostname' => 'localhost');
JacobSingh’s picture

chx, myself and Crell duked this out in IRC, and came up with the following (please correct if I have something wrong).

1. We will separate the FTP classes back to two backends, we will not have logic in the factory to figure out which one to use. This was bad architecture because it assumed we knew how many FTP backends there are and assumes no PHP bugs.

2. We will continue to have factories which will have the signature ($jail, $settings) and will handle setting defaults, etc. The constructors will be fully formed with proper arguments.

3. We will change system_filetransfer_backends to include a weight. The "lightest" weight entry will be the default and we will use FTP for this (either extension or wrapper, not determined yet), this is based on what the user is able to use.

To handle the UX problem of showing users FTP Wrapper vs FTP Extension vs SSH w/ Pass vs SSH w/ key, as opposed to simpler settings, we don't have a proposal yet. The best guess at present is that we will just offer people the FTP form, with a drop down showing all the available backends and try to make the language not silly and overly technical. I personally would like to solve this problem, but we'll cross that bridge / refactor when we get there.

chx’s picture

Yes, we agreed on these and also on having just one base constructor. Notice how nicely we agreed at the end to remove almost all the bloat I was so thrown aback about :) I have recommended using a $conf override for the weight-based default backend but that's not this issue.

YAY WE AGREE!

Berdir’s picture

Wow. Am I dreaming ? ;)

Weight sounds fine, I had that idea too. Regarding which ftp backend should be priorized, maybe we can make some performance tests (in a follow-up issue, of course) to find out which backend is faster/more reliable.

Dries’s picture

*group hug*

dww’s picture

Status: Needs review » Needs work

Been away from core issues for a while, just catching up... wow, what drama. Glad to see everyone's back on the same page again. ;)

Based on the latest comments, #192 (or #185? I've totally lost track with all the warring patches in here) needs to be rerolled to deal with #194-#196...

Bojhan’s picture

So instead of adding 3/4 UI elements on every single follow up patch, lets create one issue which is all about the UI and layout the elements that need to be in any UI. For me this patch is not reviewable on UX.

chx’s picture

Because this patch is not about UX. We merely keep an eye on a possible UI but the actual UI is another patch, see the links on the top.

JacobSingh’s picture

Ugh... FTP related classes had all kinds of bugs in them (variables misnamed, etc).

Time to get out the broom...

Bojhan’s picture

doh..

@chx I confused part 1 from 2! Yup.

JacobSingh’s picture

Okay, there were quite a few regressions in the FTP Extension class as well as an architechtural flaw handling the CopyDirectory operation in FileTransfer.inc

I've tested both of the FTP implementations and the SSH implementation. The unit test also works.

I'm still of the opinion that we should have one FTP option, and figure it out for the user, but I'm tired of fighting. I think we can put this one to bed. This patch is anyway better than what is in core now. And I suggest we commit it (after testing by others) because chx is okay with the design, and I can't be bothered to re-roll endlessly.

Please. Please. Please. If anyone re-rolls this patch:

1. Test every single backend to make sure it can connect to your local machine(quite simple now with the patch over on #395474: Plugin Manager in Core: Part 2 (integration with update status)).
2. Fix the unit test and run it.

JacobSingh’s picture

Status: Needs work » Needs review
FileSize
30.57 KB

Damn, an update came into system.module I didn't update :(

Hopefully no problems. Just posting an updated patch.

chx’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

jrchamp’s picture

Status: Fixed » Needs work

Attempting to visit admin/development/testing results in:

Fatal error: Class 'FileTransfer' not found in modules/simpletest/tests/filetransfer.test on line 133

./modules/simpletest/tests/filetransfer.test:133:class TestFileTransfer extends FileTransfer {

cwgordon7’s picture

Status: Needs work » Postponed (maintainer needs more info)

Clear your cache? Should be autoloaded.

jrchamp’s picture

Status: Postponed (maintainer needs more info) » Fixed

Sorry, clearing the cache resolved the issue.

cwgordon7’s picture

skilip’s picture

Congrats!! This is awesome!!

Berdir’s picture

Status: Fixed » Needs review
FileSize
750 bytes

Sorry for reopening this, but it is *really* minor.

This patch (and also #373201-36: Enhance drupal_render() themeing. Return renderable array on tracker page.) added calls to dd() to core, I assume someone just forgot to remove them again. This leads to the following errors:

Fatal error: Call to undefined function dd() in /home/berdir/workspace/drupal7/modules/simpletest/tests/filetransfer.test on line 122

Unfortunatly, the test bot currently doesn't mark these patches as failed...

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

D'oh! Thanks for catching that...

It's so hard to remember, especially when they are silent.

Oddly, the testing bot doesn't like the patch though...

Berdir’s picture

This is probably because the test assertions after the fatal error are now actually tested and returned, that test class silently died before my change.

I'm getting two fails locally too now

Expected copy files operations made to sites/all/modules Other filetransfer.test 107 FileTranferTest->testCopyDirectory()
Expected copy files operations made to sites/all/modules/fake Other filetransfer.test 122 FileTranferTest->testCopyDirectory()

I don't have time to fix them currently..

JacobSingh’s picture

That's odd that the bot doesn't think an undefined function isn't a fail.

Also odd that they still pass fine on my box. Are they also failing on your box?

Perhaps it has something to do with creating the directory for the fake module in tmp?

deekayen’s picture

The tests on this fail on my local client and break PIFR v2 tests where the site is installed as multisite (instead of sites/all/modules).

class: FileTranferTest
message: Expected copy files operations made to sites/all/modules
function: FileTranferTest->testCopyDirectory()
file: /home/deekayen/web/public/sites/deekayen.no-ip.org/files/checkout/modules/simpletest/tests/filetransfer.test
line: 107

You can see I'm using sites/deekayen.no-ip.org and the simpletests specifically hard-code sites/all/modules, which doesn't even exist on a fresh install (just sites/all).

JacobSingh’s picture

@deekayen: Look at the test. It should have nothing to do with the destination directory, as it doesn't really hit the file system besides to read in a fake module from the tmp dir.

It just tests the logic of copying to a an existing / non-existing folder, and recursive copies. It also tests the jail functionality.

deekayen’s picture

I didn't intimately study the code, but I also didn't manufacture the simpletest failure. I'm setting up another v2 pifr client without the multisite dir to see what happens with it.

catch’s picture

Looks pretty hard coded to me:

 $this->testConnection->shouldIsDirectoryReturnTrue = TRUE;
    $this->testConnection->copyDirectory($directory, "{$drupal_root}/sites/all/modules");
    $expected_commands = array(
      "mkdir {$drupal_root}/sites/all/modules/fake",
      "copyFile {$directory}/fake.info {$drupal_root}/sites/all/modules/fake/fake.info",
      "copyFile {$directory}/fake.module {$drupal_root}/sites/all/modules/fake/fake.module",
      "mkdir {$drupal_root}/sites/all/modules/fake/inc",
      "copyFile {$directory}/inc/fake.inc {$drupal_root}/sites/all/modules/fake/inc/fake.inc",
      "mkdir {$drupal_root}/sites/all/modules/fake/theme",
      "copyFile {$directory}/theme/fake.tpl.php {$drupal_root}/sites/all/modules/fake/theme/fake.tpl.php",
    );
deekayen’s picture

Priority: Normal » Critical
Status: Needs work » Reviewed & tested by the community

Now I'm bumping this to critical because I've reproduced it on two different PIFR v2 install formats in Ubuntu Jaunty and Debian Lenny. This is preventing people from testing patches with PIFR v2 at all. #219 was from Jaunty installed as multisite. Now I also get it on Debian Lenny with PIFR installed in sites/all/modules, and files in sites/default/files.

class: FileTranferTest
status: fail
message: Expected copy files operations made to sites/all/modules
message_group: Other
function: FileTranferTest->testCopyDirectory()
line: 107
/home/drupaltesting/web/public/sites/default/files/checkout/modules/simpletest/tests/filetransfer.test

The specific line it is failing on is

    $this->assertEqual($received_commands, $expected_commands, 'Expected copy files operations made to sites/all/modules');

Locally, I get the dd() mentioned in #214, which prevents me from running simpletests at all on file transfer, so I'm setting back to RTBC to get those dd() calls out, even though it still leaves the patch without resolution to #219/#222.

...and as a side note, the testing class is not spelled correctly (FileTranferTest).

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.52 KB

Ok, the dd() calls cause a fatal error, and thus hide the error in the test itself.

Reordering the expected order of the calls fix the issue. Anyway, that test seems fragile.

Status: Needs review » Needs work

The last submitted patch failed testing.

Paul Natsuo Kishimoto’s picture

Subscribing.

gordon’s picture

subscribe

JacobSingh’s picture

Weird... the patch provided by Demian fails for me on my OSX box (and the bot doesn't like it either). My original one without the dd() is fine for me.

I don't understand 2 things:

1. Why does the testing bot not FAIL when it gets a fatal error? If dd() is not defined, shouldn't it throw an exception and mark it red all over the place?

2. I don't know how we could make this test less "fragile". IMHO, most Drupal tests in core are not unit tests and are way more fragile, because they pretend to be a browser, let 500 systems operate on their data and then assert that drupal_set_message() was called with the right param. Very hard to ensure nothing is happening in the middle. This isn't anyone's fault, it's just that Drupal has low orthogonality. This one is a very basic, straightforward unit test which is only exercising logic and should almost never break, no matter what any other system exists.

It would appear the file recursion is happening in a different order on OSX and Ubuntu. I suspect this is related to: RecursiveIteratorIterator::SELF_FIRST.

Seems that Ubuntu's PHP, it is going alpha through the dir. In OSX, it is going alpha through the files first and then through the dirs. That's causing the failure. Perhaps this is a PHP bug. Maybe it is filesystem related. We have two options:
1. Identify the root cause and file a PHP bug, etc.
2. Hack around the behavior and do a switch based on the OS or something so it still works. It isn't a functional difference AFAICT and only affects the test.

One thing we don't want to do is disregard the order entirely, because it is important to know that directories get created before their children.

Best,
J

catch’s picture

Status: Needs work » Needs review
FileSize
761 bytes

#443154: Fatal errors in tests not reported as failures for fatal errors.

With Damien's patch I get 2 fails and 2 exceptions on Ubuntu.

Keeping the dd() removal but changing the order I get passes (here's patch for reference) - should fail on the test bot since I think it's identical to Berdir's above.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

I had to modify the order on my Ubuntu 9.04 development box (PHP 5.2.6.dfsg.1-3ubuntu4.1).

Strangely #224 and #229 have been tested by the same test slave, and both fail?!?

JacobSingh’s picture

wtf... bad voodoo.

Well, it's more important that it works on Ubuntu than MAMP, but really, I think this is a PHP bug. I am dead sure the test passes as it did when I sent it in on my box, and fails with the patch to switch the order. Would be nice to have another os x user and a windows user weigh in. Although obviously non-critical, it's pretty ugly.

We should get the dd()s out immediately though. Here is a patch which just does that and renames the test (thanks @deekayen).

I recommend we commit this as a start.

Damien Tournoud’s picture

Status: Needs work » Needs review

There is no reason for #232 not to fail. We need to fix the root cause before removing the dd(), or testing will fail all over the place.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

Status: Needs work » Needs review

Yeah, sure I agree that the testing bot should catch exceptions... But isn't that a separate issue from this particular test having an undefined exception in it?

I'm just thinking it would be nice to have this test not fail in that way so we can solve the other issue of PHP not being consistent in the Iterator.

Teasing out how the testing bot deals with errors is not worth leaving this error in core IMO.

Damien Tournoud’s picture

Status: Needs review » Needs work

@Jacob: you don't understand: our development model is based on the assumption that core passes 100% of its tests all the time. If a test fail, core is "broken", and all patches will get rejected by the test bot. We only have two options here:

  • Keep the dd() inside (status quo): those hide the failure in the test, and allow us to continue
  • Remove the test, and wait for it to be fixed before committing it again
JacobSingh’s picture

Okay, so let me get this straight:

1. The testing bot has a bug where it doesn't report errors when it is a fatal exception, but says that it passes.

2. The testing bot will not run for any new tests while current tests are failing

3. By leaving a fatal error in the code, on purpose we can keep the test bot running :)

I'm assuming that isn't desired behavior and I should help out on some issue to fix this, but if I get this right, in the meantime at least, let's do patch where we remove the dd() calls and put the following in:

test_bot_please_die_so_that_patches_may_be_tested()

OR, just put a return statement in to back out and not try to assert the funny one while we sort it out...

Anyway, I suppose the status quo is okay for now, just have to remember... I'll try to hunt down the ordering bug, would be great to get feedback from people on other OSs.

Best,
Jacob

catch’s picture

Lets remove the whole test and put it back later, this could happen for a while and fatal errors in HEAD aren't pretty.

JacobSingh’s picture

I agree.

Could someone else on osx run the test to confirm the same behavior I'm seeing (#232).

I also think that the test is most likely failing on buildbot for another reason because when you order it the way it works for me, or the way it worked for DamZ, et al. it still doesn't work on the bot...

I'm guessing it has something to do with creating the temp directory... Is it possible to get a verbose dump of any exceptions the bot gave?

Best,
j

Damien Tournoud’s picture

Here are the result of manual testing in #29:


First test (actual results):

array (
  0 => 'mkdir /var/www/drupal/other_drupal/sites/all/modules/fake',
  1 => 'copyFile /tmp/fake/fake.module /var/www/drupal/other_drupal/sites/all/modules/fake/fake.module',
  2 => 'copyFile /tmp/fake/fake.info /var/www/drupal/other_drupal/sites/all/modules/fake/fake.info',
  3 => 'mkdir /var/www/drupal/other_drupal/sites/all/modules/fake/theme',
  4 => 'copyFile /tmp/fake/theme/fake.tpl.php /var/www/drupal/other_drupal/sites/all/modules/fake/theme/fake.tpl.php',
  5 => 'mkdir /var/www/drupal/other_drupal/sites/all/modules/fake/inc',
  6 => 'copyFile /tmp/fake/inc/fake.inc /var/www/drupal/other_drupal/sites/all/modules/fake/inc/fake.inc',
)

Second test (actual results):

array (
  0 => 'mkdir /var/www/drupal/other_drupal/sites/all/modules/fake',
  1 => 'copyFile /tmp/fake/fake.module /var/www/drupal/other_drupal/sites/all/modules/fake/fake.module',
  2 => 'copyFile /tmp/fake/fake.info /var/www/drupal/other_drupal/sites/all/modules/fake/fake.info',
  3 => 'mkdir /var/www/drupal/other_drupal/sites/all/modules/fake/theme',
  4 => 'copyFile /tmp/fake/theme/fake.tpl.php /var/www/drupal/other_drupal/sites/all/modules/fake/theme/fake.tpl.php',
  5 => 'mkdir /var/www/drupal/other_drupal/sites/all/modules/fake/inc',
  6 => 'copyFile /tmp/fake/inc/fake.inc /var/www/drupal/other_drupal/sites/all/modules/fake/inc/fake.inc',
)

The difference here is in the order of the file copied. This is on PHP 5.2.6-2ubuntu4.2

catch’s picture

Status: Needs work » Needs review
FileSize
2.64 KB

Here's a revert patch for that test hunk.

cwgordon7’s picture

I'd much rather fix the root cause of the failure than remove the entire test - the test is currently the only thing that calls the plugin manager code (which appears to be broken, at least on my machine). It also currently does not cause tests to fail, so there's no rush for getting a fix in. But if we want to remove the test, we should definitely create a separate issue for its inclusion in core.

On a side note, the tests will always fail on windows because windows uses \ instead of / in file names... also the underlying code appears to be broken, at least on both environments I've tested on (windows xp and kubuntu), but I don't have the energy to debug the code right now, and that might belong in a followup issue in any case.

JacobSingh’s picture

"I'd much rather fix the root cause of the failure than remove the entire test - the test is currently the only thing that calls the plugin manager code (which appears to be broken, at least on my machine). It also currently does not cause tests to fail, so there's no rush for getting a fix in. But if we want to remove the test, we should definitely create a separate issue for its inclusion in core."

Go for it! Just figure out the ordering thing, which seems to change depending on environment

On a side note, the tests will always fail on windows because windows uses \ instead of / in file names... also the underlying code appears to be broken, at least on both environments I've tested on (windows xp and kubuntu), but I don't have the energy to debug the code right now, and that might belong in a followup issue in any case.

Please be more specific. What do you mean by underlying? You submitted a couple patches here earlier which had nearly the same code, did those work for you? I've personally tested every FileTransfer class and the test. All work.

I originally used DIRECTORY_SEPARATOR in my patch, but chx took it out IIRC. I thought he made the right move as PHP for Windows no longer cares...

drewish’s picture

cwgordon7, I'm not on a windows machine right now but I'm 99% certain that JacobSingh is correct, windows will use either path separator.

cwgordon7’s picture

True, but that doesn't matter if you're reading a system filename (with "\" in it) and checking to make sure it's equal to a hard-coded string with "/". Both will find the same file if requested, but PHP will (correctly) not consider the strings equivalent.

catch’s picture

Again, I think we should remove the test and open a new issue to put it back in.

Berdir’s picture

I think I spotted something..

I did run the tests in cli mode and got the following messages:

rm: Entfernen von „/tmp/fake/theme/fake.tpl.php“ nicht möglich: Permission denied
rm: Entfernen von „/tmp/fake/inc/fake.inc“ nicht möglich: Permission denied
rm: Entfernen von „/tmp/fake/fake.info“ nicht möglich: Permission denied
rm: Entfernen von „/tmp/fake/fake.module“ nicht möglich: Permission denied
rm: Entfernen von „/tmp/fake/theme/fake.tpl.php“ nicht möglich: Permission denied
rm: Entfernen von „/tmp/fake/inc/fake.inc“ nicht möglich: Permission denied
rm: Entfernen von „/tmp/fake/fake.info“ nicht möglich: Permission denied
rm: Entfernen von „/tmp/fake/fake.module“ nicht möglich: Permission denied

Looks like a permission problem, not sure if it is related to the failing tests.

boombatower’s picture

Issue tags: +PIFR 2.x blocker

tag

gordon’s picture

@catch agreed. Lets remove this test and get the plugin manager fixed properly. This is blocking more important work with PIFR 2.0 testing.

webchick’s picture

Over at #443154: Fatal errors in tests not reported as failures while trying to get the test bot to flag the fatal error introduced by this patch, we noticed that the test's setUp() function is missing a parent::setUp(). If you add that, does that do anything to help make testing bot happier?

boombatower’s picture

Please make sure we don't forget that again...crazy hard to figure out.

deekayen’s picture

FileSize
986 bytes

On webchick's queue, I added parent::setUp(), removed the dd() debuggers/fatal error makers, and tests passed locally when they didn't before by just removing dd(). Not sure what PIFR 2 will say yet, but we'll just have to see.

Status: Needs review » Needs work

The last submitted patch failed testing.

Berdir’s picture

Hm, is there anything that we can do to avoid missing setUp() calls? Maybe setting a flag and checking that in some code that is always run, tearDown(), or even by the test runner?

gordon’s picture

It should not be an issue once we get in the new life system as it will detect this issue

Gordon

catch’s picture

Back to needs review for #241.

catch’s picture

Status: Needs work » Needs review
deekayen’s picture

I had made a big long post before I decided to roll #252, but it basically boiled down to the following:

9 people are trying to setup PIFR 2 clients on various servers.
Amazon has been putting a lot of effort into attracting testers, servers, and admins.
I'm concerned if this issue continues to prevent clients from trying test patches on d6.drupal.org, we'll loose a lot of momentum Amazon created in hyping up the next test system.

chx recommended patching pifr to work around this bug, but I think that's the wrong solution to the issue.

I'm in favor of catch's #241 on the path to fixing this.

drewish’s picture

catch’s picture

Title: Plugin Manager in Core: Part 1 (backend) » HEAD broken: Plugin Manager in Core: Part 1 (backend)
Status: Needs review » Reviewed & tested by the community

#241 still applies, marking my own patch RTBC so we don't hold up testing.drupal.org indefinitely.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

jrchamp’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.86 KB

Reroll #241 with parent::setup() change from #252.

webchick’s picture

We still need to add that missing parent::setUp(), and ensure testing bot is ok.

webchick’s picture

oops. cross-posted.

deekayen’s picture

#262 with parent::setUp() passed tests.

catch’s picture

FileSize
2.64 KB

cross-posted, please ignore.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok. Committed #262.

We now lack tests for this component, and we need to put that back in a way that works. But it seems like this issue has gone on for awhile, so this could be fixed in a follow-up. Anything else to do or can we mark it fixed?

catch’s picture

Title: HEAD broken: Plugin Manager in Core: Part 1 (backend) » Plugin Manager in Core: Part 1 (backend)

Status: Fixed » Closed (fixed)
Issue tags: -Favorite-of-Dries, -PIFR 2.x blocker

Automatically closed -- issue fixed for 2 weeks with no activity.