When you're prompted to authorize filesystem changes via FTP and the auth process fails (which it will in properly secured servers that don't support FTP), the user is left staring at a page with an error message, the data form, and no navigation out of the dead-end path.

Minor, but I expect a long series of Saturday morning "I'm lost" posts from newbies.

CommentFileSizeAuthor
#60 686060-60.check-filetransfer-availability.patch11.58 KBdww
#55 686060-55.check-filetransfer-availability.patch11.43 KBDavid_Rothstein
#55 install-modules.png40.08 KBDavid_Rothstein
#52 686060-51.check-filetransfer-availability.patch11.52 KBDavid_Rothstein
#52 authorize-php.png52.4 KBDavid_Rothstein
#46 step_1.png32.22 KBcarlos8f
#46 step_2.png48.97 KBcarlos8f
#46 step_3.png29.48 KBcarlos8f
#46 try_back_uh_oh_something_broke.png104.89 KBcarlos8f
#45 686060-45.check-filetransfer-availability.patch8.89 KBdww
#44 686060-44.check-filetransfer-availability.patch7.68 KBdww
#42 686060-42.check-filetransfer-availability-A.patch7.8 KBdww
#42 686060-42.check-filetransfer-availability-B.patch9.1 KBdww
#34 686060-34.check-filetransfer-availability.patch6.71 KBdww
#34 686060-34.check-filetransfer-availability-update-A.png79.4 KBdww
#34 686060-34.check-filetransfer-availability-update-B.png50.22 KBdww
#34 686060-34.check-filetransfer-availability-no-update.png46.29 KBdww
#33 686060-33.check-filetransfer-availability.patch6.35 KBdww
#33 686060-33.check-filetransfer-availability-install.png59.2 KBdww
#33 686060-33.check-filetransfer-availability-no-install.png26.76 KBdww
#33 686060-33.check-filetransfer-availability-update.png40.19 KBdww
#33 686060-33.check-filetransfer-availability-no-update-A.png37.8 KBdww
#33 686060-33.check-filetransfer-availability-no-update-PSYCH.png27.42 KBdww
#30 686060-filebackend.patch5.43 KBCrell
#24 686060-filebackend-24.patch5.86 KBDavid_Rothstein
#22 misleading_ui.png58.26 KBcarlos8f
#14 686060-filebackend.patch1.54 KBCrell
#13 686060-filebackend.patch1.82 KBCrell
#2 authorize-php.png31.42 KBDavid_Rothstein
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcvangend’s picture

Version: 7.0-alpha1 » 7.x-dev
Priority: Minor » Normal

This may not occur very often, but I still think it's not minor. (see http://drupal.org/node/45111) Also setting it to 7.x-dev so it will get a little more attention in the issue queue.

David_Rothstein’s picture

Title: authorize.php is a UI dead end » authorize.php is a UI dead end (when trying to install new modules on servers that don't support FTP via the update manager)
Component: update system » update.module
Priority: Normal » Major
Issue tags: -#d7ux +Needs usability review, +String freeze
FileSize
31.42 KB

Bumping this to major. Here is a sampling of forum posts and issues where people seem to be experiencing some version of this confusion:

http://drupal.org/node/876020
http://drupal.org/node/930988
http://drupal.org/node/956030
#965836: Fail to install themes and modules
#966132: Can't get past authorize.php

The problem is that the "Install new module" action link on the Modules page is very inviting, and it takes you to a page where you can paste a URL or upload a file. On any server, you can successfully do that.

However, the next page looks like the attached screenshot, which is pretty daunting looking to begin with - but more to the point of this issue, it completely fails if your server is not configured to allow FTP or other kinds of uploads.

Basically, to solve this we somehow have to tell people on that page (or the previous one) that if they can't use this interface, they can still log on to their server and upload modules the old-fashioned way.

David_Rothstein’s picture

The related documentation page (which we should perhaps be linking to somewhere in this interface) is http://drupal.org/node/895232 and also looks like it needs some updates along the same lines. We need to explain to people that on some servers, the Update Manager UI is not able to be used.

vcardoso’s picture

Yes, I was one of the fireflies bumping onto the lamp. I posted the #965836 issue only to realize that it was acting like expected...
I first tested on my localhost without success. Later I installed D7b2 on a VPS at Rackhosting and it worked OK, but with the annoying issue of having to insert *for each and every new module* the server connection settings...
We'll on a production server with all the security issues concerned, I can live with that, but for a localhost development environment it is really a pain. It goes that it really does not work at all on my localhost machine. I've even installed ftpd (that accepts my user and password to connect to the server) but it still does not accept my connection settings. On development environment we should be able to install/remove modules and themes on the fly, not having to ask the master gate keeper for permission on each time.
Can't this be eased by a "safe and automated login", some kind of "ssh public key"?
How does it work on Wordpress? It is really transparent!

I agree, as David states, that to point this feature as one of the great innovations on D7 is a true "bite your own arm" disappointment, if it will only run on "selected theaters". Better not to show the candy with one hand a take it off with the other...

Crell’s picture

I just talked this through with webchick. We decided the way forward is to check to see if there are any valid backends on the module install/update pages and if not show a nice "hey, enable a backend" message instead of the form. The same message should also be shown on the status page. A link off to more detailed instructions on d.o would be good, too.

Anyone want to tackle this? :-)

David_Rothstein’s picture

I don't understand how that would solve the problem?

From PHP's perspective, FTP will be a valid backend on many many systems - but that doesn't mean the server is actually configured to allow FTP access. Many will not. (That's the situation on my laptop certainly, where I made the screenshot in #2.)

See also:
http://api.drupal.org/api/drupal/modules--system--system.module/function...

I am not sure we have any good way to detect what we want to detect automatically.

David_Rothstein’s picture

For what it's worth, I took a quick look at Wordpress to see if they have some magic solution for this problem. They don't - the same issue occurs there :(

I think we can mitigate this most simply by putting some help text (or other visual message) on the install form and on authorize.php itself - and showing that message unconditionally.

If we want to also hide the form and replace it with "hey, you can't use this unless you enable a backend" in the case where there are actually no backends, that seems reasonable too (probably a good idea because authorize.php currently seems to choke if there are no backends!). But I think that's a bit lower priority because fewer people should find themselves in that situation.

Crell’s picture

It's the responsibility of the backends hook to determine if a given backend is actually viable. Right now it's just checking for a function_exists(). However, we have no valid error handling if that module_invoke_all() returns nothing. That's an easy fix we should do.

Making the "so can we REALLY use this backend" check more robust is also something we can do, but I see that as separate. (I think we've now got 2 issues conflated in the same issue. Yay!)

David_Rothstein’s picture

Do you know if there is an easy way to improve on the function_exists()?

We can (and should) assume for now that it won't be improved upon (and I agree it's a separate issue), but in that case we need to show a message to everyone in order to meaningfully solve the bug here. If there's an easy way to avoid having to show a message to everyone, it would be great.

I assumed it was a chicken-and-egg thing (you need to ask the user for the connection info before you can test if the connection can actually work on their server)...

Crell’s picture

Assigned: Unassigned » Crell

I am working on improving the error handling.

carlos8f’s picture

Basically, to solve this we somehow have to tell people [...] they can upload modules the old-fashioned way.

Yes! I think a simple message like that would be enough to solve this. Maybe along with a link back to the front page modules page, so it's not such a dead end.

We decided the way forward is to check to see if there are any valid backends on the module install/update pages and if not show a nice "hey, enable a backend" message instead of the form.

The target audience for this feature will probably not know what a "backend" is, much less how to enable one :) function_exists() is obviously not enough, either. The screenshot in #2 is very daunting, but I think we just need better help text. Especially if WP has the same issue.

carlos8f’s picture

Assigned: Crell » Unassigned

Hi Crell! Sorry for the cross post. I have to plug this issue: #218066: Prevent cross posting from reverting metadata fields

Crell’s picture

Assigned: Unassigned » Crell
Status: Active » Needs review
FileSize
1.82 KB

Here's a patch that gives people a nicer message that they can't continue, and directs people to a handbook page for more information. (The page may need additional details, but that's outside the scope of this thread.)

All of the relevant menu items appear to route through the same form callback, so there's only one page we need to edit.

Crell’s picture

FileSize
1.54 KB

Grr... Stupid htaccess.

carlos8f’s picture

"file handling backends" sounds overly technical. My impression is that this feature is for folks that may not even know how to use an FTP client. How are they supposed to know what a "file handling backend" is?

dmitrig01’s picture

This is a good start. However, I think that this issue (and correct me if I'm wrong) is more about linking people back to their Drupal sites (BLAH BLAH ERROR to get back to your drupal site click here) than that (but, your inclusion of the handbooks is a great idea).

carlos8f’s picture

Also, #6 is still a valid point. PHP may have the FTP extension installed, but the user may still need a help message like this. function_exists() != user_does_not_need_help.

Crell’s picture

Per #7, there's two issues here:

1) When there are no usable backends, our error handling sucks.

2) Our detection of "usable" backends is imperfect.

The patch here addresses #1. I am not convinced that we can solve #2, and it appears that WordPress doesn't solve it either. Also, #2 is not RC-sensitive or string-freeze sensitive so those are not a priority right now. #1 is a string-freeze issue so it needs to get solved in the next ~10 days or it doesn't get solved.

The error message text I am open to input on. Suggest something less technical and I'll drop it in. I'm easy. :-) The particular details of how to fix possible issues belong on the linked handbook page. The user-action we want to encourage here is "go see the handbook page to figure out how to fix this."

As for linking back to the site, I don't think that's necessary. We're still on an admin page. You still have the logo link, breadcrumbs, toolbar, and probably more links to go back to where you were, to say nothing of the browser's back button. Having yet another javascript:go(-1) link on the page is a complete waste.

carlos8f’s picture

You still have the logo link, breadcrumbs, toolbar, and probably more links to go back to where you were

http://drupal.org/files/issues/authorize-php.png (#2) seems to be void of any of this. authorize.php is a separate entry point and, I think, doesn't bootstrap normal stuff.

I think the help text should display regardless of whether module_invoke_all() is empty. Help text, especially if it refers to the handbook, can be useful for reference, even if they have the corresponding PHP extensions on their server.

Crell’s picture

Title: authorize.php is a UI dead end (when trying to install new modules on servers that don't support FTP via the update manager) » Display meaningful error message when no file transfer backends are available

That's not the page where this patch is adding a message. It's adding it on ?q=admin/modules/install, which has the toolbar, shortcuts, module/theme page tabs, and breadcrumbs.

How would the text differ for "we can continue" vs. not? If it works, we don't need to explain the concept of transfer backends to the user. It would just confuse them and add nothing to the process. If it doesn't, we want to explain why they're not getting a pretty form and then direct them off to instructions on how to fix it so that they will get a pretty form.

Also updating title to be more accurate...

David_Rothstein’s picture

We need to also display some help text when there are available backends, or we won't be solving the original reported issue in this thread (which is the main issue that people have been experiencing in real usage).

Since we don't think it's likely we'll be able to make the backend detection itself more robust, the only way to solve that is via help text - i.e., it's on a string freeze deadline also.

I think something along these lines?

[Installing/updating] a [module/theme] from this page requires [name of the backend] access to your server. Without it, you will not be able to proceed past the next page. Alternate methods for installing [modules/themes] are described in [link to handbook page].

Probably just displayed as simple help text on the first page (unless someone comes up with a better design that is easy to implement). Maybe if they do go to the next page and then get an error message when typing in their credentials, the error message should also direct them to this handbook or something also.

carlos8f’s picture

Title: Display meaningful error message when no file transfer backends are available » Explain that admin/modules/install only works if you have (S)FTP access to your server
Status: Needs review » Needs work
FileSize
58.26 KB

The "Install" button on admin/modules/install is misleading, too. It should be "Continue". The message in #21 can go in hook_help(), I think. And if the user gets an error on authorize.php, the place to inject the message would be in authorize_filetransfer_form_validate() before $e->getMessage().

dww’s picture

Title: Explain that admin/modules/install only works if you have (S)FTP access to your server » Explain that the Update manager only works if you have FTP or SSH access to your server
Issue tags: +Update manager

Thanks to everyone's help in this issue! It's such a relief to see action on fixing this stuff without me having to do it alone. ;)

A) Changing the title:
- Core doesn't support SFTP, only FTP and SSH...
- This isn't just about admin/modules/install. All the update manager operations have the same workflow/UI problems.

B) Right, we can't know if FTP or SSH will work until we have legitimate connection credentials and try them. So sure, it's a good improvement to alert users beforehand that it might not work, but we need much better error detection and propagation during authorize.php itself, too. This is starting to blend into what #605292: propagate failure during batches in update manager is trying to address, but we can at least focus on failing to connect via FTP/SSH in here and make sure those cases are well detected and reported to the end users in a helpful way.

C) The nasty looking UI on authorize.php itself is being addressed in both of these:
#602484: Fix the report page when authorize.php completes an update manager operation
#932846: authorize.php connection settings form broken (doesn't degrade, pointless fieldset)
Let's not worry about the cosmetic details of authorize.php in this issue. Let's focus on better error propagation when we fail to connect via FTP/SSH. Think of the kittens... ;)

D) Re: #21, I'm not sure what "[name of the backend]" is supposed to mean. There can be multiple possible backends. Core potentially supports 2 and contrib should be able to add more (although that won't be possible unless #609772: Impossible to extend the FileTransfer class system in contrib is fixed).

E) +1 to #22 that the button on the install landing page should say "Continue" not "Install". However, that should just be a separate issue since it's such a trivial fix, and is basically unrelated to the rest of the problems we're trying to address in here. Hence #976232: "Install" button at admin/*/install should be labeled "Continue" ...

There's probably more I should say here, but it's 2am where I am now, and I need to get to bed.

Thanks again, everyone!

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
5.86 KB

Here's a patch for what I suggested above. (In order to avoid overloading the page, this also required shortening the existing text.)

I realized an unfortunate complication for this issue: We need to take into account the possibility that the FileTransferLocal method will be used. If no backends are available, we don't want to prevent people from accessing the update manager interface if no backends are going to be used anyway. My patch contains a preliminary attempt to deal with that, which I'm not too happy with yet (for one thing, it could litter the temporary directory with lots of files, so it would benefit from some error handling and cleanup) but which does seem to work.

David_Rothstein’s picture

Oh, and this doesn't yet try injecting a similar message when an error occurs on authorize.php (#22), which we probably should do too.

Status: Needs review » Needs work

The last submitted patch, 686060-filebackend-24.patch, failed testing.

dww’s picture

Thanks for moving this forward!

In addition to whatever the test bot was upset about, this patch seems to duplicate a lot of code in 2 places. I'd rather see a shared helper function that returns the appropriate content for $form['available_backends']. I guess the text is slightly different in the two cases, but all the other logic is the same. So, it seems the helper would just need an argument to select which set of text to use...

Also note that this patch (and the above logic) is going to conflict with the outcome of #609772: Impossible to extend the FileTransfer class system in contrib. I guess that's the price of trying to fix a lot of broken aspects of the Update manager all at once. :/

-Derek

cosmicdreams’s picture

Status: Needs work » Needs review
Issue tags: -Needs usability review, -String freeze, -Update manager

#24: 686060-filebackend-24.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs usability review, +String freeze, +Update manager

The last submitted patch, 686060-filebackend-24.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review
FileSize
5.43 KB

Revised version that merges the text into a utility function. Unfortunately the logic for the other string is not easy to merge given that it has a return in the middle of it, so I left it alone for now as I am short on time. :-(

Hopefully the bot will give us useful information on this one...

Status: Needs review » Needs work

The last submitted patch, 686060-filebackend.patch, failed testing.

dww’s picture

Assigned: Crell » dww

#30 has a bunch of problems. I'm just going to re-roll instead of trying to enumerate them and hope someone else fixes this in time for the string freeze. Stay tuned. ;)

dww’s picture

Just to see what the bot thinks, I'm setting to needs review. I'm not thrilled with how _update_manager_check_backends() is working in this patch, but it's closer to what I had in mind.

However, as the screenshots will reveal, this is the wrong place to be adding the check for updates, anyway. We already present them with the first step in the workflow and have them invest time, energy and perhaps excitement that it's going to work, then we bother to download and verify all the updates, then we tell them: "Psych! This doesn't work given your PHP config -- suckah!!!" :/

Also note that these final landing pages in the 'no' cases are total UI dead-ends in these patches. We should probably at least include escape-route links back to /admin or something.

dww’s picture

Okay, using the magic of the shared helper, I just added the checks to both the first page in the update manager and the "ready" landing page (just in case they missed the memo the first time). I think this is at least a viable candidate, although I'm sure Bojhan will hate this. ;) The install cases are unchanged from #33.

Also, the 'no' landing pages are less of a dead-end if you look at the whole page. ;) Since this is all happening in the fully-bootstrapped parts of Update manager, we've still got the whole admin header/toolbar crap. So, if folks want to escape, they'll just have to use the header.

carlos8f’s picture

I like #34 using _update_manager_check_backends() to encapsulate the check and form injection. Screenshots look good, too.

Shall we try adding an additional help message if authorize.php encounters an error? That would help curtail the "stuck on authorize.php" support requests from folks that have a backend installed but still get errors later on. We should give them a link to the handbook in the error message so they don't feel so stuck.

Status: Needs review » Needs work

The last submitted patch, 686060-34.check-filetransfer-availability.patch, failed testing.

dww’s picture

Assigned: dww » Unassigned

Well, crap. Those test failures are what happens now if you run the tests on a site where PHP isn't configured to talk to FTP or SSH. Previously, such hosts could still get you to the install page and verify that the filename extension stuff (from our Archiver class hierarchy) still works. With this patch, that page is now just printing an error:

http://drupal.org/files/issues/686060-33.check-filetransfer-availability...

Instead of the install UI:

http://drupal.org/files/issues/686060-33.check-filetransfer-availability...

Ugh. Not sure what to do here. I also don't have a ton of time right now to focus on this, so I'm going to unassign myself so no one thinks I'm planning to drive this home myself. I'll try to get on IRC and see if anyone has ideas and wants to help...

David_Rothstein’s picture

Maybe update_test.module can implement hook_filetransfer_backends() with a fake backend (doesn't have to be functional on the server, just has to be an info array returned by the hook)? Then it should work.

I'll try to get back to working on this patch... not sure when I'll have time, though.

dww’s picture

I sure wish someone would commit #609772: Impossible to extend the FileTransfer class system in contrib before we spend a bunch more time on this issue, since that's going to change the hook we need to implement for this test to work. Note that that patch already has a fake backend in update_test.module...

EvanDonovan’s picture

#609772: Impossible to extend the FileTransfer class system in contrib was committed. FWIW, I like the strings in this patch (#34).

dww’s picture

Assigned: Unassigned » dww

I'm working on a re-roll now that #609772: Impossible to extend the FileTransfer class system in contrib landed... stay tuned.

Meanwhile, I forgot that the dummy FileTransfer stuff from #609772 is actually in system_test.module, not update_test.module. Is it more evil for an update.test class to install system_test, or should we do a 2nd dummy FileTransfer in update_test.module?

dww’s picture

Re-roll to work properly and apply cleanly now that #609772 is in.

Heh, maybe the problem isn't that the test bot's version of PHP can't talk to FTP or SSH. That might also be true. But the more obvious problem is that the string changed which the test was asserting. ;) It was looking for "archive extensions are supported" but Crell changed that text to "file extensions are supported". ;)

So here's two patches.

A just fixes the tests to look for the right strings. Works fine locally (unless I intentionally break my version of system.module to pretend PHP doesn't support FTP or SSH).

B adds a dummy UpdateTestFileTransfer class and update_test_filetransfer_info() function, so that the tests pass even if PHP doesn't natively support any FileTransfer mechanism. Maybe due to the architecture of how simpletest works, the files are all going to be owned as the httpd user, and we're always going to hit the local transfer case, anyway, in which case B is pointless.

Providing both to see what the bot thinks, and so that core committers can decide which they prefer.

yoroy’s picture

Status: Needs review » Needs work

String review of the screens in #33 and #34:

## http://drupal.org/files/issues/686060-34.check-filetransfer-availability...

(remove the *note: * bit)
Updating modules and themes here requires *FTP access* to your server. See the handbook for other installation methods

Updating Drupal core is not supported at this time. Install these updates manually.

## http://drupal.org/files/issues/686060-34.check-filetransfer-availability...

(remove the *note: * bit)
Updating modules and themes here requires *FTP access* to your server. See the handbook for other installation methods

## http://drupal.org/files/issues/686060-34.check-filetransfer-availability...

Your server does not support updating modules and themes from this interface. Update instead by uploading the new versions manually to the server, as described in the *handbook*

(wondering if it needs a stronger "instead, your next step now would be to manually etc.)

## http://drupal.org/files/issues/686060-33.check-filetransfer-availability...

(remove the *note: * bit)
Updating modules and themes here requires *FTP access* to your server. See the hankdbook for other installation methods

Find modules and themes on drupal.org. Supported file extensions: …

## http://drupal.org/files/issues/686060-33.check-filetransfer-availability...

(remove the *note: * bit)
Updating modules and themes here requires *FTP access* to your server. See the handbook for other installation methods

## http://drupal.org/files/issues/686060-33.check-filetransfer-availability...
Your server does not support updating modules and themes from this interface. Update instead by uploading the new versions manually to the server, as described in the *handbook*

(wondering if it needs a stronger "instead, your next step now would be to manually etc.)

- Putting in an escape 'back to /admin' link would be useful on the dead-end pages. Re-use what update.php does at the end of a succesfull update?
- (remove the *note: * bit)

dww’s picture

Status: Needs work » Needs review
FileSize
7.68 KB

New patch based on IRC chat with yoroy and David_Rothstein. Some of yoroy's concerns are out of scope, and some were a bit hasty. How's this?

dww’s picture

Whoops, I missed a few from IRC:

A) s/will require/requires/

B) s/Instead install/Instead, install/

Also, at David's suggestion, just to be more deterministic and less fragile, let's include the dummy filetransfer method in update_test.module, just in case folks try to run tests on a platform without FTP nor SSH support compiled into PHP. Even if our bots are happy, not all local tests will be.

carlos8f’s picture

Status: Needs review » Needs work
FileSize
104.89 KB
29.48 KB
48.97 KB
32.22 KB

I tried applying this patch, and although I never actually got the Update manager to work properly, I found a bug that we might want to look at that prevented this patch from showing anything useful:

+++ modules/update/update.manager.inc	1 Dec 2010 04:45:34 -0000
@@ -491,6 +512,77 @@ function update_manager_install_form($fo
+  // Do a preliminary check to see if file transfers will be performed locally.
+  // See update_manager_update_ready_form_submit().
+  $local_transfers_allowed = fileowner(drupal_tempnam('temporary://', 'update_')) == fileowner(conf_path());

conf_path() is sites/default, which www-data owns, so that check passed. But the install destination is sites/all/modules/..., which is not owned by www-data. So the install was doomed to fail before I even hit the "Install" button. Can we check the install destination instead of conf_path()?

Also got a strange message the first time I hit Install: "Archivers can only operate on local files: temporary://update-cache/blah.tar.gz not supported." Second time I tried, the batch started, but got an error. Fun screenshots attached.

webchick’s picture

So, this patch didn't make it in before RC1, and represents string freeze breakage. However, I'm ecstatic about the amount of momentum we've managed to build around update.module, and I think the usability WTF here is severe enough that it's worth breaking string freeze to get this patch in once it's ready.

dww’s picture

More evidence of the confusion this patch is trying to address: #967988: "Install from URL" can't install http://ftp.drupal.org/files/projects/views-7.x-3.x-dev.tar.gz. FTP login error.

Yes, we probably should be checking the final install location, not just sites/default, but that's a deeper bug.

The rest of your errors are definitely scary, but totally out of scope for this issue. :/ If you wanted to open a new issue about it, that'd be great.

carlos8f’s picture

Status: Needs work » Needs review

So properly checking writability of install destination needs its own issue... I'll cross-link when I get around to that.

Back to needs review.

Bojhan’s picture

As yoroy already proposed this, at the very least needs to remove the "Note" bit - in the way we want Drupal to speak its equal to using caps or explanation marks.

However I question the validity of this solution, as we are basically saying "it might not work". Unless you either know that you 1) don't have FTP/SSH capabilities in your server settings (which seems rather unlikely) or that you don't have access to that information (which is more likely).

In general though, this will only affect a minority of our users. For the majority it will either be useless, because 1) they dont know either or 2) they already have the information thus useless.

So I propose to either, leave this message out or to decrease its importance. For example appending only the important bit to the description. Looking at the support requests, they seem to be knowledge gaps we cannot fix with help text.

David_Rothstein’s picture

Updated the patch to fix/improve a few issues:

  1. Added an additional helpful error message when authorize.php fails to connect to the server (as requested above, most recently by @carlos8f in #35). This is also shown in the attached screenshot. We are starting to rely a fair amount on the same handbook page in this patch - maybe we need a followup issue to make sure that page is up to par :)
  2. Moved the $local_transfers_allowed check to its own helper function, so it can be better documented and do some rudimentary cleanup.
  3. Removed the $form = array() declarations from the previous patch (those are incorrect because they clobber what was passed in to the function).
  4. Added some documentation to the update_test_filetransfer_info() implementation to explain why we are defining this mock method for the tests.

On IRC last week, I discussed with @dww if _update_manager_check_backends() might be better named something like _update_manager_check_and_add_backends_message() - to emphasize the fact that it modifies the passed in $form array (and to help someone looking at the calling code who would otherwise have difficulty understanding how the form is built up). He said he'd think about that, so no changes for that in the attached patch just yet.

David_Rothstein’s picture

Lost the attachments, apparently.

David_Rothstein’s picture

By the way, in the screenshot, the only changes this patch introduced are that the error message in black ("Cannot connect to FTP Server, check settings") used to be a message on its own, without the "Failed to connect to the server..." and "For more help installing or updating..." messages before/after it. This patch adds the help text before/after it. Any other bugs you might happen to see in that screenshot are not related to this patch :)

@Bojhan (#50):

As yoroy already proposed this, at the very least needs to remove the "Note" bit - in the way we want Drupal to speak its equal to using caps or explanation marks.

The "Note" is not great, but it's there to provide some visual distinction between the two messages, for example in this screenshot (http://drupal.org/files/issues/686060-33.check-filetransfer-availability...).

If we can find another way to do that, we could remove it.

Unless you either know that you 1) don't have FTP/SSH capabilities in your server settings (which seems rather unlikely)...

Many people who have Drupal running on a VPS (or a dedicated server) will be in this situation, I think. They will probably have been told that their server wasn't set up to allow FTP access. What we need to communicate to those people is basically: "It's OK, Drupal doesn't have a requirement that you need FTP access to the server in order to install a new module."

Bojhan’s picture

Just a seperate paragraph is already visual distinction enough.

David_Rothstein’s picture

Maybe "visual distinction" was the wrong phrase. The point is that they are two different types of messages entirely and need to somehow be rendered to show that they have different purposes. One is help text for the form, and the other is an informational message. We don't - that I know of - have a standard for informational messages in Drupal core (e.g., blue background with an "i" icon or something like that), so we're stuck making something up.

The attached patch and screenshot shows what it looks like removing the "Note". I don't think it's an improvement, really.

The one simple thing I can think of here that might help would be to move the "Installing modules and themes requires..." informational message towards the bottom of the form instead (right near the Install button). Maybe we can try that. Either way, let's see if we can get this committed soon :)

Tor Arne Thune’s picture

Speaking as an end-user, without much experience in the correct lingo out there, "Note:" feels wrong and unprofessional. As for the distinction between the help text and the informational message, I feel that a separate paragraph is enough to separate the two.

This patch makes Drupal 7 easier to understand and will probably avoid some support requests. Therefore I hope it can be committed in time, seeing as it already got an extension from the string freeze.

yoroy’s picture

Thank you 1V. #55 Looks fine to me too. No need to visually seperate 'informational' from 'Help for the form', it's all equally related to the task at hand: installing modules. The problem child in that screen is actually the "The following file extensions…" line which is too much, too soon (Could be made part of the description for the local file upload, but that's out of scope for this issue).

UX-wise this is go. The last paragraph in #51 hints at this needing another *code* review though.

carlos8f’s picture

Code review time :) I'll try to give it a look soon.

Tor Arne Thune’s picture

So this basically needs a code review, and it can be marked as RTBC.

dww’s picture

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

This is mostly my code, but we're getting desperate here for 7.0. ;) I did another careful review and tested this manually, and only found a single, trivial problem, which is that the handbook link has been moved. So, instead of linking to a redirect, here's a new patch that links to the right handbook URL directly. That seems trivial enough not to feel bad about RTBCing my own patch. ;)

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs usability review, -String freeze, -Update manager

The last submitted patch, 686060-60.check-filetransfer-availability.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review, +String freeze, +Update manager
bfroehle’s picture

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

Status: Reviewed & tested by the community » Fixed

The translators are going to kill me for committing this patch so close to 7.0. :( But when you look at the never-ending list of duplicate support requests/bug reports about this problem alluded to in #2 and elsewhere in this issue, I really don't think we can ship update manager widely without this patch. We would leave our users basically stranded and without a way to report a sensible error message so we could deduce what's going on.

Reviewing the patch, _update_manager_check_backends() is a bit of a WTF since it doesn't just check backends, but also adds status messages to the form. I also seethe at adding $operation arguments to things. ;) But I talked to dww about it and it's basically the equivalent of the $phase argument in system_requirements(), not an $op like hook_block() and friends in D6 and below.

There are probably some other clean-ups we could do here, but in the interest of more-or-less locking down HEAD tonight, I think we're better off with this patch. Thanks so much to everyone who put so much time into this. It's awesome that this is getting cleaned up!!

Committed to HEAD.

Status: Fixed » Closed (fixed)

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

ehowland’s picture

Adding a link to some solutions since this link is at the top of a Google search on the error message.

It turns out that changing permissions within the sites folder can eliminate this problem.

https://drupal.org/documentation/modules/update/#noftp

Others suggest that you do not have to change permissions on the whole site but but just the sites/default folder (or equivalent in a multi-site):
https://drupal.org/node/1036494#comment-4690116

dww’s picture

I cry a little and wish I had never worked on the Update Manager every time I see these "helpful" suggestions about making your site less secure added to our official documentation and read threads like https://drupal.org/node/1036494