Move update manager upgrade process into new authorize.php file (and make it actually work)

No particular objections to #1, but if #371679: Allow modifying forms using the FAPI throughout the entire install process then it might not be necessary?

catch : the entire point is that some of these forms should not be modifiable.

ditto the db details form.

look at #525594: Installation should consist of 2 phases instead of one

@adrian, I mean it might not be necessary to leave code in update.admin.inc as opposed to taking it out.

Having said that, being able to provide a nice interface with a proper menu item etc. for the bulk of the work here, then just the credentials/failsafe in plugin.php sounds like a big win to me.

I tested these patches, but I found an issue where the chmod didn't work, because the directory wasn't created yet.

still looking deeper into it.

Also should note that the drupal_maintenance_theme will need to be updated by the d7ux people to make it all fit together better.

Ok. i've almost got this working on this end.

I've found some caveats tho.

The sites/$site/module and sites/$site/theme directory should be created by ftp if not found.

also, we need to do some twiddling with umask, to allow us to actually set the permissions to 775 on directories, because the default umask on most operating systems is 0022, which means it will only allow you to chmod to 755, which is not enough to do what we need to.

Security wise, I am not too happy with storing things in _SESSION before submitting it to the form, because php code can still modify that and the form will just execute it, which can include sending the ftp password elsewhere.

I have a small bug here that wipes the entire sites/$site directory atm, but i'll fix that and try to get a patch out as soon as i can actually connect to cvs again.

a further wrinkle is setting the correct permissions on the file afterwards.

chmod will only work if the file is owned by a specific user.

chown will only allow you to change the owner of the file if you are root, chgrp will only allow you to change the grp if you are a member of that group.

the web server has neither of these.

This only really takes effect with the sites/$sites/[type] directories.

Taking a look at this now (subscribe).

cleaned up the patch. update.css wasn't applying anymore.

this is the first patch i've rolled with git, so hopefully i didn't mess it up completely =P

@Jacob:

A) Why not just write your code using the Drupal code style in the first place? ;) It's a lot easier and faster than writing everything twice, and saves everyone else time when reviewing your patches so they focus on content, not style (and so other people don't have to redo the work to "port" your code to the code style). Drink the kool-aid already, ok? ;)

B) Why is this patch touching the existing report at all? Seems like scope creep and completely irrelevant to the task at hand. If you want a smaller patch (good goal!), don't add superfluous things to it that don't help the feature you're trying to add. If you want to change the existing report, do that in a separate issue, don't bog this one down with that. include_once('kittens.inc');

C) I haven't had a chance to think about it much, but generally, it seems ok to have the UI to select updates inside update.module, and redirect to plugins.php to do the deed. Seems like a reasonable compromise to me. I agree it's slightly sketchy to pack sensitive goodies into SESSION, but it seems potentially worse to try to handle secure forms without FAPI. I assume there's no way to use FAPI without a full bootstrap, which is why you're heading in this direction, huh?

If a miracle happens and I have some free time this week, I'll try to help out here with a more careful review...

subscribe

reroll of the patch with proper comments at the top and no $Id$ change (and also to send to the test bot). we're working on the patch in the code sprint and more fixes are likely to come.

Anything to be done by those following along at home?

add tag

Subscribe.

... aaand this one fixes all (well, most of, i skipped the 'trailing whitespace') the warnings from coder about in introduced code.

At this point, the patch is mature. It actually does a lot more than what is described in the issue, so I am renaming the issue. With this patch, we are able to install and upgrade modules through the web interface reliably.

* We still have harmless stream warnings when installing/updating modules
* There are some usability problems
* We cannot yet enable modules automaticaly because we don't actually know which module to enable in the project (e.g. "cck" features the "content" module while "audio" is... audio, for that we need #568288: provide a project meta information file first)
* We need to upgrade the right modules
* Finally, when we will We need to fix dependency checking

Those issues, in my opinion, are relatively minor at this point, and should be ignored in the context of this bug at this point, unless we want to keep the patch to grow to a size that becomes completely unmanageable. I would therefore advocate committing this patch so we can move on to other issues with the plugin manager.

Some screenshots would be nice so that anybody (like me) who's unable to do a full review at the moment could at least do a usability review.

I have made a simple test, installing one module from URL, and worked for me.

Uploading as pach for Dreditor

Ahem. :P

Jacob and I sat together at the code sprint and he walked me through the patch as it stands. Just in a cursory look, I can tell this isn't ready yet, but because it adds so many files, it's true that it's extremely difficult to collaborate on, and this is one of our most critical strategical issues.

What I would like is a detailed list of outstanding problems that need fixing, along with cross-referenced issue links, to show that we have a game plan for driving this home. Otherwise I start to get real nervous about momentum staying strong after the 'con. If we don't end up meeting the Oct. 15 deadline for this, then we'll have to roll it back. :\

a detailed list of outstanding problems that need fixing, along with cross-referenced issue links

Isn't that what #536630: [Meta issue] Plugin manager for D7 code freeze spec was supposed to be? That came after the whole multi-part issue set with #395472: Plugin Manager in Core: Part 1 (backend) was declared dead in the water (even though some of the issues are still open), and this issue is one of the phases linked from the "meta-issue." If there were new problems identified at the con, can #536630 be tidied up instead of making a third level of branching from this issue? Or, is everything expected to happen in this patch?

In short, I don't know if it's fair to ask Jacob to both do the patch queue management and keep his mind wrapped around a constantly-changing 50 kB+ patch. Maybe he deserves some help on the former from those aware of the current status.

As a minor comment: when no updates are found the manager reports that "All of your projects are up to date." Since we're using "plugin" as a common word for what modules and themes are, it might be best to replace the word "project" with "plugin" here. In the original version of this module it used to be called an "extension". We don't want to get into the whole taxonomy/category issue again.

The writable files issue is for when someone has the ability to write files due to an exploit in software elsewhere on the same server, uses this to write PHP to the modules directory, then execute the PHP they've just written from there. So I don't think it creates an additional security issue within Drupal itself, but it makes it softer when something else on the server is compromised. afaik anyway.

Some links for the curious:

• Patch that put FTP in WP core
• adrian's review and #395474: Plugin Manager in Core: Part 2 (integration with update status), the issue it was posted on.
• Problems with permissions in WP.

adrian's review has four points in the section where he calls PM "security theatre": two which attack the way FTP credentials are handled, one which points out that SSH2 isn't an option, and one which tries to justify a direct method (which, as you are pointing out, doesn't work). Only the first two are actual criticisms of the current code.

From what I can see, the FTP credentials for WP must be manually entered in the configuration file, and are never exposed or accepted via the admin interface. This is parallel to the way database credentials are treated in Drupal—i.e. used to be "hardcoded" in settings.php, now accepted only by the installer and then made off-limits to anything but a PHP read. As update.php can trust settings.php for database credentials, plugin.php would also trust settings.php for database and FTP credentials.

I think this would address adrian's concerns.

From some of the comments on WP trac issues, developers seem to consider this permissions/ownership muddle to be something that can never be resolved as cleanly as users might hope, simply due to the way web- and ftp servers and PHP operate.

I don't actually agree with the issue assessment here.

And, it can create a usability issue because if it is not 777 or 666, when the user wants to move their site somewhere, they will not have the perms to do so.

Actually, it doesn't need to be 666 or 777 for the user to access it, o+rX (664 on files, 775 on directories) is fine and should be the default "safe" permission.

The potential issues here are two-fold:

1. Compromised Admin account could write to the modules in the plugin directory.
I don't think this is a huge deal, as mentioned above, if they can execute PHP, who cares if they can write code which can be executed later.

That's a "by design" issue with the plugin manager: there's no way to automatically manage the code without raising this issue.

2. On a shared host, having your code be 777 can be a problem if it is not set-up chrooted.
This shouldn't be a problem on most shared hosts, as they are usually intelligently set-up. If it was, then you could say your files directory is not safe from reading (or your settings.php for that matter in many setups). However, it is a not a good practice to give write perms to everyone when you don't need to.

Well, you don't *have* to leave the code 777, as I mentionned above. Furthermore, if a hosting provider allows site A to access site B's files, you have problems, as even if files are not writable, they are readable, which means site A can break site B easily just by reading the DB credentials off site B's settings.php.

Note that I'm not fundamentally against the FTP upload idea, I just think it's suboptimal because it moves so much stuff all over the place. I think by default we should try to chmod and if that fails transfer the files directly.

Also, I'm not sure that putting the creds in the settings.php change or fix anything. Prompting the user is not a problem, and *not* prompting the user will create usability problems, as we would then need to direct users to modify their settings.php.

I have no objections to simplifying the task here to get something working. We can always add more bells and whistles once something is functional and in.

(Side note: The reply at #568288-25: provide a project meta information file didn't answer my technical objections in comment #24, so I still don't know why that request is relevant here. Please answer there if you think that's still needed. Thanks!)

Haven't looked closely at the new patch, but very quick functional test (if you patch -p1 #46 applies cleanly to HEAD) revealed a few problems, some minor, some major:

A) The new default tab at admin/reports/updates "Update" just says "All of your projects are up to date." even though the full update report at admin/reports/updates/full tells me that both my copy of core and cvs_deploy require an update. I'm using cvs_deploy and checking out from CVS, but that shouldn't matter. Both are -dev, which also shouldn't matter.

B) There's help text at the top of admin/reports/updates that belongs with the full report. We might want to add help text to the update tab, but I'd have to see what it looks like when it says I have something to update. ;)

C) When you're at the full report and you "Check manually", you're redirected back to the "Update" tab. Probably we should be using drupal_set_destination(), etc there.

Given that I can't see any of the new UI in action, I can't comment on any of that. I'll try to block out some time for a close review of the patch itself in the near future, if possible.

@Jacob: Thanks for keeping this going!

Cheers,
-Derek

To clarify, the hurdle to jump through for auto-enabling once you install something new would really be #571664: Add a way to specify modules to be enabled by default when a project is installed. But, that seems to imply the plugin manager is installing something new, and I thought weeks ago we jettisoned the idea of having a project browser built in to core via the plugin manager. I was under the impression that all we're doing here is making it possible to update modules and themes you already have. Maybe I'm mis-remembering the plan, if so, apologies. ;) Anyway, let's discuss at #571664 based on clarity on what problem we're actually trying to solve. Thanks!

Do dee doo...

The video looks fantastic, Jacob!

Another very quick review. I still haven't wrapped my head around the whole chmod vs. ftp debate and all the security implications... hope to have time to do so soon, but I'm not sure I will. So, a brief skim of the patch will have to do for now:

A) Most urgently: there's still no killswitch here (at least not that I can see). It refactors the update status menu structure to assume that the task only some people will have permission to use is the default task. That seems like trouble. ;)

B) This is exactly the kind of thing that makes the existing update status API so brain-dead:

function _update_get_recommended_version($name) {
  if ($available = update_get_available(FALSE)) {
    module_load_include('inc', 'update', 'update.compare');
    $project_data = update_calculate_project_data($available);
    $project = $project_data[$name];
    return $project['releases'][$project['recommended']];
  }
}

update_calculate_project_data() shouldn't operate on every project, just so you can turn around and get the info on a specific project you care about. :( Sure, many things are cached, but wow it's RAM intensive to cache all the data for all releases of all projects in a single array that has to be loaded and passed around a lot. update_calculate_project_data() should be calling smaller functions that process a specific project at a time. Nothing is shared across projects, so there's no reason it all has to be (or should be) processed at once. This desperately needs to be cleaned and refactored, but I'd rather not do it as part of this patch -- it should be done over at #238950: Meta: update.module RAM consumption. Maybe we should agree to just leave this lame hack in here (with a @todo comment added in the code) so this patch can move forward, while I work on fixing the underlying API and removing the need for the hack.

C) Re: the existing tests: I answered at #253501-26: Tests for update.module. As far as fixing existing tests with a patch that changes something out from under those tests goes, the hunks in this patch are extremely trivial. I've faced much worse, even in my limited D7 core efforts. ;) You should consider yourself lucky. ;)

- if (!@drupal_mkdir($directory)) {
+ if (!drupal_mkdir($this->connection . $directory)) {

Why did you remove the @?

I have raised my voice against throwing exceptions in another issue as something that needs to be done consistently in Drupal 8 but you are doing them here so then be consistent with t() usage at least: throw new Exception($backend . " error, this type of connection protocol doesn't exist."); needs t() or so I think. But then you want to review the whole patch and the whole code flow to check whether there are t calls in exception throws which happen during the limited bootstrap plugin.php provides. I suspect a $t = get_t() is in order at a number of places. Or just dont use t() in exceptions. As said I did not want to open this can of worms but you wanted to be so OOP to throw these exceptions around...

+ $a = new Archive_Tar(drupal_realpath($local_cache)); <= we dislike variable names like that. Another place uses + $archive_tar = new Archive_Tar(drupal_realpath($file)); i recommend that here as well.

call_user_func_array($available_backends[$method]['class'] . '::factory', array(DRUPAL_ROOT, $settings)); <= oh my. use array($available_backends[$method]['class'], 'factory') for the first parameter.

Sometimes the method / function doxygen has no one line summary, please fix. getUpdaterFromDirectory, findInfoFile, update_get_file, _update_get_recommended_version (the doxygen of this is not professional too), _update_batch_create_message

+ // Has the same name as the directory, is prefered. <= what has?

+ // If the plugin in instlaled in sites/all, we will put it in sites/default <= the word you wanted is installed but WTH this comment means?

+ $this->postUpdate(); one space too many

} catch (FileTransferException $e) { formatting

class ThemeUpdater line has a whitespace error at the end and a comment in the class itself contains a word that I am shocked you submitted it for core inclusion.

What about moving the necessary includes to an install.inc helper function and reusing it here and in install.php? Copypasting 10+ lines makes me frown.

There are likely more things to complain about but i am out of time for now.

Just a quick response to:

a comment in the class itself contains a word that I am shocked you submitted it for core inclusion.

I think it's pretty obvious that that comment is meant to be changed before being committed. Just didn't want Jacob taking heat for no reason.

Is this ought to work on local machines? Can we detect Drupal is run locally and then just download the module instead of going through the FTP steps? When the file system isn't properly configured yet, you should get a warning message @ admin/config/modules/install.

Hm. So I tried to test this tonight by applying this patch and then installing from scratch on DreamHost. Same deal as usual; I've tried to ignore the issue entirely so I can come in fresh, and will review it after. Although I'm fairly tainted at this point since Jacob and I already talked about some stuff at Drupalcon. :)

After initially installing, you can't actually see the "Updates & Add-Ons" menu until you clear your cache. I spent a bunch of time smashing my head thinking the patch was broken. admin/reports/updates just shows the contents of admin/reports.

I then tried to directly access plugin.php, and it tells me:

"Updating your site
It appears you have reached this page in error."

I think this is by design. Looks like I need a session variable of some kind.

Once I cleared the cache and the menu showed up, I went to admin/reports/updates where I got a notice that my projects were up to date and then a huge page of nothingness. Is this a bug, or by design? It certainly feels weird to have a page in Drupal with only 2 sentences on it. If nothing else, I'd love to see some sort of green/red indicator to let me know at a glance if I'm good or in trouble.

Clicking on "Full Update Report" gives me the page I'm used to seeing here:

I can't immediately think of a reason why we would have two tabs here.

Moving on...

The first module I found in the list of modules with 7.x releases with stable releases was DHTML menu. I downloaded 7.x-1.0-alpha1:

Refreshing the page, still nothing. I don't have the module enabled yet.

I went to the settings page at admin/reports/updates/settings and toggled the checkbox for "Check for updates of disabled modules and themes"

Once more I visit admin/reports/updates and... oh hai! There's stuff there now!

I click the button and am taken to a page where it recommends that I back up my site and put my site in maintenance mode:

I click the button again and am taken to a screen in Minelli (I thought we fixed this?) prompting me for my FTP credentials:

1. SSH is not an option. Sad panda. :( Though I guess I'd need SSL to make that actually secure...
2. I have no flipping clue whether I should use "FTP" or "FTP using file streams." Do we absolutely need to offer the user this choice? Can we not just make an educated guess for them based on what's available?

I got stuck here for awhile. Turns out I had disallowed FTP from my SSH user once I learned that the complete effing morons over there were storing my SSH credentials in plain-text. I oh-so-very-temporarily enabled FTP access from my account.

Incidentally, when such a failure to connect occurs, here's what happens:

I would think that if you'd limited it down to it being a username/password problem, there's no need to highlight in red every single field on the form. I originally thought I'd totally blown the whole up, until I realized it was basically just saying "Access denied."

Also, I do not see a place to enter a path? My site is located in /home/XXX/DOMAIN/drupal-7.x-dev, or /DOMAIN/drupal-7.x-dev from the FTP root. Maybe that's why on the next screen I got a WSOD? When I reload index.php, I don't get any dsm()s to tell me what problem it ran into, so unfortunately I'm now stuck.

Anyway, signing off for now since I can't test any further. :) It looks like it'll be really awesome when it works! Let me know if there's something I can do to help debug.

Just had a nice session in IRC trying to get this working. We discovered some interesting failures when the ftp user's umask during the ftp session itself is "too restrictive". My interactive shell umask is 022, but the default umask for the ftp shell appears to be 027 on my osx 10.5.8 box. The result is that plugin.php creates stuff like sites/defaults/modules with perms 750, owned by me. On my site, that can't be read by httpd (which runs as _www, which isn't in the same group as my account).

Basically, the only way any of this can work is if the umask for the ftp user is 022 (or 002, etc -- basically, things just need to be world read/execute by default).

We can just hope that in most hosting environments, that's going to be the default during FTP, anyway, so this is probably a minor thing, but it'd be nice to detect this and somehow gracefully fail. If other folks are trying to test, a workaround is to put umask all 022 into your /etc/ftpd.conf file (or just use that if you don't have such a file already).

Doesn't seem worth blocking commit for this, we can always address this post 10/15 as UX polish.

I've only begun a close review here, but here's a work-in-progress to fix some stuff I found so far:

A) [critical] update_plugin_manager_access() was completely broken and allowed anonymous users to access admin/config/modules/install for example. ;)

B) The hook_help() text for the available updates report wasn't showing up because of the wrong path being used.

C) Started fixing code style bugs in updaters.inc.

D) Jacob pointed out in IRC that updaters.inc was using the wrong exception class name in one spot -- fixed that what I was at it so our patches don't collide.

There's lots more to do here, but it's a start...

Note: Tested #83.

Last night when we were trying to debug this, Jacob informed me that this patch also has the ability to /install/ modules too, which I never would've found myself from this tiny little link on the modules page:

I wonder if it makes sense to move "install" to an "action link" rather than a sub-tab here.

Anyway, when you add a URL, such as http://ftp.drupal.org/files/projects/taxonomy_csv-7.x-4.1.tar.gz, you're directed back to the FTP credential screen. Your password is not remembered, which is great, since that means we're not storing passwords in plaintext. :) If someone wants to, they can use the browser "remember this password" feature to store it.

One minor nit: the button here says "Process updates" even though this is not an update. Maybe should pick a more generic label for that button, such as "Continue" or something.

I meant to test this newer patch to see what it did on DreamHost, and initially got:

However, I am not sure if it was actually related to permissions or not, since the second time I tried it to get the error again, it just...magically... worked. Huh. In any case though, this error gives me absolutely no clue on how to fix it, so we need to make this more clear.

Once you submit the FTP screen, you're taken to a batch progress bar screen. When that's finished, you arrive here:

Looks great... EXCEPT. It didn't work. :( When I click to "enable newly added modules in Taxoomy CSV import", I'm taken to the modules page and it's not listed there.

Digging through the file system, I see that it did in fact download it, but it's in sites/default/modules instead of sites/all/modules (which should be fine, since I'm not running a multisite). Hrmmmm...

Ok. Stuck again. Back to you, Jacob! ;)

@JacobSingh: sounds like progress, thanks. I'm deep in other parts of this right now, so I'll have to re-read your comment to make sure it makes sense. ;)

So, building on #88, this fixes lots of brokenness in plugin.php itself:

A) It used to give a WSOD if you visited and weren't uid 1.

B) It now provides a real 403 access denied page, with the maintenance theme.

C) It now honors the new 'administer software updates' permission: #67234: Update script access rights

D) It now honors the settings.php killswitch, and gives the 403 page for that, even to UID 1.

E) Added a more detailed (and hopefully sane) @file comment.

Also, the last few patches lost the hunk against system.module, which is restored here. Now I'm going to work on fixing the menu items in update.module to honor the killswitch, too (e.g. comment #59).

Awesome work, folks! I'm going to hold off on anymore testing until this gets to the point you guys think it's ready for review. I don't want to hold you up with distractions from testing old patches, etc.

Only night I'm not available for this this week is Tuesday.

#89

An error occurred.
Path: http://drupal7.infonoticias.eu/plugin.php?batch=1&id=2&op=do
Message:
Warning: ftp_mkdir(): /sites/drupal7/sites/default/modules/alt_login: Permission denied in /persistent/home/int/sites/drupal7/includes/filetransfer/ftp.inc on line 145

Notice: Undefined property: ModuleUpdater::$arguments in /persistent/home/int/sites/drupal7/modules/update/updaters.inc on line 267

Catchable fatal error: Argument 2 passed to t() must be an array, null given, called in /persistent/home/int/sites/drupal7/modules/update/updaters.inc on line 267 and defined in /persistent/home/int/sites/drupal7/includes/common.inc on line 1488

Error installing one module from drupal.org by http.

Just pinging the issue to note that dww and I had a discussion on IRC where he asked about some of this UI stuff.

We both independently came to the conclusion that it really doesn't make sense for there to be an interface for upgrading modules in the *reports* section of the website, and that this should move as an action link off the modules page (same as install). This simplifies a number of things, including:

- Reports is what it says on the tin. ;P
- You don't get a big blank screen as your default "welcome to the updates report!"
- admin/reports/updates always shows the same thing for everyone with access to it.
- Doing the killswitch thing can affect plugin.php stuff independently from update.module stuff.

We can liberally cross-reference these two sections of the site with links, as necessary.

Jacob: Ha! We talked about /that/ afterwards, too. :) But I see this as analogous to the permissions page.

If I'm at admin/by-module (or whatever the new path is), and I click "Configure permissions" on "Node" module, I'm taken to the *full* permissions page, but jumped to the place in that list of whatever's most relevant.

So to me, I think it makes sense to link to this from *both* the modules and themes page, and just have some headings or something we can do jump-to links to get to.

I fix 'Permission denied' issue, in the folder sites/default/ i have two folders 'modules' and 'themes' with the wrong permisions (owen by www user, and not by the ftp user [why? maybe one old version of pluginmanger.]).
I remove the two folders to see, if pluginmanager will create them correctly.
Them, I install one module by http, and work's. All folders with correct permission.

So, i test installing 3 modules, and work fine.
I test upgrading one module, work fine.

I test upgrading one module that was in /site/all/modules, and the pluginmanager work's fine, the module was upgraded and work fine, but I became with this:

in /site/all/modules/x was the old version of the module
in /site/default/modules/x was the new version of the module.

Drupal give priority the default folder, so work fine, but it should remove the old directory, or warning the user to remove that folder.

Jacob thinks he found the problem that int reported and fixed some of the error propagation. I folded that change into my copy, so this should handle that failure more gracefully. Our best guess on the reason for the failure was bad permissions, but we'll see.

This patch is a pretty big UI cleanup:

A) admin/reports/updates is back how it's always been, just a report. There's no more crazy with default tabs and such.

B) The plugin update and install pages are now available as action links at 3 locations: admin/reports/updates, admin/config/modules, and admin/appearance.

C) On the update selector page, fixed some bugs introduced by the theme apocalypse patch so that we're once again seeing the table of updates that can't be updated (if any) -- e.g. core, -dev projects, etc.

D) On the update selector page, if update status is reporting about disabled projects, those go into their own tables with headings. The headings only appear if there are multiple tables shown for some reason...

E) There's now a validation callback for the update selector so that if you don't select anything to update, it blows up in your face.

F) If we have a project without a recommended version (e.g. something marked unsupported), we just completely ignore it in the update page (for now, could cleanup this edge case as UX polish down the road).

G) Tons of code and comment cleanups in update.admin.inc, though I'm only about 1/2 of the way through the file.

H) update_plugin_manager_access() from update.module now has PHPDoc.

Also attaching a bunch of screenies of various parts for folks to see.

This #97 are putting the wrong permission.

sites/default/private/temp/

drwxr-xr-x 2 www-data www-data 4096 2009-10-12 13:37 update-cache
drwxr-xr-x 3 www-data www-data 4096 2009-10-12 13:37 update-extraction

have the www permission, and not the ftp permission. So don't work. After I pass the tar.gz url, and click in install, the url change to /plugin.php and show one black page.. without any error in apache/php logs

Ok, i test again the #89 and work well, the problem isn't the permission, because with the #97 have the same permission, and work.

Edit:And I test again with #97, and now allways work.. wired

Realized that we hardly need to change update.test at all, now that all the menu items aren't reorganized there. We can still remove the silly assertion for the "Link to check manually appears", which was hard-coding HTML which is now wrong that we're setting a destination.

And speaking of setting a destination, we were only doing that on the available updates report, not on the update-your-site page(s). I went to go add the drupal_get_destination(), and realized there were about 4 lines of theme function that were completely copy-pasted from theme_update_report(). So, I split those out into a new, shared theme function, theme_update_last_check(). Now, the "Check manually" link consistently sets its destination, and the HTML for this part is no longer duplicated.

So they don't get buried in random comments in here and lost, I've started spinning off follow-up UX/UI bling issues into separate tasks. We'll be standardizing such followup issues with the new Plugin manager issue tag. Here are the first few:

#602484: Fix the report page when authorize.php completes an update manager operation
#602490: Fix help text and action links to find the update manager pages
#602496: Clean up the update manager project selector page when updating your site

Upon closer inspection, I didn't like the hunk against system.module, anyway. ;) It seems wrong to have to have call sites explicitly tell the filetransfer stuff in plugin.inc to not save passwords. We should just test #type == 'password' ourselves and ignore by default. The new logic is as follows:

          // We do *not* want to store passwords in the database, unless the
          // backend explicitly says so via the magic #filetransfer_save form
          // property. Otherwise, we store everything that's not explicitly
          // marked with #filetransfer_save set to FALSE.
          if (!isset($form['connection_settings'][$filetransfer_backend][$key]['#filetransfer_save'])) {
            if ($form['connection_settings'][$filetransfer_backend][$key]['#type'] != 'password') {
              $connection_settings[$key] = $value;
            }
          }
          // The attribute is defined, so only save if set to TRUE.
          elseif ($form['connection_settings'][$filetransfer_backend][$key]['#filetransfer_save']) {
            $connection_settings[$key] = $value;
          }
        }

I thought it was more readable to split it up instead of a massive multi-case if() on one giant line.

So, all that remained from system.module was some code-style cleanup, which I moved to #602570: Fix code style in _system_filetransfer_backend_form_common() since it's unrelated here. $kittens++ ! ;)

New patch:
- plugin.php now clears {cache_update} on success, so that we refetch and recompute to prevent showing bogus data. Having plugin.php complete an update is equivalent to saving the modules page, where we already have code to clear this cache and recompute. Prevents a UX WTF after updating so you don't keep seeing the red warning that your site is out of date.

- Removed the trivial code-style hunk from update.fetch.inc, since that's just going to collide with #597484: Use the Queue API to fetch available update data once that lands.

- Added an @file comment and fixed up a few minor issues in update.js.

TODO:

A) It's a bit of a WTF having includes/plugin.inc which is so tightly intertwined with modules/update code. E.g. update.js is only included via includes/plugin.inc, and that's just the tip of the iceberg. I have a date with Adrian to hash this out, since apparently plugin.inc like this was his idea. I have to do a line-by-line on plugin.inc.

B) It'd be nice to document the changes to ftp.inc in this patch, that's a bit of a DX WTF and could be explained better in the comments.

C) Need to do a line-by-line on modules/update/update.admin.inc

D) Need to do a line-by-line on modules/update/updaters.inc

Otherwise, I've gone through all the rest of this code, and everything is either fixed by this patch, or in followup issues (see http://drupal.org/project/issues/search/drupal?issue_tags=Plugin+manager for those).

go bot

Upon closer inspection, includes/plugin.inc, modules/update/updaters.inc, and modules/update/update.admin.inc are a giant, steamy pile of WTF. ;) I've had a number of fruitful conversations in IRC today about how it really should work. I'm going to get some sleep right now, and when I wake up, I'm going to crank this out. It will be functionally equivalent to what we have now, and the UI shouldn't change at all, but it's not going to be a tangled web of interdependencies, and it'll actually be extensible (e.g. to allow someone in contrib to write an updater that could pull in 3rd party jQuery libraries, or whatever). And, best of all, I've already gotten Dries, webchick, Jacob, Adrian and Crell to agree to the various parts of the strategy. And, it's not going to be adding dozens and dozens of DX bugs and DrupalWTFs to our code...

One aspect is that the stand-alone php script is no longer going to be called "plugin.php", since it basically has nothing to do with that. It's really the "sudo" step in the process, when you authorize the site to update itself and elevate privileges. So, I'm going to rename it to "authorize.php" and we'll have includes/authorize.inc, etc. Jabob, Adrian and I had originally agreed on "sudo.php", and Dries was okay with that earlier, but when I mentioned it just now, webchick threw a small tantrum in IRC about it. ;) So, I suggested "authorize", and there was general agreement (catch, JohnAlbin, and a few others were around at the time and +1'ed).

Anyway, this is my top priority on Tuesday, so I hope to have something posted within 12 hours from now or so... Stay tuned.

subscribing

First of all, I just want to say publicly what I said privately to Jacob, which is that I'm sorry that comment #109 wasn't more clear, and that it could come across as a personal jab at Jacob and the other folks who've been working on this issue. "Steaming pile of WTF" was probably a bit too strongly worded. ;) That said, the code was completely crazy in a lot of ways, but it's not Jacob's fault. This issue (set of issues, really) has gone through so many architectural changes, conflicting pulls, had so many cooks in the kitchen, etc, it's a miracle that Jacob has brought it as far as he has. He's an inspiration in perseverance!

Now that that's been said... here's an update where we're at. This new patch is not yet functioning, but it's very close, and I think it gives enough of the flavor of where it's headed that I'm not ashamed to post it in its currently broken state. ;) Still definitely needs work (see TODO below), but now, with at least 50% less WTF. And, while I keep working, I'd love to get feedback (even just on what I explain in this comment, if you don't have time to read the whole patch) on where it's going.

Summary of changes since #107

- plugin.php is now authorize.php. It doesn't necessarily assume batch API anymore, but it supports it if you want a low-bootstrap place to run a batch through. It also supports displaying a results report page, but again, it doesn't assume you have to. The forms and helper functions for authorize.php are currently in includes/authorize.inc, but only that (no updating-related functions at all). Basically, authorize.php doesn't know crap about anything. All it does is it looks in SESSION for an operation to perform, prompts the user to type their credentials to authorize that operation, instantiates a FileTransfer object with those credentials, and then it invokes the operation, passing in the FileTransfer object (along with whatever optional arguments were in SESSION) to the operation callback. That's it. The rest is up to the operation callback, which is free to setup a batch, or just do something directly, etc.

Note that if we thought it was cool enough UX bling (and therefore, could work on it post 10/15), we could use this mechanism now during the installer to create a safe copy of settings.php instead of prompting the user to do that crap. We could use this for downloading 3rd-party code and non-GPL libraries and such during install without them ever touching d.o, etc. At the very least, we can build stuff like that in contrib now, even if we can't do it in core for D7...

- Added system_run_authorized() to setup an operation to run via authorize.php. It's the shared place to setup the SESSION appropriately, and then it redirects you to authorize.php. One bit of magic here is that the FileTransfer class was written as an extensible system. You can define your own child classes and register those. However, authorize.php intentionally runs at low bootstrap, so calling module_invoke_all() to get the list of classes (like it used to) was pretty pointless with only system.module loaded. ;) So, system_run_authorized() (which runs fully bootstrapped by the thing setting up an operation for authorize.php) invokes that hook, and stashes the array in the SESSION, too, along with the arguments to define the callback. That way, the class autoloader can save the day inside authorize.php and load just the relevant class .inc files for all the FileTransfer classes (not just system.module's implementations), without needing to do a full bootstrap.

- The theming for authorize.php is from includes/theme.maintenance.inc, the CSS lives in modules/system/maintenance.css, and the JS lives in misc/authorize.js, instead of all that being provided by update module.

- The Updater base class and DrupalUpdaterInterface (now named to end with "Interface", as per our conventions) live in includes/updater.inc, not modules/update/updaters.inc. common.inc defines a drupal_get_updaters() registry, which invokes hook_updater_info() and hook_updater_info_alter(). The info hook basically just defines the class names for different classes that implement the DrupalUpdaterInterface interface. system.module implements system_updater_info() to register ModuleUpdater and ThemeUpdater, which both live in modules/system/system.updater.inc. Contrib can now implement other updaters, e.g. for installing 3rd party libraries, translations, whatever.

- All the code for the plugin manager UI in update module now lives in modules/update/update.manager.inc. These are the forms for either updating your site to the latest releases, or installing new modules or themes. I haven't touched any of this UI, just moved the code out of update.admin.inc into this separate file. The submit handlers for these forms sets up some stuff and then calls system_run_authorized() to trigger the authorize.php workflow.

- All the callbacks and functions that actually do things (should) live in modules/update/update.authorize.inc. This will be the only file from all of update module that's loaded by authorize.php when the batch runs, and none of this code needs to be loaded for the UI (or normal functioning of update status).

- So, update.admin.inc is gone. Everything that was in there is now pulled apart into either update.manager.inc (front end) and update.authorize.inc (backend that runs with elevated privs).

TODO

A) It's really weird that if you're installing a new thing, the submit handler (now in update.manager.inc) directly downloads that thing to a temp cache location as the www user, *then* invokes authorize.php to copy it into the sites/default directory as you via a batch operation; instead of the update case, where both the fetching and the copying into place happen as part of the batch operation via authorize.php. It'd simplify a lot of code if everything was always handled via authorize.php and an Updater class. About the only reason I can think of for it being like this is that the UI also supports a file upload to upload a tarball from your local disk, and that wouldn't really work via the Updater stuff. I guess it also allows for easier error reporting if anything goes wrong, without having to get redirected, type your password, etc...

B) There's entirely too much hard-coded logic in update_batch_get_project() and update_batch_copy_project(). Nearly all of those functions should move into the Updater class, both since that's where they make more sense, and so that they can be extended or reimplemented via contrib.

C) There used to be code to try to create sites/default/modules as the authorized user that lived directly in includes/plugin.inc (now includes/authorize.inc). That's completely gone in this patch, but needs to be restored to the workflow, probably as some kind of DrupalUpdaterInterface::initialize() function that the Updater classes all implement.

D) It's *really* evil that the batch operations being invoked via authorize.php are trying to figure out what version of a module to download. :( We already know that in the fully-bootstrapped UI in modules/update/update.manager.inc, so we should just pass that data along when we're defining the batch operations. The thing executing the batches (update.authorize.inc) should be as tiny as possible, basically *just* instantiate the desired Updater object (the class name to use should also be defined by update.manager.inc as part of the arguments to the operation, not figured out when the batch is run) with the right data, and invoke "doYourThing()". ;)

Those are the main items left. But, those are totally do-able, especially now that everything else makes sense and at least everything lives in the right files now.

Weird, my copy of includes/authorize.inc doesn't have a syntax error. :( Is the bot broken? Try this (identical patch, new name).

Re: #111: chx asked for clarification about the TODOs in IRC. I guess I wasn't clear.

A) is explaining that right now, in all these patches for a long time, there's support for two basic manager operations: 1) update existing code that's out of date vs. 2) install some new thing you provide a tarball or URL for. Currently, the install new stuff operation directly gets that code as the www user via the submit handler, *then* hands off control to authorize.php to copy that code into place on the live site with the elevated perms. The update existing stuff operation does both the fetching and then copying into place via the Updater class inside the batch driven by authorize.php. This is the WTF I'd like to clean up.

D) is explaining that it's evil and wrong that in the update-your-existing-stuff operation, the low-level thing that's supposed to perform the upgrade is once again trying to figure out what version you want to upgrade to. That's broken on many levels. Point is, we already know that in update.manager.inc when we provide you the form to verify what you want to upgrade, so we should just save that data, too, and the low level thing can use that, instead of trying to re-figure it out on its own.

Hope that helps...

I had to add this when running an update/upload, otherwise getting a fatal function_not_found:

[13-Oct-2009 21:30:50] PHP Fatal error:  Call to undefined function  update_get_file() in /Applications/MAMP/htdocs/drupal-cvs/modules/update/update.authorize.inc on line 186
[13-Oct-2009 21:41:22] PHP Fatal error:  Call to undefined function  update_untar() in /Applications/MAMP/htdocs/drupal-cvs/modules/update/update.authorize.inc on line 190

function update_batch_copy_project($project, $url, $filetransfer, &$context) {
  // Ensure we have the proper functions.
  module_load_include('inc', 'update', 'update.manager');

Apologies for the lack of a patch.

@agentrickard: thanks. Sorry that wasn't clear, but that's part of what I was talking about in the TODO section. #111 doesn't work at all. ;) Adding that include is just a hack, I don't think we want those functions to live in update.manager.inc at all, IMHO. It seems like they belong in the Updater class.

Which reminds me of another TODO:

E) We should revisit the way in which the Updater system figures out which Updater class is appropriate for updating a given project. The current approach is a bit wonky, although I don't have a great counter-proposal at present.

Anyway, new patch, still broken, but it contains that include so at least you don't get the fatal error.

I also fixed some stupid code from drupal_get_updaters() which Crell pointed out in IRC. It's not only "babysitting broken code" to have that registry function ensure that the info returned from the hooks point to classes that actually implement the Updater interface, calling class_implements() would autoload every class. ;) That was code I had lifted from file_get_stream_wrappers(), which probably should also die as a separate bug fix.

Comments as I read through the patch in #113, focusing on the architecture dww describes in #111 that he and I discussed the other night.

- authorize_run_operation(): This can take $filetransfer as a type hinted parameter, provided there is an Interface that we can type hint for. (Type hinting to a class is a generally bad idea, which is one reason you want to have defined interfaces.)

- drupal_get_updaters() should not do interface checking on classes. That forces an autoload of all classes so that they can be checked, which is unnecessary work/memory usage.

- Most methods of DrupalUpdaterInterface probably shouldn't be static. Just because it doesn't depend on runtime state doesn't mean it shouldn't be a normal method. Static methods are less functional (they do not extend cleanly) and are a few nanoseconds slower than normal methods. Really, they should only be used when they MUST be accessed from outside the context of an object (rare except for factories) or they MUST use data that is shared between all instances of an object.

- Methods should always have their visibilty specified. (See DrupalUpdaterInterface and children.) The default if not specified is public, but that should be stated explicitly anyway for better self-documenting code.

- All exceptions should end in "Exception". This is a very common PHP convention that we are following as well. It's also cleaner to have a separate simple exception class for each type of error rather than to have a single exception for the entire update system that you can only tell apart by the message string. That means you can catch different error types separately. Also, messages to an exception constructor should be translated, I think.

- getUpdaterFromDirectory() is iterating all possible classes, and will therefore autoload all possible classes. This is what dww is talking about in #116 that is still icky.

- getInstallArgs() should not be private. It should be protected. Private methods cannot be called from child classes, making them a code dead-end.

- Why is update() catching FileTransferException only to re-throw a different exception with the same message? If there's a reason for it, it should be commented.

- ModuleUpdater does not need to declare that it implements DrupalUpdaterInterface if it extends Updater. It inherits that implementation flag from the parent class.

- getInstallDirectory() implies that sites/default/modules is where all modules will live. That's wrong. :-) The standard recommendation for where to install new modules has been sites/all since Drupal 5, when sites/all was added. Drush also adds code to sites/all, which means trying to use Drush and this at the same time will lead to modules getting scattered all about the system. We should just pinch code from Drush to see how it decides between sites/all and sites/default and do whatever it does. Don't reinvent the wheel.

- DocBlock for postInstall() and postUpdate() should make it clear that they return not functions to call but instructions for the user.

- There's a big block of comments inside update_batch_copy_project() that uses a docblock comment style. I don't know if that will break the API parser or not. It's still best to use // comments for anything inside a method, IMO.

- On the same train of thought, gah and double gah to unset($filetransfer->connection). That's horrid. Horrid horrid horrid. If you need to skip or re-initialize some properties of an object when it's serialized, PHP provides methods specifically for that. __sleep() and __wakeup(). See:

http://us3.php.net/manual/en/language.oop5.magic.php

If we have to serialize the file transfer object through the batch API and have it re-initialize its connection on the other end, that's the correct way to do it. The code example on that page even looks like what we want here. Exposing a bare property so that we can unset it out from under the object is a horrid cludge. Let's not do that and say we didn't.

I think that's all I've got at the moment. Discussing deeper architectural issues with dww and Jacob in IRC as we speak. :-)

@Paul Kishimoto: I know you're trying to be helpful, but I know this still needs work, even though I'm posting a new patch. ;)

Re Crell #118: "drupal_get_updaters() should not do interface checking on classes."

Right, that was already fixed in #116, since you had mentioned that early in our IRC session. ;)

@Jacob #119: Re: "Yes, this kinda sucks, I guess we need yet another declarative hook for this :( poo."

We already have that hook, it's called hook_updater_info(), and it's invoked by drupal_get_updaters(), as explained in the 4th point of the summary in #111.

That said, I realized I had left a stray update_get_updaters() in update.manager.inc (a hold over from update.admin.inc). That's dead code (yay!) since there's now drupal_get_updaters().

Okay, I need to sleep, but once again, I've cleared my schedule tomorrow to really drive this home. Many thanks to Crell for the design discussions and reviews!

Re the interface, that's why you can make Updater an abstract class. :-) But that's a minor point compared to the deeper issues of code shuffling that need to happen.

FYI for interested parties: #604618: Create a common interface for Archive operations so we can handle .zip, .tar.gz.

Jacob and I just had another fantastic IRC chat. He's writing up the results now. I'm going to start implementing... stay tuned. ;)

WOO HOO! It works. ;)

Not *quite* RTBC yet, but this is getting really close. Of course, the UI still needs all kinds of cleanup in other issues, but the basic plumbing is getting very solid. Everything we've discussed since #111 is done, with the following exceptions:

TODO:

A) #111 (C) is still not done.

B) Most of Crell's goodness from #118 isn't yet folded in (except the thing about drupal_get_updaters() which was done in #116).

C) #604618: Create a common interface for Archive operations so we can handle .zip, .tar.gz.

D) Code style and docs in includes/updaters.inc and modules/system/system.updaters.inc

E) I can't seem to get the upload a tarball directly via a file attach field feature to work on my local install.

Note: you won't be able to see the case where the download step fails and you're sent back to the update selection form with an error unless you apply my patch from #604902: Batch API only returns $success == FALSE for fatal PHP errors and uncaught exceptions. :/

p.s. Batch API is totally fubar see #604902: Batch API only returns $success == FALSE for fatal PHP errors and uncaught exceptions. So, even if you force some kind of failure in the downloading updates batch, you won't see where it sends you back to the first page and prints errors. :( Oh well, we can fix bugs like this later.

Started folding in Crell's concerns...

Done

- All class methods declare their visibility.
- s/UpdaterError/UpdaterException/
- all exception messages are wrapped in t().
- getInstallArgs() is now protected, not private.
- Started improving some of the doc blocks in updater.inc.
- Added an UpdaterFileTransferException that deals with t() issues from FileTransfer exceptions, and to provide metadata to callsites that happen to want to catch these separately.

Not yet addressed:

- authorize_run_operation(): This can take $filetransfer as a type hinted parameter, provided there is an Interface that we can type hint for. (Type hinting to a class is a generally bad idea, which is one reason you want to have defined interfaces.)

There's no Interface in FileTransfer*. #fail. Needs a followup issue.

- ... unset($filetransfer->connection). That's horrid. Horrid horrid horrid.

Agreed. Another case of #file from FileTransfer. Not sure if we want a big "Fix the steaming pile of WTF known as FileTransfer" or a bunch of smaller tasks...

- getInstallDirectory() implies that sites/default/modules is where all modules will live. That's wrong. :-)

I think we all agree, but we don't have time to reopen this whole debate on the eve of the freeze. Needs follow-up. #605272: Use a better directory when installing code via the update manager (documentation followup)

Won't fix:

- getUpdaterFromDirectory() is iterating all possible classes, and will therefore autoload all possible classes.

There's really no alternative. That's called from the factory to figure out what kind of Updater to use to update a given thing. If we're going to have a weighted info hook (now sorted in this patch, btw), and a way to extend this, we need to give every class a chance to say "yeah, I know how to update that thing -- lemme have it!".

Okay, I think that's everything from Crell's #118 review.

Fixed. The action links now have more self-documenting titles, and are context sensitive (e.g. on admin/appearance, they say "Install new theme" and "Update existing themes".

#125 (E) seems to be a red-herring. File uploads work fine for me in FF, and they work fine for Jacob in FF and Safari both, so I don't know what was going on with Safari for me.

So, the blocking TODOs still are down to just these two, IMHO:

- #111 (C)

- theme_authorize_report() appears to be broken. We keep getting:
Notice: Undefined variable: output in /[webroot]/includes/theme.inc on line 899

Fixes #111 (C). Adds Updater::prepareInstallDirectory() and Updater::makeWorldReadable(), and invokes them in the right places inside Updater::install() and Updater::update().

Heh, yeah, knew it'd be something trivial and dumb once we started looking. ;) Thanks for saving me the trouble of tracking it down.

- Fixed the authorize.php report theme bugs.

- Split the batch finish callbacks in update.authorize.inc into separate functions for the update and install cases. I know they look a bit copy+paste, but most of the lines of each function are messages to the user, and those need to be different in the two cases. Plus, batch API doesn't let you pass in your own arguments to your finished callback, so we'd have to use the SESSION to remember what operation we're doing. Anyway, fixed all the messages to actually reflect reality.

- On a successful update, we now clear the update status data so we force a refresh and don't print bogus results.

Todo: NOTHING!

Other than http://drupal.org/project/issues/search/drupal?issue_tags=Plugin+manager and some of the other followup issues referenced above (e.g. FileTransfer cleanup, if possible) this patch now contains an almost completely sane architecture, it's well-commented, core-worthy code, and it actually works!. ;) I think this is now RTBC. And it's not even noon UTC on code-freeze day yet...

The UI is *not* perfect. Please do *not* nitpick, especially if you haven't read the issues with the "Plugin manager" tag.

/me crawls off to bed -- nearly 5am here.

Ran all the new code through a whitespace cleaner, which found a few minor problems...

Keeping up with HEAD since there were a few recent commits.

Note to committers: don't forget the following before you commit:

cvs add authorize.php
cvs add includes/authorize.inc
cvs add includes/updater.inc
cvs add misc/authorize.js
cvs add modules/system/system.updater.inc
cvs add modules/update/update.authorize.inc
cvs add modules/update/update.manager.inc

OK I read through the whole patch. And also tried it out. I've been half-following the near 24-hour irc conversation between dww, JacobSingh and at times Crell and others, and between that and eariler versions of the patch can see how far this has come. While I can't comment on OO correctness, overall things look sane enough and are well documented.

I opened #605344: Docs and code style fixes for update manager for very minor nitpicks, of which there weren't loads and loads, nothing which should hold this up.

Ran through a few things including installing vsftpd locally to try this out, but I ran into:

An error occurred.
Path: http://d7.7/authorize.php?batch=1&id=8&op=do
Message:
Warning: fileperms(): stat failed for /home/catch/www/7/sites/default/modules/dhtml_menu in /home/catch/www/7/includes/updater.inc on line 344

But presumably it works on crappy hosts with ftp set up somewhat properly, instead of my five minute attempt on localhost, which is all that matters.

What I /loved/ was that when I'd not got chmod correct on my sites/default/private/files/temp directory properly, the updater didn't bother sending me to authorize.php at all, this should save some pain and misery. There's a lot of UI tweaking which can happen here, but it's not awful, and glad to see admin/reports/updates left somewhat intact for now - since not every site (probably none of mine) is going to have this enabled at all.

This sounds like a better option to always storing in default to me.

Last patch fixes the bug I had.

@JacobSingh, thanks for keeping this moving forward. A few quick replies:

Re: help text: It's never been my intention that the help messages generated by update.module saying "you're out of date" should send you to the old report if you have access to the new action. I just don't want to hold up this patch while we argue about such UI text. Hence #602490: Fix help text and action links to find the update manager pages.

Re: install location: after 3.5 hours of sleep just now, I can't really comment on #146. I just know that I opened #605272: Use a better directory when installing code via the update manager (documentation followup) last night as the place to sort it out. ;)

+++ includes/authorize.inc
@@ -0,0 +1,232 @@
+  if (empty($_SESSION['authorize_filetransfer_backends'])) {
+    drupal_set_message(t('Unable to continue, no available methods of file transfer'), 'error');
+    return array();
+  }
+  $available_backends = $_SESSION['authorize_filetransfer_backends'];

i think that can be shortened.

+++ includes/authorize.inc
@@ -0,0 +1,232 @@
+  elseif ($authorize_filetransfer_default = variable_get('authorize_filetransfer_default', NULL));
+  else {
+    $authorize_filetransfer_default = key($available_backends);
+  }

I needed to look 3 times before understanding this construction, can't we just give the default value to variable_get() or something.

+++ includes/authorize.inc
@@ -0,0 +1,232 @@
+   * enabled, they will see submit_connection on the first form (whden picking

typo here.

+++ includes/filetransfer/ftp.inc
@@ -70,15 +79,18 @@ class FileTransferFTPWrapper extends FileTransfer {
+   * If the ftp extenstion is available, we will cheat and use it.

we will cheat and use that instead.

+++ modules/system/maintenance.css
@@ -21,3 +21,18 @@
+.connection-settings-update-filetransfer-default-wrapper {

whoa.. could you repeat that class again? ;)

+++ modules/system/maintenance.css
@@ -21,3 +21,18 @@
+#edit-connection-settings-change-connection-type {

just like this one...

+++ modules/update/update.authorize.inc
@@ -0,0 +1,302 @@
+/**
+ * Batch callback for when the authorized update batch is finished.
+ *
+ * This processes the results and stashes them into SESSION such that
+ * authorize.php will render a report. Also responsible for putting the site
+ * back online and clearing the update status cache after a successful update.
+ */
+function update_authorize_update_batch_finished($success, $results) {
...
+/**
+ * Batch callback for when the authorized install batch is finished.
+ *
+ * This processes the results and stashes them into SESSION such that
+ * authorize.php will render a report. Also responsible for putting the site
+ * back online after a successful install if necessary.
+ */
+function update_authorize_install_batch_finished($success, $results) {

update_authorize_install_batch_finished() and update_authorize_update_batch_finished() seem to be having almost the same code, could we optimize that somehow.

+++ modules/update/update.authorize.inc
@@ -0,0 +1,302 @@
+      variable_set('site_offline', FALSE);

we shoudn't turn online when we were already offline before we started the process.

+++ modules/update/update.test
@@ -45,7 +45,6 @@ class UpdateTestHelper extends DrupalWebTestCase {
diff --git sites/default/default.settings.php sites/default/default.settings.php
diff --git sites/default/default.settings.php sites/default/default.settings.php
old mode 100644
new mode 100755

I don't think we should touch default.settings.php in this patch ;)

I'm on crack. Are you, too?

beside these, i don't see any issues, besides a couple of @todos, which'll probaly be fixed in follow-up issues.

Yoink. Taking this for a final spin.

Ok, hit some issues with my final testing, none of them are worth holding this beauty up. But for completeness:

---
admin/config/modules:
- I don't initially see install/update links until I clear cache.

- When I enter in 'askjdhkasd' as the install URL, I get:
* Notice: Undefined index: scheme in update_manager_file_get() (line 650 of /home/.honor/snarkles/webchick.net/drupal-7.x-dev2/modules/update/update.manager.inc).
* Unable to retreive Drupal project from asdjhasdhkasd.
We ought to have some validation there.
--

Test install:
http://ftp.drupal.org/files/projects/taxonomy_csv-7.x-4.5.tar.gz

Having to choose between FTP and FTP using filestreams IMO is totally unacceptable from a UX standpoint, and I consider this a release blocker. (but not a commit blocker)

---

I entered in bunk FTP info, clicked ok, it successfullly said "Cannot login, pleae check FTP user/pass" but then the entire form with FTP connection settings went away until I clicked where the fieldset title should've been.
---

I got confused at the end because it said update was completed and it took me out of site maintenance mode, but yet still said I had updates remaining. A coupe of things here:

1. Ideally, it would run update.php for me. (It did at one time, so I would consider this a bug fix follow-up)
2. If it doesn't, it should at least not take me out of site maitenance mode until I've done so myself.
3. It should only tell me about db updates if there actually are some (bug fix)

---

Other than this, the patch worked flawlessly on DreamHost, no less! I scanned through the code and nothing horrible stood out to me, which is not shocking given the total rockstars that have worked on this. :)

It is therefore with extreme happiness and glee that I have committed this to HEAD!

Thank you all for doing such an amazing job on this patch. This is absolutely core development done right! :D Especially thanks to Jacob for powering through and for Derek for jumping in at the end there to do a WHOLE bunch of heavy-lifting.

This, of all the exception patches, has the biggest potential to open Drupal up to new audiences, IMO. It is eliminating a huge pain point in the maintenance of a Drupal site, and is one of those completely awesome patches that is going to make it physically painful to run an older version of Drupal once people start playing with it. ;)

Thank you, thank you, and thank you again for all of your tremendous work!!!

YAY! ;)

Re: "Having to choose between FTP and FTP using filestreams IMO is totally unacceptable from a UX standpoint, and I consider this a release blocker. (but not a commit blocker)"

#602520: Make FileTransfer form have one option for FTP (ftp_extension prefered).

In that light, note to everyone who comes here to comment about something they don't like, please don't. Instead, start here:

http://drupal.org/project/issues/search/drupal?issue_tags=Update+manager

If there's not already an issue, submit a separate issue and tag it "Update manager".

Thanks!
-Derek

Awesome. Congratulations to all!

@xmacinfo: http://drupal.org/project/issues/search/drupal?issue_tags=Update+manager -- there are already 3 separate issues open for doc fixes. This issue should be put to rest.

Since I doubt anyone's going to pay any attention to this repeated request, I'm just going to lock comments in here.

@all: Want to comment here with a follow-up? Search for related update manager issues, instead! Either comment at the appropriate spin-off issue, or in the rare case that there's not already an issue open, create a new one.

Thanks,
-Derek

Note, since this functionality is now known as the Update manager (see #602582: Change name and description in update.info to reflect update manager functionality), I changed the issue tag from "Plugin manager" to "Update manager", and fixed a few comments above that linked to the issue listing using this tag. Here it is one more time for clarity:

http://drupal.org/project/issues/search/drupal?issue_tags=Update+manager

Thanks,
-Derek

The patch in #146 was missing a js. See #613654: update.manager.js is missing

In Drupal 8 authorize.php has been changed in a way such that all modules are loaded. Probably this was by accident, still it would be great if the original authors could give feedback on the severity of #2344017: All modules loaded in authorize.php.