We want to run the updating and installing code outside of the DRUPAL_BOOTSTRAP_FULL environment so that if a broken contrib module is added during an update, it doesn't make the site un-usable to complete the update.

Also, by putting the interface asking for the FileTransfer password outside of Drupal we mitigate some risk of XSS attacks.

Files: 
CommentFileSizeAuthor
#146 authorize_php-538660-145.patch90.99 KBJacobSingh
Passed: 13838 passes, 0 fails, 0 exceptions View
#146 140-145.diff.txt1.96 KBJacobSingh
#140 authorize_php-538660-140.patch90.06 KBJacobSingh
Passed: 13824 passes, 0 fails, 0 exceptions View
#140 137-140.diff.txt5.53 KBJacobSingh
#137 authorize_php-538660-137.patch93.17 KBdww
Passed: 13961 passes, 0 fails, 0 exceptions View
#136 authorize_php-538660-135.patch93.17 KBdww
Passed: 13934 passes, 0 fails, 0 exceptions View
#135 131_134.diff10.74 KBJacobSingh
Failed: Failed to apply patch. View
#134 authorize_php-538660-134.patch93.18 KBdww
Passed: 13947 passes, 0 fails, 0 exceptions View
#132 authorize_php-538660-132.patch89.88 KBdww
Passed: 13960 passes, 0 fails, 0 exceptions View
#131 authorize_php-538660-131.patch86.97 KBdww
Passed: 13931 passes, 0 fails, 0 exceptions View
#129 authorize_php-538660-129.patch86.6 KBdww
Passed: 13905 passes, 0 fails, 0 exceptions View
#125 authorize_php-538660-125.patch85.6 KBdww
Passed: 13920 passes, 0 fails, 0 exceptions View
#120 authorize_php-538660-120.patch80.22 KBdww
Passed: 13916 passes, 0 fails, 0 exceptions View
#116 authorize_php-538660-116.patch80.64 KBdww
Passed: 13943 passes, 0 fails, 0 exceptions View
#113 authorize_php-538660-113.patch80.71 KBdww
Passed: 13911 passes, 0 fails, 0 exceptions View
#111 authorize_php-538660-111.patch80.71 KBdww
Failed: Invalid PHP syntax in includes/authorize.inc. View
#107 plugin_php-538660-107.patch70.94 KBJacobSingh
Passed: 13921 passes, 0 fails, 0 exceptions View
#105 103_104.diff2.35 KBJacobSingh
Failed: Failed to apply patch. View
#104 plugin_php-538660-104.patch72.53 KBdww
Passed: 13926 passes, 0 fails, 0 exceptions View
#103 plugin_php-538660-103.patch72.98 KBdww
Failed: Failed to apply patch. View
#101 plugin_php-538660-101.patch74.5 KBdww
Failed: Failed to apply patch. View
#97 plugin_php-538660-96.patch76.93 KBdww
Failed: Failed to apply patch. View
#97 plugin_php-538660-96.normal-case-available-updates-report.png195.78 KBdww
#97 plugin_php-538660-96.module-page-action-links.png98.99 KBdww
#97 plugin_php-538660-96.appearance-page-action-links.png71.39 KBdww
#97 plugin_php-538660-96.normal-case-update-page.png31.01 KBdww
#97 plugin_php-538660-96.worst-case-update-page.png175.08 KBdww
#89 plugin_php-538660-89.patch74.04 KBdww
Failed: Failed to apply patch. View
#88 plugin_php-538660-88.patch69.28 KBJacobSingh
Failed: Failed to apply patch. View
#85 plugin_php-538660-85.patch69.09 KBdww
Failed: Failed to apply patch. View
#83 plugin_php-538660-83.patch67.58 KBJacobSingh
Failed: Failed to apply patch. View
#82 plugin_php-538660-82.patch66.66 KBJacobSingh
Failed: Failed to apply patch. View
#75 plugin_php-538660-75.patch65.88 KBJacobSingh
Failed: Failed to apply patch. View
#74 plugin_php-538660-74.patch65.8 KBJacobSingh
Failed: 13632 passes, 9 fails, 0 exceptions View
#69 plugin_php-538660-66.patch64.31 KBJacobSingh
Failed: 13593 passes, 17 fails, 0 exceptions View
#67 plugin_php-538660-66.patch65.67 KBJacobSingh
Failed: Failed to apply patch. View
#65 plugin_php-538660-68.patch63.71 KBJacobSingh
Failed: Failed to apply patch. View
#64 plugin_php-538660-64.patch62.45 KBJoshuaRogers
Failed: 13523 passes, 17 fails, 0 exceptions View
#62 plugin_php-538660-62.patch61.95 KBJacobSingh
Failed: 13568 passes, 17 fails, 0 exceptions View
#57 plugin_php-538660-55.patch61.37 KBJacobSingh
Failed: 13691 passes, 9 fails, 0 exceptions View
#54 plugin_php-538660-52.patch58.3 KBJacobSingh
Failed: 13573 passes, 19 fails, 0 exceptions View
#52 plugin_php-538660-52.patch58.38 KBJacobSingh
Failed: Failed to apply patch. View
#46 plugin_php-538660-45.patch56.8 KBJacobSingh
Failed: Failed to apply patch. View
#45 plugin_php-538660-45.patch32.86 KBJacobSingh
Failed: Failed to apply patch. View
#35 plugin_php-538660-35.patch53.05 KBJacobSingh
Failed: Failed to apply patch. View
#31 plugin_php-538660-31.patch51.8 KBwebchick
Failed: Failed to apply patch. View
#30 plugin_php-538660-28.diff51.8 KBwebchick
Failed: Failed to apply patch. View
#28 plugin_php-538660-28.diff51.8 KBJacobSingh
Failed: Failed to apply patch. View
#26 538660_plugin_manager.patch51.74 KBanarcat
Failed: Failed to apply patch. View
#25 plugin_php-538660-25.diff51.17 KBJacobSingh
Failed: Failed to apply patch. View
#22 plugin_php-538660-20.diff49.55 KBJacobSingh
Failed: Failed to apply patch. View
#21 plugin_php-538660-20.diff49.93 KBJacobSingh
Failed: Failed to apply patch. View
#19 538660_plugin_manager.patch49.35 KBanarcat
Failed: Failed to apply patch. View
#18 plugin_php-538660-18.diff50.77 KBJacobSingh
Failed: Failed to apply patch. View
#17 plugin_php-538660-17.diff49.59 KBJacobSingh
Failed: Failed to apply patch. View
#16 plugin_php-538660-16.diff50.52 KBJacobSingh
Failed: Failed to apply patch. View
#12 plugin_php-538660-10.diff.patch48.06 KBadrian
Failed: Failed to apply patch. View
#10 plugin_php-538660-10.diff48.51 KBJacobSingh
Failed: Failed to apply patch. View
#2 plugin_php-538660-2.diff52.61 KBJacobSingh
Failed: Failed to apply patch. View

Comments

JacobSingh’s picture

I have another idea here.

Since making forms works outside of Drupal is a royal PITA, and both install.php and update.php kinda hack around it. What if we do this:

1. Leave the code in update.admin.inc (and restore update.report.inc for people who want that).

2. All of the screens to select modules, upload modules to install, etc happen here. These are not security conscious operations, and building them here is much easier.

3. Upon submit, instead of running the batch, we store it in a session var and redirect the user off to plugin.php

So plugin.php would:

  • Take a list of operations
  • Display to the user a description of what it is being asked to do
  • If necessary, ask the user for FTP login creds so it can chmod a directory
  • Takes site offlien (if requested) Runs the batch, shows the report, site back online

By doing this, we save ourselves a massive headache, and open up the forms in update to contrib authors, etc to provide value added features there like a backup routine, future functionality like vcs integration perhaps or alternative repos.

In short: plugin.php's purpose would be to just provide a secure environment for entering credentials and a "oh shit" interface to perhaps fix something that blew up (in the future).

Sound good?

Best,
Jacob

JacobSingh’s picture

FileSize
52.61 KB
Failed: Failed to apply patch. View

Okay, this is not in need of review. I'm putting it here so there is a record and a general direction inplace.

This code works (on my machine), if you apply: http://drupal.org/node/528326#comment-1891236

That patch provides a chmod operation to filetransfer classes.

The attached patch basically moves 1/2 of the old Plugin Manager Code from #395474: Plugin Manager in Core: Part 2 (integration with update status) to a new plugin.inc.

It's 1/2 done, it violates in a violent way all style guides and conventions, it is probably broken on your machine, it is a start :)

It also doesn't have a proper page at the end to report success.

Best,
Jacob

catch’s picture

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?

adrian’s picture

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

catch’s picture

@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.

adrian’s picture

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.

adrian’s picture

Status: Active » Needs work

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.

adrian’s picture

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.

JacobSingh’s picture

Chmod

So it sounds like the chmod solution isn't going to work then?
How was WP doing it?
If it isn't working, at least we have the old code. Also, what is the umask issue? Even if the umask is 022, we can still chmod to 777, can't we? I mean, we assume that the FTP user is the same as the owner of the sites/$site/* directories.

In terms of setting the perms back, what is the proposal here then? Should we do it again with FTP?

$SESSION

Currently, the $_SESSION can't be used for harm, because the bit that deal with the password only does one thing (sets the perms). Even if a trojan added some additional tasks, the worst they could do would be to add more PHP code in sites/$site/modules, and since they can already execute code, no big deal.

Isn't this the case?

I also want to make a confirmation screen on plugin.php to say "you are about to do x,y and z" , of course malicious code can modify this, but would be a useful addition IMO.

Looking forward to your patch!

-J

JacobSingh’s picture

FileSize
48.51 KB
Failed: Failed to apply patch. View

This is getting to be a little bit of a pain in the butt because this patch is getting quite big.

It would be nice to try to find some committable part, even temporarily to facilitate breaking the work into smaller issues.

At present, you have to apply the patch for filetransfers cited in #2, and this one.

This new patch provides:

  • a very basic form where the user can upload a .tar.gz or paste in the URL of a .tar.gz and it will be installed. It will not be enabled by default (there is a stub to start this).
  • Some basic dependency checking which may or may not work, but seems to be the right direction
  • As per @dww's request, the old update report is back, although much of the styling is gone - needs work

I didn't mess with plugin.php much as Adrian is working on it. Adrian, if you get this, please integrate your changes into this patch, I imagine there will be no major conflict, I didn't edit plugin.php from the last iteration IIRC.

axyjo’s picture

Taking a look at this now (subscribe).

adrian’s picture

FileSize
48.06 KB
Failed: Failed to apply patch. View

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

dww’s picture

@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...

JacobSingh’s picture

1). Sorry, if we had a real VCS to use, I probably would act differently and just commit small bits constantly. As it is, I am sending up patches to allow for collaboration. Since I have very limited time to work on this, and don't want to hold up anyone else, I push stuff quickly (sometimes too quickly) rather than obsess over perfection, especially at a prototype phase. I don't naturally write perfect Drupal, I don't really like the style, so it doesn't come fluidly, plus I'm just sloppy at first pass. I have actually coder'd it on the plane today, and will post a new patch soon with that.

2). I don't think it effects the report much... If that is included, it wasn't meant to be. However the CSS got a hit, and needs to be resurrected (although I have a suspicion all that custom coloring will look bad in the new admin theme).

3). Yes, that and it opens it up for contrib authors to eventually build a module browsing interface, etc.

Okay, more tomorrow when I am a little more alive.

mattyoung’s picture

subscribe

JacobSingh’s picture

FileSize
50.52 KB
Failed: Failed to apply patch. View

Okay, I'm posting this in a hurry in the spirit of collaboration, not perfection.

I took Adrian's last patch, and I codered stuff, and also spruced up the "install new module" form, and *sorta* unbroke the log on plugin.php

-J

JacobSingh’s picture

FileSize
49.59 KB
Failed: Failed to apply patch. View

Okay, this patch is trying to keep up with the big API changes in the registry, stream wrappers, etc.

It is functional on my machine, but DB updates have been broken due to the new update API (will be fixing soon). Installing modules from URLs and from tarballs works, as does updates.

I recommend trying this:

Go to reports -> Avail updates
Click on Add-ons
Install http://ftp.drupal.org/files/projects/permission_select-7.x-1.0.tar.gz
When done, go to the modules page, and enable it.
Go to the Avail updates page
It should be there, check it off and run the update. (pray).

I know there is a warning at the end about the stream wrapper stuff. Just posting to keep other devs up2date

Best,
Jacob

JacobSingh’s picture

FileSize
50.77 KB
Failed: Failed to apply patch. View

A little better. Mostly working (We think). Also should be able to handle file permissions in Ubuntu.

Throwing some warnings due to stream wrapper changes somewhere. FIND ME at DC and work on this please! If we can get something in today, it will get an exception to code freeze.

anarcat’s picture

Status: Needs work » Needs review
FileSize
49.35 KB
Failed: Failed to apply patch. View

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.

Paul Natsuo Kishimoto’s picture

Anything to be done by those following along at home?

JacobSingh’s picture

FileSize
49.93 KB
Failed: Failed to apply patch. View

I hope this doesn't cause any problems. I'm still getting used to git, but this fixes issues w/ Permissions and chmod + some text improvements

JacobSingh’s picture

FileSize
49.55 KB
Failed: Failed to apply patch. View

Okay, 2nd try, maybe more up2date. I will git it soon.

If anyone wants to collaborate on my github for this issue, it is here: git://github.com/jacobSingh/drupal.git

Ask me for write access if you want it.

int’s picture

add tag

mcrittenden’s picture

Subscribe.

JacobSingh’s picture

FileSize
51.17 KB
Failed: Failed to apply patch. View

Okay, here is a new one. Added a kill switch, moved the menus around.

anarcat’s picture

Title: Move existing plugin manager code outside of update.module to a new plugin.php file » Move plugin manager upgrade process into new plugin.php file (and make it actually work)
FileSize
51.74 KB
Failed: Failed to apply patch. View

... 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.

mcrittenden’s picture

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.

JacobSingh’s picture

FileSize
51.8 KB
Failed: Failed to apply patch. View

Okay, this one fixes a few of the above problems. I believe the file stream warnings are fixed.

It also fixes a couple of problems with database updates.

int’s picture

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

webchick’s picture

FileSize
51.8 KB
Failed: Failed to apply patch. View

Uploading as pach for Dreditor

webchick’s picture

FileSize
51.8 KB
Failed: Failed to apply patch. View

Ahem. :P

webchick’s picture

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. :\

Paul Natsuo Kishimoto’s picture

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.

wmostrey’s picture

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.

JacobSingh’s picture

FileSize
53.05 KB
Failed: Failed to apply patch. View

Wim! Great to hear from you.

I apologize folks, my vacation has started (wife arrived) before I could properly hand this off and make a punch list.

I'm just uploading what I got so it doesn't get lost.

This update makes modules auto-enable, and looks for the $project.project.info file (which of course doesn't exist yet).

Very quick punch list in the 2 minutes I've got (will have sub-issues once we get this patch in).

1. Fix the report at the end of the operation
2. Perhaps go back to using file uploads to make sure everything stays owned by the FTP user, not the webserver. Actually, decide whether a writable module or theme dir is such a bad idea too (I don't think it is, if someone can execute PHP code why do they need to write to a file to execute PHP PHP code?). If we go for writable plugin dir, then is fine as is, but make sure it stays 777 after the write so that the FTP user can move / remove it.
3. API cleanup. It's currently shit, and makes a lot of assumptions about modules and themes being the only types. If I could re-write and not got slammed for it, I would write it OOP, but it's okay for a v1 IMO.
4. Do not provide multiple FTP options in the select box on the filetransfer form. See part 1 #395472: Plugin Manager in Core: Part 1 (backend) for the painful debate about this.
5. Dependency checking on install.
6. Finish off the "kill switch". You can see that in update_menu_alter() I tried to provide one, but to no avail. It works, but with an E_NOTICE, probably a core menu bug.

If I can steal a few minutes, I will update some more, but if anyone else feels called to run with this for the next 12 days, now is your time to shine :D

Best,
Jacob

catch’s picture

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

Well, looks like I got no takers with the post DC slow-down :/

I'll be making a re-roll tomorrow after I haul myself out of a massive pile of email.

JacobSingh’s picture

Okay, was looking at the permissions issue a bit, and this threatens to derail us from our deadline.

Here it is spelled out:

The original PM in core patch used FTP put or SCP to send files up. Adrian proposed in his review to use chmod to change the perms on dirs instead of uploading files via FTP. So we would change the perms on the modules dir to 777, and then copy the module using php. From his examination, this was how wordpress did it.

This seemed a little more fault tolerant, so I coded it up. However, there is a fatal flaw in this approach which he also found after some time.

Note: This is not actually how WP does it as I discovered after trawling through their code for 2 hours today - they use FTP puts

Here is the problem:

Via FTP (as user jacob) I cannot chmod a file created by apache
and as PHP (apache) I cannot chmod a file created by jacob
so, if I install a new module via PM using the chmod approach (not copying the files via FTP)
the new module will be own'd by apache

That's fine, but it is essentially the same as having it be writable by PHP
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.

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.

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.

What do we do about this?

It was a PITA to get chmod in, now it will likely be a PITA to take it back out, however, it seems like the best approach given the circumstances and I think we ought to do it. Does anyone else have an opinion here?

Paul Natsuo Kishimoto’s picture

Some links for the curious:

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.

anarcat’s picture

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.

JacobSingh’s picture

Have to go to sleep, so no time to write much, but consider it more carefully:

If a directory is created by the webserver with 775, it will not be writable by the user. So if they try to mv or rm that dir later, they will not be able to. They can also not chmod it.

int’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

FileSize
32.86 KB
Failed: Failed to apply patch. View

Okay, I'm taking a new direction with this given the amount of time left.

I've decided to switch away from chmod, and also to rebuild the API for updating using a simple Template pattern with a Plugin element. I think this architecture is a hell of a lot easier to read and maintain.

I also removed some functionality:

PM no longer runs update hooks
PM no longer enables modules upon install

I am sad to do this, as I feel it is a big usability issue, but there are just hurdles to get over to get these things working properly that I don't have time to jump.
One such issue is #568288: provide a project meta information file.

And I think these can be add-ons.

To sum, all PM does now is download from d.o. and upload to your server

Sadly, coder is broken with the current HEAD, so I haven't codered yet ( /me ducks).

I think this needs some style cleanup and another UI review, but functionally seems solid to me. I will steal a couple hours tomorrow to put some more into this.

Best,
Jacob

JacobSingh’s picture

Status: Needs work » Needs review
FileSize
56.8 KB
Failed: Failed to apply patch. View

Sorry, git is still kicking my butt.

Here's another.

Status: Needs review » Needs work

The last submitted patch failed testing.

dww’s picture

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

dww’s picture

To clarify, the hurdle to jump through for auto-enabling once you install something new would really be #571664: have 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!

JacobSingh’s picture

Sorry, it's night time for me, not time for a proper reply:

#536630: [Meta issue] Plugin manager for D7 code freeze spec

Should explain some stuff.

See the modules page (new local task to install mods).

JacobSingh’s picture

Status: Needs work » Needs review

hey Derek,

Thank you much for taking a look.

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.

The original plan was not to support dev releases and core is obviously impossible to update from this tool at the moment (although it's on my plate for D8).

I've just modified this code a bit, so it supports dev -> stable updates and it will show core and dev updates in a new table at the bottom entitled "manual updates". I'm not thrilled with this, but it sorta solves what you are talking about.

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. ;)

Add a module or two (permission_select is a good one as it has 2 stable 7.x releases).

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.

Fixed this.

"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. ;)"

Yeah, see the meta-issue. If PM doesn't install modules and themes, it kinda defeats the purpose (except perhaps for install profile users), because to get the plugins on your system, you need to know FTP :)

Dries's concept for 1.0 was a simple form to upload a tar.gz or provide a URL. This is implemented. See in admin/config/modules or admin/appearance. There is a new Install link which goes there. Because we only moved the credential stuff off to plugin.php, a contrib author can later build a module / theme builder in.

Okay, hope that addresses what are referring to, I'm going to post a screencast and a new patch in a follow up (hopefully in a couple hours).

Best,
Jacob

JacobSingh’s picture

FileSize
58.38 KB
Failed: Failed to apply patch. View

Okay, here's another go. I don't like the solution for manual updates.

I'm thinking it might be better to just say:

"There are manual updates required to make your Drupal site up to date. See the full update report for details."

Anyway, here is a video of the current workflow and a new patch.

http://www.vimeo.com/6848678

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

FileSize
58.3 KB
Failed: 13573 passes, 19 fails, 0 exceptions View

Sorry, git prefixes still there.

Paul Natsuo Kishimoto’s picture

Status: Needs work » Needs review

Do dee doo...

The video looks fantastic, Jacob!

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

FileSize
61.37 KB
Failed: 13691 passes, 9 fails, 0 exceptions View

Okay, now I've added a couple modifications from the tests at

http://drupal.org/node/253501#comment-2106598

To make this pass.

I'm really not happy with this solution because those tests are not what I would consider unit tests, and really shouldn't be related to this patch, but since I'm changing some urls a bit, it causes this problem.

I removed the check for the "Check Manually" link, because:

A). It's kinda useless as a test IMO.
B). It breaks when you use drupal_get_destination() which you need to us in this case.

JacobSingh’s picture

Status: Needs work » Needs review
dww’s picture

Status: Needs review » Needs work

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. ;)

JacobSingh’s picture

Status: Needs work » Needs review

Hi Derek,

Again, many thanks for your review. We'll need you and a lot more people interested if we're going to pull this off.

"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. ;)"

There is in fact:

variable_set('plugin_manager_enabled', FALSE);

Should remove the install pages from the menu, and remove the checkboxes from the page. See the form building code and update_plugin_manager_access(). If you want to make it just remove the PM table and redirect straight to the old report, that works for me. How would you recommend we do this? You can't have two default local tasks can you?

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

I agree. I saw the code and wasn't sure the best way to approach this. Thankfully, in the case of PM, performance doesn't matter much because you're just operating this code when you're using it, not frequently and only by a single user. Really, we could do a lot with a Package Management API, and I have some ideas here, hopefully we can get some time to discuss them at a DrupalCon sometime...

"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. ;)"

I guess I'm lucky :)
Yeah, that was a really good reply. At least we agree about how lousy it is to have to change tests in just about any commit with front-end implications. I do feel the same way, that in the rush to say "we've got tests", we have a lot of bad tests. The counter argument is often "some tests are better than no tests". I disagree with this sentiment though: bad tests are worse sometimes. Anyway, we'll fix the APIs, decouple them, make small edible functions - like finger sandwiches to the hogies which currently exist and re-write them, so no worries :)

We need to get this puppy in.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

FileSize
61.95 KB
Failed: 13568 passes, 17 fails, 0 exceptions View

Okay, re-fixed the update tests, since new ones have been added.

Also codered about 10 lines.

JacobSingh’s picture

Status: Needs work » Needs review
JoshuaRogers’s picture

FileSize
62.45 KB
Failed: 13523 passes, 17 fails, 0 exceptions View

It looks good overall. I had one issue though: plugin.php kept dying because of references to ts(). I changed them to st() and rerolled. Other than that, it worked wonderfully on my setup.

JacobSingh’s picture

FileSize
63.71 KB
Failed: Failed to apply patch. View

Okay, there were some WTFs in there from a coder session + cut and paste run awry.

Also, chatted with chx and he helped out a lot getting this puppy to run at a lower bootstrap - bonus.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

Status: Needs work » Needs review
FileSize
65.67 KB
Failed: Failed to apply patch. View

Okay, here's another.

  • Added some more docs
  • Fixed the FTP w/ streams FileTransfer class - and tested it
  • Removed some old code from the chmod "solution"

-J

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

FileSize
64.31 KB
Failed: 13593 passes, 17 fails, 0 exceptions View

Okay, screwed something up with the last patch, git still hurting me.

JacobSingh’s picture

Status: Needs work » Needs review
chx’s picture

Status: Needs review » Needs work

- 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.

JacobSingh’s picture

Thanks a lot chx, means a lot to me, all good comments.

But, heh, that's not such a dirty word, I can do worse! Just kidding, should have seen that though, we do have kids working on core after all :P

Did you run the code? Everything working properly on your box?

I'll make these edits if I can tonight, else first thing in the morning tomorrow.

Best,
J

mcrittenden’s picture

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.

JacobSingh’s picture

Status: Needs work » Needs review
FileSize
65.8 KB
Failed: 13632 passes, 9 fails, 0 exceptions View

Okay, this should address the stuff in @chx's review.

It also fixes some new stuff in udpate.test.

The only one I didn't touch yet is the install.inc idea. I agree, but I don't want that to hold things up. Also, it's not 100% that these requirements will remain the same.

JacobSingh’s picture

FileSize
65.88 KB
Failed: Failed to apply patch. View

K, @chx asked me to fix my url() calls, which I did.

Also, re-rolled due to the Theme Apocalypse.

-J

skilip’s picture

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.

JoshuaRogers’s picture

My understanding is that the only time FTP is invoked is to change the file permissions so that the webserver can write the files. I'm not sure how the system would need to work any differently if you were connected via localhost.

JacobSingh’s picture

@skillip, @JoshuaRogers

I re-wrote this to use chmod after Adrian's review, but that turned out to not work too well in all cases. See http://drupal.org/comment/reply/538660#comment-2065738 for the run-down and eventual decision to go back to using FTP. So for now, you need to have FTP running for this to work. I suppose we could alter it to just do the operation if everything is writable, but the decision at the time was that code created by the webserver can then not be rm'd or moved by the system user, and this was undesirable (obviously).

"When the file system isn't properly configured yet, you should get a warning message @ admin/config/modules/install"

Can you explain this? I don't really get it.

The intention is that sites/default is not writable by the webserver, and when you use PM, it uploads there. To run an FTP server on a mac, it's quite simple, it's already there, just start it. On Ubuntu machines, I've used proftpd with this - no issues.

Hope that helps, and thanks for looking - keep it up!!

J

webchick’s picture

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.

Bug or by design?

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

Full update report

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:

drush  dl dhtml_menu-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!

one module upgrade needed

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:

Scary

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

FTP

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:

Errors

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.

JacobSingh’s picture

Quickly before running off to family stuff:

1. SSH is not an option. Sad panda

That's probably because you don't have the ssh extension to PHP.

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?

Yes, I think this is stupid. I fought like crazy to simplify this, others fought back, I got tired of fighting: http://drupal.org/node/395472#comment-1737564. Is one such comment where I suggest going back to it. Eventually what got committed did not reflect my original design here. My original design was that we offer one option "FTP" which uses the first working method (extension of streams).

Is this a "must do"? I would be happy to change the architecture back, but not happy to open up another huge argument which I apparently lost before.

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."

Kind of a PITA because we don't have good exception handling really, and there was resistance to building any. Ideally, I'd like to have a FileTransfer Exception which is extended by FileTransferAuthentication, etc. so we can trap more accurately.

Is this a "must do"?

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.

No need to enter a path, we figure it out for you. If you got a WSOD, we need to check some logs most likely, I have no idea. Let's figure that out.

JacobSingh’s picture

Thanks for the review Angie,

We also chatted on IRC, and since I'd never seen the WSOD, I got ahold of a DH account, and tested this. It's not specific to DH, but it's also quite obvious what is going on.

to continue reviewing / testing, set perms on sites/default to 755 and make sure it is owned by the FTP user.

When installing Drupal, if the webserver and the user owning the files are the same - like most mod_cgi setups, we(Drupal) set the perms of sites/default to 555. Why we do this I don't know. It seems kinda stupid to me because if the webserver can set the perms to 555, it can also set them to 777, making it no safer AFAICT if it gets compromised.

Anyway, the PM default is to install to conf_path() . '/modules' and .'/themes'. Because of this, PM creates these dirs if they don't exist. Unfortunately, it is unable to create them because of the permissions being 555.

As long as sites/default is owned by the same user as the web server, it is easy enough to set these perms higher during the PM operation
This is a todo for me, I had it before and should be simple to re-implement.

If it is owned by the FTP user and NOT the webserver, but set to non-writable (for whatever reason), then we are kinda outta luck.

I wrote a chmod method for the FileTransfer classes. Snuck it into #528326: FTP FileTransfer classes need to be able to establish if they are chrooted.

Problem is, this only works the the FTPExtension and SSH classes, FTPWrapper in PHP simply doesn't support chmod :( Ultimately, it would be good to write / re-purpose a straight socket based FTP interface to avoid these complications (I believe WP does this).

So, our options are (in order of easiest to best).

1. Show the user an error and tell them they have to create these directories and set them to 755 or set conf_path() to 755.

2. Try to chmod for the user if they pick a chmod compatible FileTransfer mechanism, else, show them the error in #1.

Conclusion
I will fix the first point above (mod_cgi thing), so that if the webserver can chmod, we should just do it. This is minor, should wrap it up tonight and doesn't impede review. However, I also suggest we stop the false security of setting sites/default to 555 if it doesn't do anything. If I'm wrong (and not surprised if I am, please correct me).

Regarding the case where the user sets to the perms too low for themselves to write to, I suggest we simply make sure we can show an error to this effect post code freeze as a follow up and move on. They obviously know enough chmod to get themselves into trouble, I think they can get themselves out.

JacobSingh’s picture

FileSize
66.66 KB
Failed: Failed to apply patch. View

This should address the WSOD we encountered due to permissions and mod_cgi/php. There is polishing around the error handling needed, this I'm hoping can be a follow-up.

JacobSingh’s picture

FileSize
67.58 KB
Failed: Failed to apply patch. View

Add some more docs to updaters.inc

dww’s picture

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.

dww’s picture

FileSize
69.09 KB
Failed: Failed to apply patch. View

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...

webchick’s picture

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:

Install link

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:

Error

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:

Ready

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’s picture

God, I wish we had google wave. :(

Cannot save the tarball. This is actually in file.inc, and is probably because the perms on your files/private/temp dir are not setup correctly, can you check this out? If it worked the 2nd time, perhaps file.inc is doing some magic? no idea here.

Regarding the sites/all vs. sites/default thing
This has a long and painful history I won't trouble you with. Basic gist is this:

1. If we install to sites/all you can update your field's module without meaning to. This is bad for MS.

2. ALL of our docs, say to install stuff in sites/all :(

3. To remedy, it will always install in conf_path(), and if it is a newer version than something in sites/all, it should override it.

I assume you're not looking closely enough at the modules page, or something else is amiss because if it is in sites/default/modules it should be working... what are the perms on the created directory?

BEst,
J

JacobSingh’s picture

FileSize
69.28 KB
Failed: Failed to apply patch. View

Okay:

This should fix the problem dww noticed with the very restrictive umask and provide *slightly* better error handling.

Namely, when creating conf_path() . '/modules', it will try to chmod conf_path() using the webserver. If that doesn't work, it will try to create the modules directory with the FileTransfer. If for some reason that doesn't work, it will try to chmod conf_path() with the FileTransfer and try again.

After this, it will ensure that the created directory is o+x.

Hell yeah.

dww’s picture

FileSize
74.04 KB
Failed: Failed to apply patch. View

@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).

webchick’s picture

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.

int’s picture

Status: Needs work » Needs review

#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.

int’s picture

Status: Needs review » Needs work
webchick’s picture

Status: Needs review » Needs work

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.

JacobSingh’s picture

@webchick:

That's fine, but:

This is meant to update all types fo Drupal projects (for now, just modules and themes). SO the modules page may not be best.

I agree about the menu placement, AND 99% of the time, people go there because of a message they get. Like software update on your mac, do you ever just wake up and say... huh, I wonder if I am up2date :)

@int: SOB... I need more details, we're not seeing this. ls -l your sites/deault /modules directory. Tell us some backstory. Did you create the modules directory?

As per the problem with the error handling, I'll fix that, that was a recent regression I think.

Best,
Jacob

webchick’s picture

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.

int’s picture

Status: Needs review » Needs work

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.

dww’s picture

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.

int’s picture

Status: Needs work » Needs review

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

int’s picture

Status: Needs review » Needs work

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

JacobSingh’s picture

Status: Needs work » Needs review

Marking back to needs review because it seems to work.

dww’s picture

FileSize
74.5 KB
Failed: Failed to apply patch. View

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.

dww’s picture

Issue tags: +Update manager

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

dww’s picture

FileSize
72.98 KB
Failed: Failed to apply patch. View

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++ ! ;)

dww’s picture

FileSize
72.53 KB
Passed: 13926 passes, 0 fails, 0 exceptions View

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).

JacobSingh’s picture

FileSize
2.35 KB
Failed: Failed to apply patch. View

btw, here is the interdiff of the above

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

FileSize
70.94 KB
Passed: 13921 passes, 0 fails, 0 exceptions View

In anticipation of probably the most important follow-up (the messed up report):
#602484: Fix the report page when authorize.php completes an update manager operation

Ugh.. I got into merge hell, and before posting this, I realized it is completely screwed up.

This patch changes the report page to stop using update.php's report, sanitizes the log format so it doesn't use update.php's array from hell, and implements it as theming functions.

The report looks like hell now, but it is a start toward something more sane, even if we have to move the theming functons around.

int’s picture

Status: Needs work » Needs review

go bot

dww’s picture

Title: Move plugin manager upgrade process into new plugin.php file (and make it actually work) » Move plugin manager upgrade process into new authorize.php file (and make it actually work)
Assigned: Unassigned » dww
Status: Needs review » Needs work

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.

dcoun’s picture

subscribing

dww’s picture

Status: Needs work » Needs review
FileSize
80.71 KB
Failed: Invalid PHP syntax in includes/authorize.inc. View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

dww’s picture

Status: Needs work » Needs review
FileSize
80.71 KB
Passed: 13911 passes, 0 fails, 0 exceptions View

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...

agentrickard’s picture

Status: Needs work » Needs review

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’s picture

Status: Needs review » Needs work
dww’s picture

Status: Needs review » Needs work
FileSize
80.64 KB
Passed: 13943 passes, 0 fails, 0 exceptions View

@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.

Paul Natsuo Kishimoto’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Needs work

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. :-)

JacobSingh’s picture

Nice review.

I agree with everything not mentioned here (and some of the stuff mentioned here).

- 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.

Yes, this kinda sucks, I guess we need yet another declarative hook for this :( poo.

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.

Agreed generally, but I feel like some of those methods have value being static elsewhere, and I don't think we need to lock them in to an instantiation pattern. Plus, this is never going to be a performance critical operation AFAICT.

- 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.

Sorry, Python hangover. I like using Error, but you're right, that is the convention.

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.

Basically because we want to handle an error transfering the file in a friendly way. Other types of exceptions should pass up the chain and either cause massive failure with a stack trace, or be caught by someone else.

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.

This is a royal PITA. And has been discussed ad naseum. See Adrian's review, this issue and Part 2 of the original one for background if interested. The problem is that we don't want to cause problems for multi-site installs. I've always maintained I don't give a shit, and we should just upgrade it, or install in a user specified location. This tool isn't for multi-site :)

The compromise though is that we will install in conf_path() because it will override whatever is in sites/all.

I also hate this because the user can be confused by having two copies of the same module.

- 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().

I tried this, and it didn't work for me. If you can figure out how to make it work, be my guest. I know it is supposed to, so perhaps it is possible.

Best,
J

dww’s picture

FileSize
80.22 KB
Passed: 13916 passes, 0 fails, 0 exceptions View

@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!

JacobSingh’s picture

Oh, one more thing:

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

Updater does not implement DrupalUpdateInterface - at least it didn't used to. If it did so, that would defeat the purpose (and throw fatal errors because the Updater class does not Implement all the methods).

perhaps DrupalUpdaterInterface should evolve into DrupalProjectInterface, with other Interfaces for DrupalCore, 3rd part libs, etc... Maybe maybe not, but if the base class implemented the interface it wouldn't really serve any purpose.

Not important, but just saying....

In response to the "todos" @#111

A) Download, verify and then go to authorize.

I don't see any other way, and don't want to. dww and Crell and I chatted. I think we agreed that it is best for Updaters to take a directory, and have the responsibility of downloading the file and extracting belong to something else. For now, probably just the way it is. To unify the process of updates vs installs, we could very well add a download step to the upgrade process. It would be a little bit disruptive to the UI, but not a big deal, just put it in-between step one and step two, and use Batch API in case of a slow connection. I'm not marking this must-have, but it is what I would do.

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.

Be more specific here. Looking at it, I see problems with the logging stuff, but otherwise there is only one conditional in update_batch_copy_project. Also, this comment was added:

+++ modules/update/update.authorize.inc 14 Oct 2009 06:41:08 -0000
@@ -0,0 +1,266 @@
+function update_authorize_run_update($filetransfer, $operations) {
...
+ // @todo WTF? Just because a single operation finished doesn't mean the
+ // batch is complete. Does this even work for updating multiple things?
+ $context['finished'] = 1;
+++ modules/update/update.manager.inc 14 Oct 2009 08:22:18 -0000
@@ -0,0 +1,534 @@
+ return system_run_authorized('update_authorize_run_update', drupal_get_path('module', 'update') . '/update.authorize.inc', $operations);

FYI: $context['finished'] refers to the operation in question, not the entire batch.
http://api.drupal.org/api/group/batch

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.

Yeah, I agree with this proposal in so much as it should reside in the updater. I think it should be called at the time of install or update though because that is where the install location is ascertained and a write operation is certainly expected, and yes, it should be required (part of the interface).

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()". ;)

I agree it is a little ugly AND... we are caching that info in a DB table, so getting it again via the same API seems saner to me then just adding variables to the session, which are anyway ending up in the DB. I just don't think it matters much either way, but I'm fine specifying the versions in the _SESSION too.

I'm on crack. Are you, too?

Crell’s picture

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.

dww’s picture

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. ;)

JacobSingh’s picture

Okay, @dww and I had a marathon IRC session. Here is the punch list and some architectural decisions:

1. All downloading and verification happens in update.manager.inc, not in authorize.php or in update.batch.inc. This streamlines the install and update processes, and makes the code easier to read. As a consequence, during updates, the code is downloaded first. This means a new step. After selecting ones' updates, a batch process to DL them and store them in update-cache will be started. When it completes, the user will see step 2 (take a backup, and go offline). If there is a failure, they will be sent back to the update selection page with an error.

2. The update.batch.inc stuff will no longer get the version again. Now, it will only copy from directory A to directory B. This is awesome, and the step of DL'ing the code first makes this so much better and extensible. yeah for communication and thinking forward. So we're changing the format of the batch operations a bit, and the method signature of update_batch_copy_project (it gets simpler).

3. A new hook. (to be named). hook_verify_update_archive? I dunno.. signature will look like this:

/**
 * Gets fired for each archive downloaded that is about to be installed.
 *
 * @return
 *   Void if successful, otherwise an error message?  Should we implement an Exception class here?
 */
function hook_verity_update_archive($project_name, $source_url, $archive_file_path, $extracted_directory_path);

This opens up the possibility of package signing as a contrib add-on later.

4. Use of stream wrappers to retrieve packages. So if someone provides a url like s3://path/to/my/module.zip in their update status XML, we should be able to deal with it. Also, extensible in the future.

5. A new interface for Archive formats. Covered in #604618: Create a common interface for Archive operations so we can handle .zip, .tar.gz., but needs to be implemented here.

Okay, I think that's a wrap.

-J

dww’s picture

Status: Needs work » Needs review
FileSize
85.6 KB
Passed: 13920 passes, 0 fails, 0 exceptions View

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. :/

dww’s picture

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.

JacobSingh’s picture

Yeah, I'm aware that Batch API is FUBAR in many ways.

To deal with the lack of error handling, update.php (and our batches which copied it), use a [#abort] key which every op has to check for. It's awful, but there is no other remedy. I put a comment to this effect in my code and on an issue queue for PM somewhere (one of the many).

More later as I caffeinate

JacobSingh’s picture

We're looking pretty good here. A couple small comments re: Crell's architecture gripes (which I mostly agree with).

I'd like to see if we can't get a commit, to get this patch down to size a bit.

+++ includes/updater.inc	14 Oct 2009 23:16:48 -0000
@@ -0,0 +1,322 @@
+  /**
+   * Figure out what the most important (or only) info file is in a directory.
+   * 
+   * Since there is no enforcement of which info file is the project's "main"
+   * info file, this will get one with the same name as the directory, or the
+   * first one it finds.  Not ideal, but needs a larger solution.
+   *
+   * @param string $directory
+   *   Directory to search in.
+   * @return string
+   *   Path to the info file.
+   */
+  public static function findInfoFile($directory) {
+    $info_files = file_scan_directory($directory, '/.*\.info/');
+    if (!$info_files) {
+      return FALSE;
+    }
+    foreach ($info_files as $info_file) {
+      if (drupal_substr($info_file->filename, 0, -5) == basename($directory)) {
+        // Info file Has the same name as the directory, return it.
+        return $info_file->uri;
+      }
+    }
+    // Otherwise, return the first one.
+    $info_file = array_shift($info_files);
+    return $info_file->uri;
+  }

Okay, I know I caused this WTF, but we need to sort the interface thing out. I'm happy to ditch it and make Updater an abstract class, but then it is just for Drupal "projects", not for Core or anything else. It anyway assumes and InfoFile, etc. The Interface was more a device to make a plugin system to be honest, and badly used. I suggest we just fold it in and revisit this.

+++ includes/filetransfer/ftp.inc	14 Oct 2009 17:26:38 -0000
@@ -19,29 +28,29 @@ class FileTransferFTPWrapper extends Fil
   function createDirectoryJailed($directory) {
-    if (!@drupal_mkdir($directory)) {
+    if (!@drupal_mkdir($this->connection . $directory)) {

I'd like to do something about this at some point. Someone put drupal_mkdir in there, and I don't like it because we don't use anything else from DrupalCore in FileTransfer classes, and if we start, we can't upgrade core.

Does not need to happen now.

I'm on crack. Are you, too?

dww’s picture

FileSize
86.6 KB
Passed: 13905 passes, 0 fails, 0 exceptions View

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.

JacobSingh’s picture

We're getting very close. Doing a little UI review:

Configuration and modules | My Site

When running:

Notice: Undefined variable: output in /Users/jacob/work/github/drupal/includes/theme.inc on line 899 Call Stack: 0.0002 1. {main}() /Users/jacob/work/github/drupal/authorize.php:0 0.0805

Also, no report shown.

dww’s picture

FileSize
86.97 KB
Passed: 13931 passes, 0 fails, 0 exceptions View

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

dww’s picture

FileSize
89.88 KB
Passed: 13960 passes, 0 fails, 0 exceptions View

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

JacobSingh’s picture

The notice is due to an unfinished theme function in theme.maintinance.inc:

function theme_authorize_report($variables) {
  $messages = $variables['messages'];
  $output = '';
  if (!empty($messages)) {
    $output .= '<div id="authorize-results">';
    foreach ($messages as $heading => $logs) {
      $output .= '<h3>' . check_plain($heading) . '</h3>';
      foreach ($logs as $number => $log_message) {
        if ($number === '#abort') {
          continue;
        }
        $output .= theme('authorize_message', array('message' => $log_message['message'], 'success' => $log_message['success']));
      }
    }
    $output .= '</div>';
  }
  return $output;
}

/**
 * Render a single log message from the authorize.php batch operation.
 *
 * @param $variables
 *   - message: The log message.
 *   - success: A boolean indicating failure or success.
 */
function theme_update_plugin_manager_message($variables) {
  $output = '';
  $message = $variables['message'];
  $success = $variables['success'];
  if ($success) {
    $output .= '<li class="success">' . check_plain($message) . '</li>';
  }
  else {
    $output .= '<li class="failure"><strong>' . t('Failed') . ':</strong> ' . $message . '</li>';
  }
  return $output;
}

Note: theme('authorize_message',

And the old function name (theme_update_plugin_manager_message).

-J

dww’s picture

FileSize
93.18 KB
Passed: 13947 passes, 0 fails, 0 exceptions View

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.

JacobSingh’s picture

FileSize
10.74 KB
Failed: Failed to apply patch. View

For ref, diff of 131 and 134.

Am reviewing...

dww’s picture

FileSize
93.17 KB
Passed: 13934 passes, 0 fails, 0 exceptions View

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

dww’s picture

FileSize
93.17 KB
Passed: 13961 passes, 0 fails, 0 exceptions View

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
catch’s picture

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.

JacobSingh’s picture

Status: Needs review » Needs work

Sorry :( Installing themes is currently borken.

Working on it.

JacobSingh’s picture

FileSize
5.53 KB
90.06 KB
Passed: 13824 passes, 0 fails, 0 exceptions View

The interdiff acted funny because of some fuzz, but I think the patch is okay.

Themes now install cleanly, and if the same name as the project, get enabled as well.

@catch: That shouldn't be happening. :( That looks like a bug to me. Is dhtml_menu in sites/all ? Perhaps one of the recent changes broke sites/all -> sites/default "upgrades".

JacobSingh’s picture

@catch:

I really disagree about going to the old report when the warning tells someone they are out of date. We have a kill switch it people want to use the old report. What will 90% of the people want to do when told their site is out of date, is it:

A). Scream running to the hills
B). Make a double whiskey
C). Go to a report which tells them a bunch of detail about what is out of date exactly, and the gory details of what is in each package - with a tiny link that is kinda hidden where they can update.
D). The place where they can get up to date.

I don't think this is a commit blocker, but I do think it's kinda important.

-J

JacobSingh’s picture

Status: Needs work » Reviewed & tested by the community

I can't replicate @catch's issue, but I'm sure it will come up again at some point.

For now, until we can, marking RTBC - but I realize it is a bit of wagging the dog, so would like to get a +1 here.

One API change will involve #604618, but is not the end of the world if we can't make it happen (just the end of zip files).

But the rest are cosmetic.

weph. just weph. @dww is awesome.

JacobSingh’s picture

Status: Reviewed & tested by the community » Needs review

Wait, probably want a bot to test #140 first.

JacobSingh’s picture

Hmm... we have something kinda like a bug. There is an assumption in the code that people install directly in sites/*/modules. But as @catch found out, if you install in sites/default/contrib... it will install another (newer) copy in sites/default/modules.

This is all because we don't want to update stuff in sites/all/modules for fear of multisite installs not liking it.

I guess conf_path() is the top of the directories that get scanned, so really the logic should be:

If $old_location is not in conf_path():
just leave it there, could be multisite, and put it in conf_path()/modules

If $old_location is in conf_path():
Replace it in its current location

Does this sound right? Does this have to block a commit? It's not a fatal error, and the upgrade still works (I still don't know what caused @catch to see a warning there).

catch’s picture

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

JacobSingh’s picture

FileSize
1.96 KB
90.99 KB
Passed: 13838 passes, 0 fails, 0 exceptions View

Okay, this should address what catch found.

Like my sweet sweet interdiffs? Will be posting about how to streamline your patch creation soon.

catch’s picture

Last patch fixes the bug I had.

dww’s picture

@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. ;)

JacobSingh’s picture

I thin #146 address the issue (at least it isn't broken). If we want add more smarts to it later we can. Really just need an RTBC here. c'mon people, it's only 100k :D

Seriously though, I think it is well wort committing at this point, the final tasks are minor in comparison.

-J

alexanderpas’s picture

+++ 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.

JacobSingh’s picture

@alexanderpas: Thank you VERY MUCH for the review.

I don't have time right now to deal with this right now sadly, need sleep badly.

Since today is code freeze day, and this patch is 90k+ and dww and I haven't slept in a few days, I'm just going to say we're ignoring stuff like "This variable is not too long" or "this variable could have a better name" for now.

It's not that what you're saying isn't valid, it's just that we have a series of followup issues to deal with even more important stuff (like language in the UI, etc), and that is the place for stuff like that IMO rather than blocking a commit here. @catch actually started another issue for them:

#605344: Docs and code style fixes for update manager.

I would encourage you to post anything from this patch which you feel should be changed, but is not a commit blocker over there. And while I agree with every comment (minus some of the variable name ones), I don't think any of them warrant a full re-roll before a commit, so I'm balking on it for now.

Thanks,
Jacob

webchick’s picture

Yoink. Taking this for a final spin.

JoshuaRogers’s picture

Status: Needs review » Reviewed & tested by the community

It works fine for me! :) Let's get this thing committed!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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!!!

dww’s picture

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

xmacinfo’s picture

Awesome. Congratulations to all!

dww’s picture

@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

dww’s picture

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

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

JohnAlbin’s picture

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

Status: Fixed » Closed (fixed)
Issue tags: -Drupal 7 priority, -Exception code freeze, -Update manager

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

znerol’s picture

Component: update system » ajax system
Issue summary: View changes

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.