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

  1. #395472: Plugin Manager in Core: Part 1 (backend)
  2. #395474: Plugin Manager in Core: Part 2 (integration with update status)
  • #395478: Plugin Manager in Core: Part 3 (integration with installation system)
  • #395480: Plugin Manager in Core: Part 4 (installation profiles)
  • For more details on the other issues, click the above links.

    How cool would it be instead of just giving you the status of your updates, update.module actually let you *perform* those updates just by getting the md5sum and entering your ftp credentials? Pretty cool, right? It would turn "Update status" into more of an "Automatic updates" module — something that's a lot more useful to site maintainers, since they will no longer need to worry about automatic updates.

    CommentFileSizeAuthor
    #229 update d6.JPG106.93 KBint
    #223 updates.JPG118.87 KBint
    #218 updates_395474-218.patch44.8 KBJacobSingh
    #217 updates_395474-217.patch41.82 KBJacobSingh
    #208 updates_395474-208.patch46.72 KBAnonymous (not verified)
    #205 updates_395474-204.patch47.1 KBJacobSingh
    #202 updates_395474-202.patch47.09 KBJacobSingh
    #200 updates_395474-200.patch47.09 KBJacobSingh
    #199 drupal7.jpg48.54 KBint
    #178 updates_395474-178.patch42.25 KByched
    #177 updates_395474-177.patch42.47 KBJacobSingh
    #174 updates_395474-171.patch34.65 KBJacobSingh
    #173 updates_395474-171.patch32.09 KByched
    #167 updates_395474-167.patch21.68 KBJacobSingh
    #164 updates_395474-162.patch21.06 KBJacobSingh
    #161 updates_395474-161.patch20 KBJacobSingh
    #157 updates_395474-156.patch20.44 KBJacobSingh
    #156 updates_395474-155.patch13.07 KBJacobSingh
    #154 updates_395474-154.patch20.07 KBJacobSingh
    #136 updates-395474-136.patch40.01 KBdmitrig01
    #135 updates-395474-135.patch20.81 KBdmitrig01
    #133 updates-395474-133.patch40.08 KBdmitrig01
    #111 updates-395474-111.patch40.08 KBdmitrig01
    #108 updates-395474-108.diff30.44 KBJacobSingh
    #107 updates-395474-107.patch30.65 KBdmitrig01
    #98 module_updates_page_wireframe_idea_3.png487.28 KBSenpai
    #92 updates-395474-92.diff22.42 KBJacobSingh
    #90 Available updates | d7.7_1246954182040.png23.12 KBcatch
    #88 module_updates_page_wireframe_idea_2.png215.07 KBSenpai
    #86 updates-395474-86.patch23.34 KBdmitrig01
    #83 module_updates_page_wireframe_idea_0.png195.92 KBSenpai
    #82 module_updates_page_wireframe_idea.png194.62 KBSenpai
    #81 module_updates_page_wireframe_idea.png192.33 KBSenpai
    #54 updates-395474-54.patch25.09 KBdmitrig01
    #53 updates-395474-53.patch25.88 KBdmitrig01
    #49 updates-395474-49.patch27.25 KBdmitrig01
    #40 updates-395474-40.patch24.47 KBJacobSingh
    #39 updates-395474-39.patch24.45 KBJacobSingh
    #35 updates-395474-35.patch24.49 KBJacobSingh
    #31 updates-395474-31.patch21.74 KBJacobSingh
    #25 filetransfer_cleanup_4.patch19.64 KBJacobSingh
    Support from Acquia helps fund testing for Drupal Acquia logo

    Comments

    cwgordon7’s picture

    Title: Plugin Manager in Core: Part 1 (integration with update status) » Plugin Manager in Core: Part 2 (integration with update status)
    cwgordon7’s picture

    Status: Active » Postponed

    Postponed pending Part 1.

    JacobSingh’s picture

    Thinking about this.

    What model do we want to follow? What are good examples people can point to?

    I'm thinking that the best is to do what is most frequently experienced. I've been a linux user (almost every flavor) for 8 years and a mac user for 1. The process in both Ubuntu and OSX are the same.

    1. You get a little alert that there are new packages.
    2. It shows you a list of them and the "recommended" or "critical" status of each one.
    3. If you click on one, it shows you the release notes
    4. You check them off
    5. You hit go
    6. It downloads them and installs them
    7. If required, it tells you to restart (but doesn't force you).

    I will develop some more screens to turn this into a workflow which translates better to a more fragile web CMS and not an OS, but I think the main concerns are:

    1. Some updates require running update.php, some don't. Generally speaking, you have to run update.php immediately.
    2. Some updates you should definitely go offline for, some is optional
    3. Once you push the code in, the code is running. There is no "update upon restart".
    4. You should have a backup. People using this tool (we can assume) don't know how to use SSH or FTP or mysqldump. Maybe we ignore this issue, it opens up a whole other big problem of building a backup too... maybe one exists in contrib?

    I propose however, that we follow the above model as much as possible. This would mean to me at first glance:

    1. You get a little alert that there are new packages. You go to update status and instead of the current view, we just show the packages with updates (other ones will be hidden by default with a tab to show them).
    2. You see a list of packages and the "recommended" or "critical" status of each one.
    3. If you click on one, it shows you the release notes.
    4. You check them off.
    5. You hit go
    6. Packages are download to tmp/update-cache and extracted to update-extraction.
    7. We store the fact that you have pending updates and which ones.
    8. You go to a screen which encourages you to backup your system and take it offline. We provide a facility at least to go offline right there and then and if they don't want to, we provide a persistant nag message liek update status does on all admin pages.
    9. The user goes offline and goes to the "install me" page.
    10. The updates are installed one by one.
    11. The user is sent to update.php
    12. Everyone cross their fingers, mumbles some incantations, buys some life insurance and... nothing up my sleeve... PRESTO-MUNDO!

    Sound good?

    -J

    joshmiller’s picture

    re: Issue #4) Regarding "problems" with installations: Would it be possible to create a restore point? A full copy of the essential database tables (users, nodes, vars, modules, etc) and sites folder? Couldn't be more than 5MB normally and it could still be optional. Some projects to look at: Backup and Migrate, Drush.

    re: Installation Model #8) This may not be the right issue, but all nag messages should be turn-off-able.

    Otherwise, Jacob, your model of how to run the updates makes a lot of sense to me.

    JacobSingh’s picture

    Okay, here we go... :)

    Check out the wireframes here

    Please give me feedback on where we are going right / wrong here. One big usability hurdle I see is the lack of key signage and trust. I know there was a plan to match up md5s from d.o. but it's so ugly. I don't know how to fit it in. Also, if there is a MiM attack, couldn't it just fake d.o.'s MD5s anyway? There must be a better way... If we trust update status from d.o. shouldn't we trust the files as well?

    Okay, enough of that. Other than that problem, how we looking. I dare say, pretty f'n sweet.

    -J

    chx’s picture

    We discussed this with Jacob and I pointed out that we need to ask for password and he said that the confirm screen would be a good place for that. I agree.

    JacobSingh’s picture

    drifter’s picture

    I second taking a look at Backup and Migrate - it works pretty flawlessly for backing up the database. MySQL only though.

    cwgordon7’s picture

    The interface outlined in #7 looks very good. I don't think we want something like Backup and Migrate in core - it would have to be coded for each individual database type, and it would require some file system setup. I don't think there's any reason why we can't provide a hook so that contributions can do that, though.

    leisareichelt’s picture

    Issue tags: +D7UX

    heya

    just wanted to stop by and let you know that as a part of the D7UX project we're going to be having a bit of a look at this issue so hopefully we can work together on this!

    i'm in the process of working on some wireframes for our Modules & Configuration section of the site (ref: http://www.d7ux.org/modules/) the wireframes currently available are pretty out of date but you can see that we're trying to make modules requiring update much more obvious (scannable) on the modules page, and we'll be using the Flooded Harmonica's interaction pattern that yoroy proposed (http://www.youtube.com/watch?v=SMcxz-5XFMs). We're thinking to combine this with a dashboard widget that alerts you when there are modules requiring updates. (no design for that yet)

    unfortunately I'm a little waylaid on some other parts of the project for the next day or so but I hope to get back onto this shortly - I wonder if you think that the general direction we're heading in sounds like it might be useful and if so, whether we can combine forces? Do let me know if there's more info you need that I might be able to provide - I'll definitely have some more wireframes soon!

    JacobSingh’s picture

    Status: Postponed » Active

    Hi Leisa,

    It's great that you guys are on this. I think there is a lot of momentum at the moment, and it would be great if you got involved here on the issue queue to push a final project ahead.

    I think someone (either myself or cwgordon, chx, etc) is going to start building what I've produced above because it is:

    1). Possible
    2). Practical
    3). Ready to work on now.

    If you can provide a wireframe which provides the above at some time in the near future (like a week). I'm sure people could build to that instead.

    If you cannot get something together in that timeframe, it would also be useful if you would comment on what is already there.

    Thanks
    Jacob

    leisareichelt’s picture

    Issue tags: -D7UX +d7ux_8_0

    Jacob - I can definitely get you something within a week!

    JacobSingh’s picture

    Also, be aware of #395478

    And the discussion there about using d.o. as the place to find and install modules and themes.

    This is very relevant to your design.

    If nothing else, it would good to get updates in using an interface similar to what I've proposed above. It is pretty non-intrusive and not dependent on improving the modules page and simulates the behavior of most commonly used update interfaces in OSs.

    yoroy’s picture

    Status: Active » Postponed
    Issue tags: -d7ux_8_0

    If you join doc team you can have img tag:

    Hello for now

    yoroy’s picture

    Status: Postponed » Active
    Issue tags: +d7ux_8_0

    dunno why but this happens…

    Anonymous’s picture

    Please give me feedback on where we are going right / wrong here. One big usability hurdle I see is the lack of key signage and trust. I know there was a plan to match up md5s from d.o. but it's so ugly. I don't know how to fit it in. Also, if there is a MiM attack, couldn't it just fake d.o.'s MD5s anyway? There must be a better way... If we trust update status from d.o. shouldn't we trust the files as well?

    For all that is good and holy, no. ;) The server downloads the package. The user browser uploads the given md5sum. This is done to insure safety. If we simply "trust udate status", all the verification is being done on the server. That means that we are vulnerable to DNS poisoning.

    And the MiM attack that you mentioned: The current system would not be hurt by it. In order to be hurt both the local server and the browser would have to be compromised. In the proposed system, only the server would have to.

    Anonymous’s picture

    Is there any way that I could be of help to you? Instead of working here in the these four issues, would it be of any more use to have the plugin manager issue queue? Also, if it would help, I certainly don't mind giving you CVS access for plugin manager. We could replace the 7.0 branch that is there. :)

    -- J Rogers

    JacobSingh’s picture

    Since this is going into core, we've got to keep it in the core issue queue.

    You can certainly help though :) Our Gardening day is in a week, so I'll be spending all next Wednesday on it. I'll probably also sneak in some work in the next couple days. I haven't heard back from leisa, so I'll probably just go ahead on these designs.

    I think a big thing we have to start on is the key management issue, perhaps you want to start taking it up with dww?

    -J

    cwgordon7’s picture

    Assigned: cwgordon7 » Unassigned

    In the words of dww, this is clearly a team effort at this point. :)

    JacobSingh’s picture

    Close to a patch on this. I'm built it with some obviously faked parts and stub functions. Here is a screencast:

    Sorry for the ugly watermark, I haven't bought the software

    http://pajamadesign.com/update_status/update_process.mov

    Bojhan’s picture

    Subscribing

    dww’s picture

    Mostly this sounds like it's moving in the right direction. One issue to consider:

    The update status release history XML does not currently contain the entire release notes for each release. That'd be way too much data in many cases. so, if we're really interested in displaying the release notes inline in the admin UI as proposed in the wireframes above, that's going to require some new plumbing on d.o. That aspect needs more thought, and perhaps an entirely new issue (or set of issues to coordinate the d.o changes and the UI changes in core).

    Also some release notes are very long so the UI is going to have to be able to accommodate that.

    JacobSingh’s picture

    A couple planned changes to the interface from the video above:

    * Add a "more details" link for each extension which goes to the release notes on d.o. No need to make another series of tubes on d.o.

    * Make the 2nd page less verbose.

    * Remove the "Take site offline" button and just do this by default when they hit the submit which will tell them that they are going to go offline. Provide a confirm form so they know what is happening.

    * Connection details will be a little more complex probably, might end up on new screen.

    * Provide a "You are finished and your site is back online" report page AND a "Your update got borked and you are screwed and your site is still offline page".

    I'll be reposting a new video and a patch for the interface on Wed/Thursday.

    Best,
    J

    JacobSingh’s picture

    FileSize
    19.64 KB

    Oops, wrong one.

    catch’s picture

    Status: Active » Needs review
    catch’s picture

    Status: Needs review » Active

    Nope, that doesn't look like the patch for this issue.

    Jacob - could you post what you've got even if it doesn't work yet? Hard for people to track between this and the install system one if the code isn't visible and that's what leads to duplication of work.

    JacobSingh’s picture

    Okay, version 2.0 video.

    This is actually functional now! Just lacking the running update.php part, but downloading from d.o. and uploading is working.

    I plan to post the patch tonight. Feedback requested.

    http://pajamadesign.com/update_status/update_process2.mov

    I know there are still some spots where the usability / language is not ideal, I'm trying to get to a functionality prototype with the basic UI in place. It's kinda hard to do with a video, but if someone wants to take screens in skitch and mark them up, that'd be really helpful.

    If people feel this is way off base, then let me know ASAP.

    enjoy!
    -J

    birdmanx35’s picture

    Jacob: Couldn't help but notice that in the screencast, "maintenance" is spelled wrong.
    Otherwise: this is beautiful!

    cwgordon7’s picture

    I understand that it's a functional prototype and you've yet to complete the UI. However, I had a couple of concrete thoughts on the interface and wanted to make sure you were aware of them:

    - Why make users enter their information ahead of time? Why not just ask for it the first time, then pre-store everything besides the password for the next time?
    - It would be awesome if we could add a bit of javascript awesomeness that would show the settings form for each of the FileTransfer backends when you switch between the select list.
    - I think the select list might be better off as a radio list.
    - When you post a patch, please include a reusable function for that part of the form so I can use it in part 3 :).

    Otherwise, awesome work, keep it up! Looking forward to seeing your patch :)

    JacobSingh’s picture

    Status: Active » Needs work
    FileSize
    21.74 KB

    Total WIP patch. I'm sure it is FUBAR in many regards. I could enumerate them, but you can just look for yourself in interested.

    I will certainly be cleaning up more as the days go on, but if anyone wants to jump in to crufty, undocumented code with no error handling (which works for me), be my guest :)

    Requires the patch @http://drupal.org/files/issues/filetransfer_cleanup_42.patch.

    Should allow you to replicate the demo. I used permission_select because it has 2 7 releases and therefor works for the demo. I recommend you use it as well.

    Have Fun!
    Jacob

    P.S. If this destroys your computer, I will not buy you a new one, but I will feel sorry for you.

    JacobSingh’s picture

    P.S:

    the actual update.php stuff doesn't run at all.

    Themes don't work

    Didn't test FTP

    Bad creds on the confirm page will throw fatal errors

    dmitrig01’s picture

    The way drush works when installing updates is automatically takes a backup, installs the update, if successful, deletes the backup, otherwise, restores to the backup and reports a problem, and deletes the backup.

    Then again, people have been nagging for backups in core for a long time. Maybe this could use them (follow up patch of course).

    Bojhan’s picture

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

    JacobSingh’s picture

    Status: Needs work » Needs review
    FileSize
    24.49 KB

    Okay, here is another one.

    @Bojhan: Don't worry too much about the UI. It's not going to be terribly difficult to move stuff around if need be later. A lot of this patch is actually dealing with the operational side. We want this in D7, which means that if there needs to be a major UI change, we need to know about it now.

    Currently, the JS enabled form I'm putting up is how @cwgordon wanted to do it. See from earlier patches that I was not in favor of it because I don't see how it will degrade. I still don't see, but he insists that it will, so I'm willing to trust him to figure it out - because he's wicked smahrt hey!

    Requires the patch @ http://drupal.org/node/395472#comment-1763744

    Still needs work.

    axyjo’s picture

    The #395472: Plugin Manager in Core: Part 1 (backend) patch mentioned in #35 is now in HEAD, so it's no longer necessary. (Subscribe)

    Anonymous’s picture

    Most important news first: It worked for me. :) There were a few things that I noticed along the way. (Hope that this is helpful.)

    A) I received the following errors on almost every page associated with the update process:
    * Warning: include(/mydir/modules/update/theme/update-confirm.tpl.php): failed to open stream: No such file or directory in theme_render_template() (line 1132 of /mydir/includes/theme.inc).
    * Warning: include(): Failed opening '/mydir/modules/update/theme/update-confirm.tpl.php' for inclusion (include_path='.:/usr/share/pear') in theme_render_template() (line 1132 of /mydir/includes/theme.inc).

    B) The following error was the result of trying to use FTP to update files that were in sites/default/modules:
    An error occurred.
    Path: /batch?id=6&op=do
    Message: FileTransferException: Unable to change to directory @directory in FileTransferFTPExtension->removeDirectoryJailed() (line 101 of /mydir/includes/filetransfer/ftp.inc).

    Updating files within sites/all/modules worked with no problems.

    JacobSingh’s picture

    @JoshuaRogers:

    Thanks a lot for the review. Regarding the include error, my bad. I forgot to put it in the theme directory, because CVS is such a royal PITA and I didn't manually edit the patch. I"ll do so later today and repost. In the mean time, you can do this to see it correctly:

    cd modules/update
    mkdir theme
    mv update-confirm.tpl.php theme
    

    re the FTP error:

    This is much more interesting. So it worked great for me, so I'll need more detail here. There is a problem with the Batch API wherein, I can't figure out how to add any error handling (we want to stop the batch process and show an error). The only way to stop it seems to be to throw an actual PHP exception. See the Exception handling code in the filetransfer.inc file. We store params and message separately, so t() can be run on 'em later, which is why you are seeing directory() there. I wanted to keep t() out of the Exception handling to make it Drupal agnostic. Maybe the solution (look at the bottom of update.module for the code throwing it), is to do a try...catch like now and then throw a new exception with the message set as t()... but this obviously sucks.

    Anyway, thanks for taking a look, please post your environment info. Also does the FTP error happen with both FTP methods, or just the default?

    Best,
    J

    JacobSingh’s picture

    FileSize
    24.45 KB

    This one should be cleaner (includes the tpl in the right place).

    JacobSingh’s picture

    FileSize
    24.47 KB

    Ah, bugger..

    Conflict with update_finished in update.php :)

    JacobSingh’s picture

    Bohjan insisted that I provide screenshots instead of video, although to be honest it's a bit tedious considering this is a work in progress and I did provide clickable wireframes at the first go and didn't get negative feedback then, so this may be the last time I do it.

    At any rate, here they are.

    update_process

    update_process-1

    update_process-2

    update_process

    update_process

    Video coming tomorrow morning to show the JS interaction.

    -J

    Anonymous’s picture

    @JacobSingh: If the module to be updated was in a location other than sites/all/modules then updating would fail. Both FTP methods gave an error. Unfortunately, I don't remember at the moment if the errors were the same. It's rather late here. I'll try to post complete setup details tomorrow after testing the new patch. (Right now I just want to test my pillow.)

    I hope to spend a little bit more time familiarizing myself with the code tomorrow.

    dmitrig01’s picture

    Here's my code review (no functionality review, and this was just a first pass - this only picks out the worst I could find.

    +<br style="clear:both"/>
    

    Never use br

    +  $form['submit'] = array(
    +        '#name' => 'submit',
    +        '#type' => 'submit',
    +        '#value' => t('Continue'),
    +        '#weight' => 100,
    +  );
    

    Indent is 2 spaces not 6

    All comments are // followed by a space and a capitalized sentence ending in a period.

    +  } else {
    and
    +  } catch(Exception $e) {
    

    put the } on it's own line

    +    $form['message'] = array (
    

    no space before (. this happens multiple times

    +  
    +  /**
    +   * This is turning into a clusterfuck.
    +   */
    

    Please stop it from being a clusterfuck or at least remove the comment.

    +function theme_update_status_project_status($status) {
    

    indentation of the cases are wrong

    +function theme_update_status_project_status($status) {
    

    The key is really ugly. Can we either not state it or figure out some better way.

    +    dd('Coppied the dir ' . $extension_source_dir . 'to' . $extension_destination_dir);
    

    1. what's dd? certainly nonstandard. 2. copied is spelled wrong

    +  sleep(1);
    

    why?

    +  return variable_get("update_filetransfer_connection_settings::{$filetransfer_backend_name}", array());
    

    People can call this themselves if they want. also use _ not ::. Also should be written as:

    +  return variable_get("update_filetransfer_connection_settings" . $filetransfer_backend_name, array());
    
    JacobSingh’s picture

    @dmitrig01: Thanks for taking the time to cull through the code. Yes, there are lots of messy bits, I'm well aware.

    This is totally a work in progress, I appreciate you finding all this stuff, and myself (or someone else) will fix it I'm sure. At this point, we're more aiming towards a functional prototype which provides a sensible UI and business rules. I'm sure code cleanup will be another mammoth effort. That being said, if you feel like fixing these issues and rolling a patch, I'd be more than happy to take a look.

    I'll probably get to them over the weekend or on my next Acquia dedicated community day (next Wed).

    Thanks!
    Jacob

    JacobSingh’s picture

    Status: Needs review » Needs work

    Just as an add-on:

    The way I typically like to work is from the user to the CPU. :) That means: mission statement, wireframes, and then some mocked up screens, and then some prototype code, and then usually amendments to APIs or building new APIs, and then some more polished UI, and then code style related stuff.

    This way, if there are problems with how something is being implemented it is known about as soon as possible and we don't waste time down the chain. We don't write APIs that don't actually serve the user's purpose, because we've established the interface already. We don't refactor and clean code that we might throw out, because we're still writing the APIs, etc, etc.

    That being said, it's just my style, it's worked for me, but I'm happy to try other ways of working as well. At this point, I'm most interested in UI reviews and feedback so we can get consensus there and present to Dries/WebChick, and if people want to add the rest of the functional piece:

    1. Getting the DB updates to run at the end of the process.
    2. Adding error handling to the batch API process (which it doesn't have at present).

    And all the other stuff related to package signing, dependency resolution, storing modules in alternate locations, etc which should happen.

    Best,
    Jacob

    yched’s picture

    Quick remark: batch_process(); is not needed in update_update_form_submit() - as outlined by the inline comment just above :-)

    int’s picture

    I'm in.. (Subscribing)

    Paul Natsuo Kishimoto’s picture

    Subscribing all over the place.

    dmitrig01’s picture

    FileSize
    27.25 KB

    changed styling of the first page, but it's not working perfectly (yet)

    JacobSingh’s picture

    @dmitri

    Thanks for jumping into the fray. Awesome to have you on board. Here is my rant now (not personal, just trying to tease out the best idea):

    I don't see why we would want to go back to the huge amount of css and markup required to produce the previous layout. Now that we have the #type = tableselect element, we can reduce a lot of code, and use a standard form element which users will be used to seeing through Drupal.

    Here's what I'm talking about:
    http://img.skitch.com/20090704-qfmkh72emxyf955wxm68waj8ge.jpg
    vs
    http://img.skitch.com/20090704-tm9qpntks24erfew3yg1878fj3.jpg

    Besides less code, the tableselect format has the following advantages:

    1. More themeable. It's not a bunch of custom markup, just a table.

    2. You can select all rows in one go.

    3. If we want to add JS handlers to clicking on rows to show contextual info, it is already setup.

    4. More standardized. Most package managers use this format [] | name | version. See OSX, Ubuntu, RedHat, Adobe, etc. Firefox has a slightly non-standard one which the old design and this new one are attempting to emulate.

    What is the point? One could say one is more aesthetically pleasing or the other. I don't know how you can prove this. I think the tableselect model is consistent, a proven UX pattern and used throughout our product and others.

    The "other" one is lots of custom markup and CSS not used anywhere else in Drupal, and its pattern is only found in Mozilla products. In Firefox, it also is much more functional because it has graphic elements (which is probably why it took its shape in the first place - to provide icon branding for add-ons).

    Why would we want to add more code to provide a design which IMO doesn't really look any better and is a less common UX pattern? I'm willing to be wrong here, but we're also very close to being done using one paradigm which was well documented in screenshots and movies for the past 3 weeks that anyone could have commented on. If we're going to spend more time on this by completely redesigning, I'd be hard pressed to justify that vs. getting the finishing touches in to get it all functional.

    Best,
    J

    Anonymous’s picture

    After applying #40, here are my findings:

    --Critical--
    A) The callback for admin/settings/updates/server-settings is defined as update_settings_filetransfer. Unfortunately, this function is never definied.

    B) With an outdated module placed in sites/all/modules, both FTP connection methods were able to remove the folder to be updated, but immediately threw an error while trying to copy the new files. Since D7 couldn't find the modules that it was trying to include, all subsequent page load attempts were met with the white screen of death. Apache threw the error "Failed opening required '/XXXYYY/sites/all/modules/permission_select/permission_select.module'." Bringing the site back to life require readding the removed files.

    According to error_log, the error thrown after removing but before replacing the files was "Call to undefined function dd() in /XXXYYY/modules/update/update.module on line 772." After removing the two instances of dd(), the update process worked fine with the two FTP installation methods.

    C) PHP complained that it still couldn't find '/XXXYYY/modules/update/theme/update-confirm.tpl.php.' I suppose the patch doesn't place that yet though.

    -- Aesthetic --

    D) The last "Updates" in "Home › Administer › Site configuration › Updates › Updates" is probably not supposed to actually appear on the navigation menu.

    E) I seem to recall the "Updates" report page having a manually check link at one point. Though one also exists at status report, it seems more natural to me to see one on the page that displays the available updates also.

    F) I'm not sure that "Reports" is the most logical place to look to update the system. The list of available updates does seem okay there, but it doesn't seem like an intuitive place to go to perform an action like updating. Maybe "site building" would be more natural for something like upgrading.

    G) I am inclined to agree about the table view. It does seem to be a more consistent form of interaction.

    -- Finally --

    It's roughly 3:12am here. I'll finish posting everything else when I'm not ready to fall asleep on the keyboard. ;)

    JacobSingh’s picture

    A). Totally, that's deprecated, should be gone. Was for when settings were in their own location

    B). d'oh those are devel debug statements. I suggest you install devel while testing. Not that I shouldn't remove them before posting, but will certainly forget, and you'll want it anyway. dd() logs to a file file_directory_tmp() called drupal_debug.txt

    Side note: Are there any issues to get a proper logging system into Drupal with verbosity levels and output handlers / formatters? That would alleivate this need, so we could leave debug statements in code and just change verbosity.

    C).PHP complained that it still couldn't find '/XXXYYY/modules/update/theme/update-confirm.tpl.php.' I suppose the patch doesn't place that yet though.

    That's a CVS problem.... I thought the diff would create theme/whatever for you... I guess I made it wrong. See the diff file, it's right at the top, should be obvious what is supposed to happen. Not sure why it isn't.

    D). Good point. I'll make a note to squash that.

    E). Yeah, check now link is also a good idea. Although I think we refresh when that page is loaded automatically, no?

    F). I've thought about the menu location. Here's my idea: It doesn't matter much, because people will only go there when they start seeing a "update me" nag message 99% of the time, so it could really sit anywhere. Reports is "OK" but I agree it's a bit confusing.

    G). Well, I agree (duh).

    Thanks for the feedback here. I'll try to roll a patch with some of this stuff today. Would like to get consensus w/ Dmitri though on the format of the avail updates page before we have dueling patches.

    Best,
    J

    dmitrig01’s picture

    FileSize
    25.88 KB

    progress

    dmitrig01’s picture

    FileSize
    25.09 KB

    Ok, now a working patch!!!

    dmitrig01’s picture

    Question: Why does all the getting, then all the copying, then all the installation happen? Why can't it just be per-module?

    joshmiller’s picture

    Status: Needs work » Needs review
    dmitrig01’s picture

    2 different interfaces:

    Apple - http://img.skitch.com/20090705-bd4542rp6fh2w3tiuynd5xm88f.jpg
    Firefox - http://img.skitch.com/20090705-tuds9d4k61f7ja98kdpp33j7fp.jpg

    Mine is modeled after Firefox, Jacob's after Apple.

    Anonymous’s picture

    A) Once more, the Firefox approach seems to be inconsistant with the rest of the Drupal interface. This causes it to be harder to visually parse for data. If this approach is to be taken, I suggest that all other forms in Drupal be changed for consistancy too... ;) (One down. 57 Billion to go.)

    B) Upgrading when the module was in sites/default/modules rather than sites/all/modules caused these two errors when using the FTP installation method. "Unable to change to directory /home/XXX/YYY/sites/all/modules/permission_select" and "FileTransferException: Unable to change to directory @directory in FileTransferFTPExtension->removeDirectoryJailed() (line 101 of /home/XXX/YYY/includes/filetransfer/ftp.inc)." FTP using file streams, however was able to upgrade without any problem.

    My Drupal installation is at /home/XXX/YYY/d7.
    My FTP server is chrooted so that it appears to be in /YYY/d7.

    I believe this is the same setup used by most shared hosting providers.

    Bojhan’s picture

    @JacobSingh As I said before I think it would do this issue well if we split out the UI discussion.

    @dmitrig01 I don't think we should be looking to much at our fellow stars - because the space we are in is vastly different in terms of use case.

    A couple principles I believe should hold strong for this page, no matter what direction we go :

    1. Security updates should be prominent.
    This is what people should do - and this is what we are striving this page to achieve.

    2. People do not update everything
    As much as we might want people to update everything, usually people are pretty hesitant in updating their whole site. If we can guarantee it won't completely kill their site - it's oke. But I don't think we can give that yet, so lets assume this page will always have at least a bit of content - beyond 1 or two rows.

    3. Developer information is not very important.
    We have been trying to push a lot of developer information trough the module page as well - and from the screen shots I see it happening on this page as well. I think it's not relevant, developers will be able to find it on d.o. If any information does have to be put there, at least put it in a harmonica or similar.

    Senpai’s picture

    You mean an accordion? :)

    @bojhan, I agree with #1 and #2. Exactly what I would have said.

    @DmitriG, I also wonder why this module does not download, delete, update, and install one module or theme at a time. It sure seems like a better approach to me. Not to mention being more robust in a failed download, server unavailable, corrupt FTP session, whatever. As to the two images in #57, the checkboxes have to be on the left cause that's just the way its done. :) In addition, a table display is much more consistent with the rest of core, so we should go with that.

    @all: Put the critical updates in a box at the top, maybe with with a special color. Put the non-critical updates into another box right below that. Don't separate modules from themes, that can be a designator in a column. Let each row show a checkbox, name, version, new version, and a link to the module/theme's release notes on d.o so that people can see what's changed between version x and version x.1

    JacobSingh’s picture

    @Senpai: Thanks for your feedback and welcome!

    I'm going to try to summarize where we are at right now in terms of issues and actions going forward:

    1. Is this a UI issue.

    IMO, yes. I didn't intend for this to be a code review for style or architechture really at this point. I started with wireframes, then moved on to a video with mockups, then moved on to *basic* prototype. This is still in a UI phase as far as I am concerned. When we agree on what it will look like (and please, let's not bike shed too much here, time is precious). Then I suggest we delve into the architechture more. It's close now, but I don't think we need to obsess at this point. I don't see a need to build a new ticket. It's totally reviewable given the patch above and screenshots, etc. That will just split up our audience, and require lots of back references.

    I'm not going to address the issues brought up by dmitri and Joshua re: module installs to "site specific" folders, order of operations, etc not because they aren't important or correct, but because I'd like to clear the UI first and be done with it. (see above). Is that cool? I mentioned 100 times, this is prototype code, the goal at this point is to finalize how it should work, not if it works. Only make UI changes until we have decent consensus and/or a committer's approval.

    2. Firefox vs tableselect. I think there is a clear agreement here in favor of using tableselect. Dmitri, if you really strongly agree, please let us know and build a case, we can continue to discuss, but if you are cool with trusting the group on this one for now, please indicate it so we can continue. Stays w/ tableselect

    3. Security updates on top. I'm okay making them more prominent from my previous wireframes, code, etc. But I don't want to bikeshed this. Senpai: Can you make make a mockup or even a wireframe drawing of some sort to demo what you mean? If we don't get this in, should it block progress? Needs a wireframe or something

    4. Accordion widget: I had a frame below to show release notes as is SOP in Ubuntu, OSX, etc. I took it out because it requires additional plumbing on d.o. I think @Senpai, @Bohjan are right, a link to release notes is good enough. Needs a release notes column w/ link

    5. We need to degrade the connection settings form. I'm still waiting for cwgordon to do something here, I don't know what he had planned, but it will require additional UI elements. It doesn't need to work 100%, but the visuals for it need to be in. Get on Charlie's case to produce this, or someone else needs to

    SO. Do we have any other UI related issues? Do we all agree on my Actions required to continue? Do you guys think we can present a "finished" prototype patch / video to Dries and webchick this week and then start fixing stuff?

    JacobSingh’s picture

    @dmitrig01 in #54:

    I like what you've done with some of the code. Could you make a re-roll using #type = 'tableselect' if you're okay with that?

    I'll try to get to it today myself if I can, but would like to base work off of your latest. I'm going to work on degrading the form and adding the release notes link today, let's see if I get to them both.

    -J

    Bojhan’s picture

    @JacobSingh 1. I might have been confused, by the #30 code related comments to call this an UI issue. It's just not good practice to attract designers to look at this, but anyways - I am not going to discuss this more.

    2. Tableselect should be fine, but yoroy adressed a critical point that we need to carefully look at the checkboxes whether they make sense.

    3. Put Security updates in a fieldset, and then the recommended updates in a fieldset - it might not be pretty but then you get an idea of what we mean.

    4. I think release notes are primarily developer information, since its rarely written for normal people in mind. I'd say either hide it, or show it in the accordion interaction - either way it should not be prominent.

    5. Can we change the workflow, so that you only have to fill in the setup connection with your first module install? It seems a bit silly asking them to setup their connection, if we don't intermediately do things with it.

    JacobSingh’s picture

    @Bojhan:

    Here's an update, and updated Actions needed for completion of UI.

    I understand you are ambivalent about #1

    #2: We don't really have time to not have a solution in place. If we're not using table select (which is used everywhere else in Drupal), we need a mockup. Please respond in the affirmative "Let's do this" instead of the negative without a proposal. Action: Unchanged, using tableselect

    #3: I can work on putting security vs other updates in two different tables, but does this really improve usability? Then the user has to check off two checkboxes to get them all? How about if we just sort by status and bring security ones up to the top. I think the combination of the key and the big red icon should be enough to alert them. Action: Sort by Update Type, then by name

    #4 Release notes. Please read my previous comment if you have not. As I mentioned, we are linking to them on d.o. not showing them inline (accordion or otherwise). Linking is "good enough" and we would force a d.o. change to include them in update_status. Not worth it IMO Acition: As before, make column with link to release notes

    #5: workflow on entering server connection details. We remember everything the user entered EXCEPT their password - which we will never store. So they will have to enter this every time. We also remember their connection type they used. We could potentially just expose the part they need to re-enter (the password), but that seemed to be out of context and the space savings didn't really enhance usability at all. Action: Please provide clarity on what you are expecting here or if you misunderstood the current functionality.

    Are we on track to resolution still? Anything else?

    Bojhan’s picture

    Sorry, but I am a bit confused why are we trying to get to a solution beforehand - without even seeing the interface changes?

    2. Let's do this, and see.

    3. I think it does, I think placing them in the same table will not increase their importance.

    4. I still don't think we really have to link this.

    5. So the step of setting up your connection before you actually did anything, is removed?

    catch’s picture

    afaik the connection setting appears every time you try to do something - but pre-filled with everything except the password after the first time, I don't think there's a prompt to complete it until you take an action though. That seems fine to me for a first pass, maybe we can use the 'expandables' patch once this is in to show the other settings as summaries rather than form fields, but that shouldn't hold this up for now.

    Quite like the idea of having the updates ordered by severity as opposed to the current list or even security / everything else - if a release is unpublished - either because it's unmaintained or maybe has a serious logic/syntax error (which in many cases are a lot more likely to hose your site than some of the XSS issues we release SAa for) that can be just as important as a security update - and it'd show just below these (I think) if ordered by severity.

    I don't think we need to link to release notes - if we link to the project page itself on this form, that's plenty.

    JacobSingh’s picture

    Sorry, but I am a bit confused why are we trying to get to a solution beforehand - without even seeing the interface changes?

    So I don't waste my time coding up / taking pictures of / making movies of something which people are going to argue about forever and undo.

    Back to consensus on critical path:

    2. Settled, unless dimitri has an issue, using tableselect.

    3. One table vs two for types of updates. Bohjan: Please wireframe this. I don't get what you're talking about. This looks to me like two tables, with two sets of column headings. I think it will just be unclear that we are talking about one form this way. It also, as @catch is saying, tells people that recommended updates are really not important, when typically they are.

    4. Release notes: Fine, no link... I think we have a decision!

    5. "So the step of setting up your connection before you actually did anything, is removed?"
    Yes, see the latest screenshots that you asked me to post instead of a video: http://drupal.org/comment/reply/395474#comment-1767136

    We are cool w/ the connection settings? - Please say yes.

    So now we are down to one discussion unless people have more (Last and final chance to complain):

    1. One table sorted by update severity vs two for list of updates.

    Any more objections before we roll a final interface?

    yched’s picture

    Er, a link to release notes is absolutely critical IMO. There's no way I'll update a module if I don't know what the changes are.

    JacobSingh’s picture

    Okay, so we have two open issues:

    1. One table sorted by update severity vs two for list of updates. Status: needs consensus

    2. Release notes link Needs consensus

    Bojhan’s picture

    @JacobSingh As I said before, we do not get to a final design unless we see the changes.

    1. Spoke with catch, that's oke for now (one table). The security updates will need visual weight, like a red background.

    2. Since changelog of a module is usually aimed at Developers, I really don't see the point - catch mentioned linking to the project page. If we do that it would be fine.

    JacobSingh’s picture

    @Bohjan: I appreciate your help, and here is how I like to work. Just because someone writes code doesn't mean they should get the final say. The best idea should go in. I think doo-ocracies suck. So I'm willing to write the code, and I'm willing to herd cats. But I'm not willing to have 1 cat say "it should be like this", I code it up and then go back to herding. To have 10 more cats say do it in 10 more ways. That's too much work. I'm also not going to just plow my agenda through, and have patch wars, that's too much emotional energy and a waste of time.

    That's why I like to start w/ wireframes. Less work, but the point gets across. Also, by discussing the issue in detail and coming to consensus, we can avoid doing work twice. For instance, if dmitri had discussed changing the layout back to the old update status format before doing all that work, it could have been avoided, as all reviewers prefer the standard element.

    As someone with a job and a family, I need that time, I can't waste it. That's why I'm bothering with this.

    What I'd prefer is that for every comment you have that deviates from the screenshots above, you take the time to actually use skitch and mark it up. Dmitri did this with the two designs, and we quickly found resolution.

    Personally, I like have the release notes links there, I think the point about not upgrading unless you know what the upgrade is about makes sense. I've used Drupal long enough to know that upgrading is not something you want to do unless you have to :) Also, I think that changelogs will become more user centric when people realize normal users will be looking at them.

    How about if we do this:

    Available updates | dev7

    So to summarize status:

    1. Severity of updates: One list, sorted with security at the top, different background color for security updates

    2. Release Notes: Link in recommended version column , project name links to project page.

    Are we in consensus now?

    catch’s picture

    Just spoke with Bojhan in irc, looks like we crossed wires a tiny bit - he thought I was suggesting this (which I hadn't thought of before but am now):

    Currently update status shows you the version of the module it's recommending an upgrade for - say Drupal 6.18 - let's make that text a link to the release node. That way we keep the information available, but have things much more compact. It also makes it clear exactly where the link goes to (the new release or the installed one).

    catch’s picture

    Cross-posted with Jacob.

    Looks like we agree :)

    But, no _blank - users can use shift-click or right click to open in a new tab, or we can show the release notes in an overlay at a later date., but forcing a new window like that is bad behaviour.

    Paul Natsuo Kishimoto’s picture

    The Organic Groups groups/myunread view has a "Replies" column containing two things:

    1. The total number of comments on a node ("741"), in plaintext.
    2. The number of new comments ("32 new"), as a hyperlink to the first new comment.

    I like this format. It conveys more information while remaining compact. Maybe, to inform those who aren't aware that there are release notes available, in the Recommended Version column:

    1. The recommended version ("7.x-3.4"), in plaintext.
    2. The text "changes" (or similar), as a hyperlink to the release notes.
    dmitrig01’s picture

    Could someone take more screenshots of package managers? I don't think the "installed version" column is necesarry at all. Status should become a background color. To paraphrase Bojhan, "if you need to explain your icons, you're doing something wrong." Additionally, the type => module column is ugly. I'm not sure if it's necesarry - it's like the split of a Mac OS X system update vs an update to an application vs an update to some sort of framework - in the apple installer, there is no distinction. We could distinguish by saying Permission Select (module) for example, that might be prettier.

    Paul's suggestion was "7.x-1.3 (changes)" or something like that. Opinions?

    JacobSingh’s picture

    Ugh... please. Can we just finalize this? I feel like this roundabout is going to kill what could be the best feature in D7.

    We just had everyone agreeing. I asked 5 times if anyone had other issues to thrash out.

    Re: the icons, explanations, etc. You need it. Coloring it red means nothing. Something has to say "Security".

    I'm okay with the run-around, but let's deal with one issue at a time, just thrash it out in detail, with proposals, which have screen shots and just get it done. This is totally a bike shed at this point.

    I will summarize the issues again, but I've got to have a life.

    -J

    catch’s picture

    The current update status UI has the following for security updates:

    red background
    icon
    security update: (vs. recommended update for others)

    So let's keep it like that - and if we need to change, we can open a issue somewhere else just for those changes.

    Similarly, current update status makes the new version a link to the release node - let's keep that the same too.

    If there's serious disagreement, then it's fine to discuss changes - but in another issue, not this one - which is about automated updates, and any redesigning of the page should be really minimal to make it action oriented (because we can do things on it now), and to not break it too much.

    @Jacob, I think this is why Bojhan wanted the issues split ;)

    JacobSingh’s picture

    Sure, we can split issues, but then we're just bikeshedding somewhere else and increasing the risk of missed communication. This issue has all the wireframes, discussions, patches, etc. Why create a new issue to back-reference them? This isn't an issue of what web page the discussion is held on IMO. It's just that everyone has new things that "absolutely must be changed" that they didn't think of a month ago when they saw wireframes, or a week ago when they looked at it. Let's aim for consensus, not nitpicking every detail.

    At a certain point, you have to just say "enough". It's perfectly usable in its current state, I think that's hard to argue. Why are we splitting hairs about red backgrounds? No one even mentioned this in the past month despite there being ample opportunity.

    I'm all about consensus. If it's red backgrounds, fine, let's do it. Let's just get something done though. At the end, only 2 people can commit. Let's get them to say "good enough" on the UI, yes?

    I'm going to post one more patch, which will be my best understanding of where the discussion is at tomorrow. After that, I suggest we limit ourselves to 2 additional rounds if needed to sort the UI. Is this okay with people? Can we at least agree to limit the bikeshedding before we start talking about the color of the shed to keep the bikes of the people who come to build the bikeshed?

    :)

    -J

    Bojhan’s picture

    http://bojhan.nl/updatepage.jpg

    So here is my take on it, dmitrig will work on an implementation this afternoon. My main effort was to keep it as simple as possible. I think I have addressed almost all edge cases. The tags should probally not be bold.

    Module name links to Module page
    Version number links to changelog page

    @JacobSingh Please, just take it as it goes. I understand your concern, but this is a very normal design iteration process - we are far from bikeshed. We are going just fine.

    Anonymous’s picture

    @Bojhan: Like what you made, though it does look like we're playing "variations on a theme" now. Also, putting two types of information in one column doesn't seem like a good idea. (Name and update type.)

    For the two open questions:
    1. One table displaying all updates is enough. It doesn't even have to be sorted, so long as security updates are easily distinguishable from other updates (which the skitch shows they are.)

    2. Release notes link really does seem like a good idea. An overlay sounds like a good idea. However, a new window makes a great deal of sense also. Either solution is valid / usable.

    Senpai’s picture

    @jacobsingh in #61: Here's that wireframe you asked for to outline the difference between Bojhan's mission critical updates and non mission critical updates being in two different sections but still part of the same table.

    If there are any changes desired, ping me and I'll change anything you guys want. I just want this in core sooo bad! :)

    Senpai’s picture

    In regards to Jacob's 64; point 5:, I've added a field at the bottom of this form for the user to re-type their server's FTP pass. This will prevent visual overload by having to have all the connection information at the top of the page, and if the FTP connection fails upon submit, all the checkboxes are still checked so we'll just redirect the user to the previous screen and ask them for all their connex details again.

    Senpai’s picture

    Messed up the image attachment in #82. Trying again.

    JacobSingh’s picture

    1. One table vs Two: It's a super pain the ass to make this design, because using type=tableselect will almost certainly break, so if we have two tableselectes, they won't sort, or select all on both sets... If we use one, putting the header in the middle is another PITA. Can we just KISS ?

    2. The connection details:

    I'd really prefer to keep this on its own page. I have two reasons for this:

    You want to show a confirmation page which says "BACKUP YOUR SITE" and "I'm TAKING YOU OFFLINE NOW". I don't think putting that inline with the checkboxes is wise. So we'll want the page anyway. It also saves the extensions you have selected if your connection fails.

    The user is doing one thing at a time. I also think this is important for usability / safety. Showing the entire form (as it is) on the 2nd page ads context. Also, since it is the only pace to change connection settings, if you hide the form entirely, then you will need to make a UI device to show it again to let them change things. If we have plenty of realestate, why would we do that?

    I'm not going to block consensus here, but I'd like to ask if it is really worth opening this can of worms. I'd prefer to just make the current design degrade and commit to it. It seems logical enough, and in the past month since I posted wireframes, no one has complained.

    Best,
    J

    Anonymous’s picture

    I'm inclined to agree.

    #1: A second table does add another degree of complexity both in terms of code and usability.

    #2: I really don't believe that the password field belongs on this page. There is no guarantee that the user has configured an installation method before. Even if they have, they might want (or be forced) to use a different installation method for some reason. This means that the user will still need to be given the choices for the rest of the installation methods.

    Viewed through this light, I'm inclined to think that the password should go with the rest of the information relevant to login.

    dmitrig01’s picture

    FileSize
    23.34 KB

    Implementation of bojhan's #79.

    dmitrig01’s picture

    @JacobSingh - why are all the modules gotten, then all the modules put, then copied? Can't it be per-module?

    Senpai’s picture

    So you're saying that after the administrator submits this Available Updates form, they'll see a second page with their connex details and a warning to back up their site? Like the page linked from comment #7?

    If so, then there's no reason for the password form to be on the first page. I was thinking that this was the *second* page the administrator sees. My bad. Let's put it all on the same page, but make it comprehensible. After all, "...if you hide the form entirely, then you will need to make a UI device to show it again to let them change things. If we have plenty of realestate, why would we do that?".

    Try number three.

    dmitrig01’s picture

    It's probably easier technically and visually to have table - also why not split conection settings onto two pages?

    catch’s picture

    I think it's better to have the connection settings on two pages - if I just want to check the status rather than do updates, then adding the settings to that screen adds just an extra thing I need to look at.

    Here's a screenshot of the current patch:

    I think we should go ahead with this interface for now.

    Dries’s picture

    Everyone has had its say, so here are the decisions after carefully reading up on the issue:

    1. We'll use one table with color-coding (see bojhan's mockup).

    2. We'll drop the icons proposed by Jacob.

    3. We'll link to the release notes.

    4. Connection settings go to a new page for now.

    We can iterate on this in follow-up issues but let's put this stick in the ground for now. Thanks!

    JacobSingh’s picture

    FileSize
    22.42 KB

    Here is an update:

    Only local images are allowed.

    Requires: #512308: Add the ability to provide attributes to rows in table select to be committed because tableselect doesn't allow different classes on different rows yet.

    Otherwise, everything UI wise is the same.

    here is a patch which also includes a couple bug fixes and incorporates some of dmitri's cleanup, a few improvements mentioned earlier and the update above. Still broken prototype code and still working on the UI primarily.

    UI Needs
    We need to degrade the settings form. I will track down cwgordon and get his idea, but if I can't get ahold of him, or his idea doesn't pan out, here is what I am thinking:

    Only local images are allowed.

    Basically, non-JS user sees radios like this:

    o SSH
    o FTP
    o FTP with streams

    They don't see a connection form (unless they have already connected once before).

    When they hit submit, it brings up the desired connection form. The radios are hidden and instead of a select box, we show a label on the fieldset like this:

    Change connection type
    ---FTP connection settings--------
    |
    | Hostname: _________
    |
    | Username: _________

    When they hit the change link, it will take them back to the radios.

    JacobSingh’s picture

    damn, cross posted :(

    JacobSingh’s picture

    I missed Dimitri's patch unfortunately amongst the posts in the past few hours.

    The big difference AFAICT is that he's using a custom theme function and I'm using the tableselect element with the patch provided above (which should be committed today). And I made some other changes to the code elsewhere. He of course, also remembered to remove bogus files like the css, etc which I did not :)

    @dmitri: are you cool with using my latest and also removing the CSS code?

    Best,
    Jacob

    dmitrig01’s picture

    It looks like you threw out most of my code. Personally, I support my patch, because it's without all the code style and documentation errors. Yours is basically like mine, except mine moves the type into the first column and colors security updates red. Additionally, code wise, mine uses the very standard drupal weights system, while yours uses uasort and create_function (which is slow as heck).

    I also don't see what the big deal about using the tableselect element is. We gain a few lines of code. That's ok. The UI convention remains, I'm just coloring the table rows differently, and tableselect doesn't let you add a class. Also, I if you read closer, the CSS still exits, there is just very little of it.

    My patch is an exact transcription of Bojhan's mockup.

    Also, my patch degrades in the following way. You see a select box for connection type, and there are fieldsets for each connection type. This interface is sub-optimal, but any degradation would be sub-optimal, so it's ok.

    In summary, I don't support Jacob's patch, and I much prefer my patch.

    dmitrig01’s picture

    In IRC, Bojhan also told me that Dries reviewed his mockup and liked it. Another reason to use my patch.

    JacobSingh’s picture

    @dmitrig01:

    Please chill out. I didn't see your patch, I didn't throw out any code. I pinged you on IRC before you posted it, but didn't hear back, If I had, I wouldn't have done any work.

    * I actually fixed every coding error btw (through coder) but something still need docs.

    * I also implemented a couple things you mentioned earlier and reformatted to how you had done things earlier, I tried to merge those patches. I didn't mean to throw out any code. I hate it when people do that without discussing first.

    * I'm also doing it without an external theming function.

    * I also fixed the default connection method once saved.

    * I think using tableselect is relevant. It's a lot less code, and it uses a consistent UI for a consistant purpose. The referenced patch will be committed shortly.

    *Your "degradation" solution does not work. Degrdation does not mean having a form which fails. It has to work, it doesn't have to to be pretty, but having user try to connect to SSH over port 21 is not accessible. I've developed sites like http://amnesty.org which have to pass level 2 accessibility standards, trust me, this wouldn't be allowed.

    Honestly, just do whatever you like. You wanted to help, and instead you've decided from the beginning that you only wanted to criticize and divide. You are obviously angry, and I'm not interested in being angry; I'm sure you'll figure out the best way to do it. You're a very smart young man, if you feel passionately about taking the work I've done to completion on your own, go for it.

    I have dedicated a lot of time to this issue and energy on a consistent basis, and am sad to let it go, but I don't have the time to engage in this kind of dispute. Let me know if you want a review in the future.

    -J

    Senpai’s picture

    Ok, here's the latest wireframe with Dries' suggestions from #91.

    1. We'll use one table with color-coding (see bojhan's mockup).

    2. We'll drop the icons proposed by Jacob.

    3. We'll link to the release notes.

    4. Connection settings go to a new page for now.

    So we've now got one table with one header and one sortable column, but themed into two divs with a pair of foreach() or similar. Also, I've shown the error states because I'm afraid we haven't thought about the administrator's workflow if both a module update were to fail and their FTP password isn't valid. Then there's the problem of error state in a single row of the Available Updates table coupled with the 'normal' visual state of critical updates, both of which will look like errors if we go with Bojhan's latest mockup in #79 (http://bojhan.nl/updatepage.jpg)

    Anonymous’s picture

    We're likely to be debating this when Driplet XVIII comes out. (In the future, after version Drupal 10 came out in 2018, the name was changed to Driplet. Oh, and we moved back to roman numerals.) ;)

    One last suggestion on #98: Can we go with just a table? No reason to do seperate divs. As long as the user can tell the difference, it's okay for them to exist in the same table. Doing otherwise would probably be more of a confusing to newbies than having them together. No reason to go stylized.

    My two cents.

    dmitrig01’s picture

    @JacobSingh - I'm sorry about that. it just looked to me like it was starting form your code, because in my code all the comments are properly formatted (slash slash space capital letter and then ending with a period). Additionally, I skipped down to the bold part of your issue, so I didn't realize tableselect could do that. Since it soon will be able to, I think that using a tableselect is fine. However, let's get my patch in first, without that, and then once your patch for tableselect in, we can use that. Alternately, if the tableselect patch gets in first, I'm fine using that in my patch.

    Lastly, the form works. It's really ugly, but it still works.

    I'm working on ATM 1) testing if security updates work, 2) making the update_N functions run.

    JacobSingh’s picture

    @dmitrig01: As you take control here:

    Also note the at I fixed the problem of hardcoded module and theme paths to sites/all/themes.

    I also did as you suggested and stopped ordering the operations in batches, but process them extension by extension. I first did this, thinking about update_n and using update.php. But I can see update_do_one should suffice and not much duplicate code.

    You may want to grab those out of my last patch and incorporate them

    Btw, I have prototype code to run the update_N functions if you want it.

    Good luck!

    Jacob

    dmitrig01’s picture

    I'd like to see the prototype code. Also, I'm glad you solved the path problem, i'll be sure to incorporate it.

    Anonymous’s picture

    @dmitrig01: Jacob's patch was just committed to core. Thus, tableselect is can be used on the first iteration. :)

    Senpai’s picture

    And just how was Jacob's patch committed to core without this issue being updated by either Dries or Webchick? And where are the tests that prove this patch works? I didn't see any tests in Jacob's original patch, and Dries + Webchick (a.k.a. DrieChick) would never have committed such a significant patch to core without adequate tests. That would be wholely stupid of them to do.

    You must be joking with us. I get it. :)

    Anonymous’s picture

    :p Sorry. I suppose that you were supposed to read my mind. ;)

    What I should have said is that Jacob's tableselect patch that Dmitri was referring to in #100 has just been committed. Thanks for catching that. As it turns out, sometimes even the preview button can't keep us from making silly mistakes. :P

    dmitrig01’s picture

    Check the commit RSS or core. it was committed.

    dmitrig01’s picture

    Component: base system » update system
    FileSize
    30.65 KB

    Incorporated the two patches. Security updates work, I tested them.

    Running updates doesn't happen automatically, waiting on Jacob's prototype code. Other than that I need reviews on everything that's happening in this patch. It's starting to come together and it's coming together awesomely!

    JacobSingh’s picture

    FileSize
    30.44 KB

    Hey folks,

    @dmitrig01: Nice work! This is looking good.

    Here is an update which includes some code which *should* work for updates. I've not really tested it, but I think it's kosher.

    I saw the update.php change you made, I think that's a general improvement, but a bit risky in terms of delaying this one while people test it IMO. What do you think about just duplicating the batch function? It doesn't do much anyway. Another option is an update.inc like install.inc to for housing these functions... anyway, you're call. I'm going away for a couple days, looking forward to where we are at on Monday.

    Best,
    jacob

    yched’s picture

    I did not test it, and might very well be overlooking something, but I fail to see how running _update_get_schema_updates($project) when setting up the batch (that is, before the new files have been retrieved) can provide an accurate list of update functions to be run for the new releases :-)

    Note that you can batch inside a batch, i.e call batch_set(operations = "run those update functions for project foo") inside one of the batch operation callbacks. The operations will be queued at the end of the currently running batch.
    Only drawback is that users will see the progress bar 'jump back' to account for the newly added operations (or possibly start fresh from 0, I don't remember exactly). This also means all updates for all project will happen at the end of the batch, no matter what the choice of grouping for previous steps was ('fetch foo / copy foo / fetch bar / copy bar...' or 'fetch all / copy all')

    JacobSingh’s picture

    heh, quite right.

    dmitrig01’s picture

    FileSize
    40.08 KB

    here's the latest version of the patch. you can download test modules at http://dmitrizone.com/update-fake - i have a security update, recommended update, and unsupported (which, for some reason, doens't work).

    running the update functions doesn't work, no idea why. @yched, can you take a look?

    diffstat sez 577 insertions(+), 450 deletions(-). not too bad for a feature this awesome.

    yched’s picture

    Not much time to test right now (and no clear idea on how to test this patch as well, I've just been intermittently lurking so far)

    It might be worth running a debugger and check that :
    - update_update_extensions() is called
    - _update_get_schema_updates($project) returns actual updates (i'm not sure the new functions will be seen without a registry rebuild ?)

    Also - in update_copy_extension():

    +  catch (Exception $e) {
    +    drupal_set_message(t($e->getMessage(), $e->arguments), 'error');
    +    // Some better error handling is needed, but batch API doesn't seem to support any.
    +    throw $e;
    +  }
    

    You'll want to store failures in $context['results'] (that array is free for you to structure in whatever way you like) and handle them in the batch 'finished' callback to provide feedback for the user. update_update_extensions() should overlook projects that are in $context['results']['failed_projects_or_whatever_name'].

    Bojhan’s picture

    @dmitrig01 Your demo doesn't work :(

    dww’s picture

    Component: update system » update.module

    Sorry to re-enter the fray at such a late date -- I've had a number of personal crises in the past few weeks and I couldn't stay on top of this issue. I just made time to read the entire thread, and I have a few points:

    A) Re: http://bojhan.nl/updatepage.jpg from #79 (which I understand is the mockup the latest patches are trying to implement). Putting different kinds of info in the "title" field via parens is confusing. I totally missed "(Security update)" when I first glanced at it. Plus, just using a red background makes it look like those two rows are pre-selected or something. And what happens to the row color when you select, it goes yellow, right? I know Dries already "decided", but what's wrong with at least keeping our watchdog icons? Also, what about all the different status text that update_status can currently print out when telling you why something is marked yellow or red? There are a number of different reasons, and it currently tells you about them. If we drop a status column, I don't see how to communicate any of that.

    B) The existing update_status UI has a lot of thought (and functionality) that's gone into it. I think a lot of that is being lost here. In particular, what happens in the following scenario:
    - You're running foo 6.x-2.1.
    - foo 6.x-2.2 is a security update.
    - foo 6.x-2.3 is a regular release, now available.
    In the current update_status UI, it tells you that you're missing a security update, and that you should upgrade to 6.x-2.3 (even though that one isn't actually a security update). And it gives you info on all N releases you're missing, with links to release notes, so you can make an informed decision on what you should do. I don't see that in any of the mockups/wireframes in this issue.

    C) Releases are on the granularity of projects. A single project can include multiple modules or themes. Again, the current update_status UI deals with this, and none of the mockups in here appear to do so.

    Maybe the idea is that the existing admin/reports/updates page is going to continue to exist exactly as it does now, and this other page will exist somewhere else, but that seems confusing, too.

    Re: running DB updates -- maybe we can punt on that in this issue until #233091: Move code from update.php into includes/update.inc is done? That's all about refactoring update.php into an update.inc that can be reused for things like this. I'd rather leave DB updates out of this patch until there's a better way to deal with that.

    p.s. drupal.org usability problem -- "update system" is about update.php and running DB updates. "update.module" is the update module. This patch is totally blurring the distinction between the two. :( Not sure what to do about it, but I'd rather not have to subscribe to all update.php issues to be able to continue maintaining update module...

    dmitrig01’s picture

    Component: update.module » update system

    @Bojhan try now.

    dmitrig01’s picture

    Component: update system » update.module

    crosspost

    dmitrig01’s picture

    A) I don't think an icon is necessary. any opinions?
    B) What we show is "Security update, download 6.x-2.3"
    C) This patch deals with it, I recommend you try it.

    DB updates - there are two versions of the patch. one of them splits it out, the other one doesn't. I think they are leaning towards the one which doesn't split it out. I like my solution, because it requires minimal changes.

    dww’s picture

    Re: B) -- and what if you don't actually want to install 6.x-2.3, since it adds a dependency on another module you don't want to install or something? There's no way to say "just upgrade me to 6.x-2.2, the security update, and I'll wait on the new features introduced in 6.x-2.3 until I can verify them elsewhere". Of course, if every contrib maintainer managed their releases as suggested and intended, this wouldn't be an issue, but in reality, this is a legitimate concern...

    Re: C) -- I don't have time in my life right now to try it. :( Can you post an updated screen-shot of the current UI? Based on all the mockups previously posted, I don't see how anything addresses the fact that a given project can include multiple modules (not all of which are enabled on any given site).

    Anonymous’s picture

    Just a few things:

    The Plugin Manager has been broken down in to 4 parts. This is part two. Parts three and four will either require use of some of the components here OR a great deal of duplicated code. This thread is effectly stopping progress in two others. Yes, icons would be nice. Either way, can we please just go ahead with Dries decision? After committing an initial patch, which will remove the blocking from the other two parts, we can go back and work on all of the fine grain details.

    I know we all want this to be awesome. I know that's why we're debating it so fiercely (and arguably, unproductively.) But I urge you, don't let your desire for perfection be the death of an imperfect, but still usable and useful system.

    @dww: I haven't been able to try the latest patch either, but I have been able to try the ones before it. The interface works good enough. We can't make it everything to everyone. People that are aiming for a paticular version will have to update manually for now. If they want a particular version, then they probably have the know-how to install it. Either way, this can be ironed out in the future, AFTER this much has been committed.

    @dmitri: I've never had any doubt that you like your patch. ;) It is a good patch. Just as a personal suggestion though, I might recommend not saying that too often. To someone who hasn't been religiously following the thread and ends up reading much of it at once, seeing you say that you like your patch several times makes it sounds like you can only see your way of doing things. (Which I don't believe to be the case at all.) You're a good developer, but people will naturally quit listening if they think that you have. Just a friendly suggestion. Having said that, keep up the good work. ;)

    /me Ducks, as I expect to get flamed... ;)

    dmitrig01’s picture

    @JoshuaRogers LOL - almost each time has been about something different :D

    dww’s picture

    @all: I'm not bikesheding here. This is not about putting a little polish on a few minor things. This patch rips out huge chunks of update module, and a lot of important functionality and thought that's gone into it over years. This is not just about how to lay out the UI -- it's about fundamental questions regarding what the UI is trying to accomplish. My objections cannot be written off as "We can't make it everything to everyone" -- BS.

    Parts #3 and #4 of this proposal are only in the planning stages now (and #4 might never happen, actually). There's no rush to get this patch in as if it's blocking other viable patches. Yes, we'd like to get as far as possible before code freeze. But let's be honest. This patch is the central effort at this point, and we need to make sure we don't take a big step backwards while trying to move forward.

    Anonymous’s picture

    @dww: A review and then test of the patch from #111 seems to show that at worst, most of the functionality was shifted around. I'm not quite sure what functionality got ripped out other than a little bit of UI, which was replaced.

    Also, I wasn't trying to write off your objections, though I don't believe B is worth fretting about at the moment. I was simply suggesting that they make it into the second patch. After committing the first patch, your objections, almost all of which seem to deal with UI and would thus have little bearing on #3 and #4, would be addressed. True, parts #3 and #4 are both in the planning stage, but I'm inclined to think that is because they obviously can't proceed without reinventing parts of this issue.

    Everyone of your points is valid. I simply ask that we commit a working version and then work on smaller, more targetted patches to fix the remaining issues.

    @dmitri: Just tested #111. It works for me, and quite honestly, it seemed rather easy to use. Good job. :)

    I too approve of Dmitri's patch.

    Paul Natsuo Kishimoto’s picture

    Iterating is of course the best way to go, but for future reference, some response to dww's concerns:

    It's entirely valid that the user will not want to update a module. I also think it is good practice to use discretion for general updates, while installing security updates without delay. However, users are not bound to that practice. The user who runs no custom code and only "high-quality" contributed modules may blindly install all updates; the user who is being "careful" or doesn't want to topple a card house of custom code may review release notes for every update—security or not.

    That is to say, I see that the features of update.module address a real need. It doesn't seem critical that they are preserved as-is. The high level goal is to enable (not force) the user to easily make and implement module update decisions. This could be done with:

    • Move update.module's "pinning" feature to admin/build/modules, by adding a drop-down or checkbox like throttle.module.
    • List updates for pinned modules on this update status page, but sorted lasted and with a disabled checkbox. Link the user to admin/build/modules to allow them to un-pin modules before returning.
    • Implement #211747: Allow specifying version information as part of module dependencies so that, for example, filefield 6.x-3.0 can state it is only compatible with cck up to 6.x-2.3. This can block a cck update to 6.x-2.4 or greater until filefield 6.x-3.1 (with updated dependencies) is available. The need for the user to identify such incompatibilities manually is removed.

    ...or some other combination of features.

    I'm not sure what other features of update.module are being discarded by this patch, so I can't speak to them.

    Bojhan’s picture

    @dww Anyway, as I haven't been able to test out this patch - so sadly can't feedback on all your concerns. Remember that I am a UX guy, I don't know nothing about patching - installing cywgins and handling xampp is already eating to much of my time.

    I understand your design concerns, but understand that most of us are biased on the system model and not the actual human behavior model.

    A)
    So regarding putting info in the title field, I am not sure if it really is confusing - as I have applied this in other interfaces as well which went trough rigorous user testing.

    The red background is to explain that you should look at it, visual importance. I don't think it will imply pre-selected, unless we use such a pattern in Drupal - which we don't for selected items.

    I haven't decided on what color it turns upon selection - don't feel it has to behave any different then what we do all over Drupal, using the dark blueish.

    Regarding icons, I think dmitrig already quoted Bojhan "if you need to explain your icons, you're doing something wrong."". We can have endless debates about these, but they are simply not good enough to serve as a usable visual language.

    So what I was hoping is to eliminate the need for description, on what colors and icons do what. And just go with two things, you either download security updates or you don't. The other colors to me seem to address issues beyond installing security updates, which is fine - but is creating to much distraction when there is an security update.

    Don't know how to address your status column, assuming its a code question.

    B) Security update first ? Then show new upgrade opportunity? I don't see this being a big UI problem though. Lets choose a sensible default here, people who use this page do not know all the underlaying issues of our contrib.

    C) Not sure how it doesn't, it doesn't group you mean?

    @dww I hope you understand, I didn't just go out and - ignore all the previous work on this page. I tried to create a more usable page, that tries to address the needs of an less knowledgeable audience. I think we are fundamentally changing the audience of this page, therefor making more decisions for them and that might not be in line - with the system model.

    dww’s picture

    <unpopular class="deeply">
    A "less knowledgeable audience" should NOT be using this page. Period.
    </unpopular>

    I don't buy the dichotomy between developers and usability experts. Just because I write code doesn't mean I don't know anything about how humans interact with these systems. For *years* I've been trying to improve the human element of the Drupal contrib landscape and release management. It's been an uphill battle, to say the least. And, while one thrust of the community has been to "lower the barrier to participate" in contrib, another thrust is this feature as a "killer feature". We can't have it both ways. We model this feature on Apple, which has a giant building full of release managers and testers to (attempt to) guarantee that running "Software update" won't break your machine. And even then, they get it wrong sometimes. Drupal has probably 3000 contributors making their own releases with wildly varying degrees of frequency, quality, testing, and care. This is a recipe for disaster for this feature.

    Either this feature needs to only work on core, we need to radically change the mechanisms and culture around contrib releases (which simply is not going to happen), or the feature needs to not "dumb down" the interface so much that people regularly shoot themselves in the foot with it.

    It was bad enough that the ability to ignore certain projects was relegated to D6 contrib when updates_status moved into core. Dries said those settings were "too complicated for core". In reality, some contrib developers make new releases every time they commit a change to CVS, and constantly being reminded to upgrade makes update_status "cry wolf". Further attempts to "simplify" this process in core are going to keep running into the sobering reality that contrib is a vast, non-uniform space.

    Upgrading your Drupal site is *not* as simple as running "software update" on your Mac, and it never will be given how Drupal actually works and exists in the world. This interface has to reflect reality. If you want to completely simplify the interface and hide all the "ugly" details from the "less knowledgeable audience", then you have to change reality. You can't make the interface reflect your ideal vision of reality and then hope reality will magically shift on its own.

    Everything should be made as simple as possible, but no simpler.

    -- Albert Einstein

    Dries’s picture

    @dww, so again, what exactly was ripped out that should be restored? Your comment in #125 spreads general wisdom, but let's try to be concrete here because I don't see the disaster here ...

    If you want to be high-level, realize that in Drupal-land reality changes all the time. Drupal 7 will be vastly different than Drupal 6, and the same could be true for contributed modules. Some would argue that changing the mechanisms and culture around contrib releases would be a good thing. We're trying to introduce test driven development too. So on the positive side, this patch could help us better enforce the best practices that were badly missing in contrib -- it would solve a big pain for users. :)

    Anyway, could you outline the specifics of the disaster that might get introduced by this patch, that I fail to see? It's important that I understand the concrete scenarios and implications so I can weigh the pros and cons. I think the pros are huge, by the way, so the cons "better" be as dramatic as you seem to suggest they are.

    Bojhan’s picture

    @dww Sorry, but if you fundamentally disagree to change this page for a broader audience. I cannot help you change your mind on that.

    Also your notion of usability experts vs developers doesn't make any sense to me. Of course you try to build for humans? I do the same - since we are building for humans, we won't always agree - just like humans don't always agree.

    I don't feel we have to reflect reality of all our contrib problems, don't see how that is helping most of our users at all. If we really do not want them to use this page, lets just kill this page? Your suggestion implies its only for developers, if thats the case I will just step out of this discussion - because then your perception of who we are building Drupal for is vastly different from mine.

    To quit the philosophical discussion, lets just focus on that we are trying to make a better interface - I don't get the problem. Is the current update page so kick ass usable, that we shouldn't change it?

    Everett Zufelt’s picture

    Having only started following this issue today I can say that I don't feel totally up to speed. The functionality sounds good, and well appreciated from a Drupal administrator.

    My cheif concern is that there is a lot of talk about using colour and icons to represent information. Being a blind screen-reader user I always get nervous when information is being conveyed visually, fearing that the same information will not be conveyed by semantic markup.

    I have never patched a Drupal install, a time to learn everything. If someone is interested in walking me through understanding which of the above patches to apply, I would love to give this module an accessibility review.

    Senpai’s picture

    Everett, I will TOTALLY help you in in doing that. I am also concerned that what we're doing here is only comprehensible by people who've spent some time in Drupal and CVS repositories before. My deep desire for this tool's User Interface is to enable any site owner to safely apply security updates to their drupal site, and optionally apply non-essential updates, all in a perfectly stable and secure manner.

    There's work going on in the other three issues to ensure that the 'plumbing' works, and this issue is all about how the tool is perceived. I tell you what I'll do. I will apply Dmitri's latest patch and dummy modules to a Drupal 7 site and place it on my public server tonight. I will report back here with a public URL soon.

    Please standby, and thank you Everett for your offer! (BTW, do you use IRC chat?)

    Senpai’s picture

    Status: Needs review » Needs work

    @DmitriG: Patch #111 seems to be broked.

    Fatal error: require_once(): Failed opening required '/www/drupalhead/modules/update/update.report.inc' (include_path='.:/Applications/MAMP/bin/php5/lib/php') in /www/drupalhead/includes/bootstrap.inc on line 1764
    

    Post-patch, update.report.inc is missing (due to deletion), and Line 1764 is:

    <?php
    1761  $cache_key = $type[0] . $name;
    1762  if (isset($lookup_cache[$cache_key])) {
    1763    if ($lookup_cache[$cache_key]) {
    1764      require_once DRUPAL_ROOT . '/' . $lookup_cache[$cache_key];
    1765    }
    1766    return $lookup_cache[$cache_key];
    1767  }
    ?>
    

    @Everett: I've installed a public sandbox with this patch and DmitriG's three dummy modules on it. http://drupalhead.transparatech.com/admin/reports/updates with user admin & pass YoobahGoo

    tstoeckler’s picture

    @Senpai: Your sandbox is currently broken: Fatal error: require_once(): Failed opening required '/var/www/drupalhead.transparatech.com/modules/update/update.report.inc' (include_path='.:/usr/share/php:/usr/share/pear') in /var/www/drupalhead.transparatech.com/includes/bootstrap.inc on line 1764

    dmitrig01’s picture

    You need to rebuild your registry before testing this patch because otherwise you will experience #fail. I rebuilt it on Senpai's d7 by visiting admin/build/modules

    dmitrig01’s picture

    Status: Needs work » Needs review
    FileSize
    40.08 KB

    Not broken, and this one's better

    tstoeckler’s picture

    Notes from clicking through the Senpai's demo:

    1. On "Available updates" there used to be a nice list of all modules (and themes) I have installed. This page is gone now, just wanted to point that out. Technically, it wasn't really showing you available updates, while with this patch it really is, but I actually found that listing to be quite useful sometimes.

    2. On the first page the button reads "Continue". Continue with what? Either the button should read something like "Update selected" and/or the help text should be updated to explain what happens when I press that button.

    3. When I tried to connect to FTP server (which I obviously couldn't) the two error messages for the two connection methods read:

    Cannot connect to FTP Server, please check settings

    and

    * Warning: is_dir(): connect() failed: Connection refused in FileTransferFTPWrapper->connect() (line 10 of /var/www/drupalhead.transparatech.com/includes/filetransfer/ftp.inc).
    * FTP Connection failed.

    The first one is missing a period and the end and in the second one, the PHP error should not be shown. And they should be consistent in that they should either both tell you to check your settings or both omit the last phrase (which would be more the Drupal standard, I think).

    ...Oh, and I WANT THIS!!!

    dmitrig01’s picture

    FileSize
    20.81 KB

    New version

    dmitrig01’s picture

    FileSize
    40.01 KB

    oops, forgot the -N

    chx’s picture

    Status: Needs review » Needs work

    Beware. This patch does hose your site. This is not the usual 'it might kill', it just deleted themes/ and misc/ and a few assorted files from my computer.

    Anonymous’s picture

    Status: Needs work » Needs review

    @chx: Could you give a few more details? I just read through most of the patch (#136) and then applied it. I'm getting no problematic behavior. Are you saying that the patching process kills Drupal, or the update process kills Drupal?

    dmitrig01’s picture

    installing was fine for both of us. updating worked for me but hosed chx's drupal.

    Senpai’s picture

    I just reverted patch #111, cvs upped, and then applied #136. Patch applied fine, and is now running on http://drupalhead.transparatech.com.

    @Dmitri, beats me why the last one wouldn't work, and I had visited admin/build/modules on both localhost and dev like 10 times each. Ahh well! :)

    _I'm still waiting for Everett's screenreader review of this patch_. Come on baby, hit us up with some ideas!

    My review?

    Screen #1

    The modules appear to be sorted alphabetically by first name rather than grouped by critical vs non-critical. I see this as un-good, because if there are 12 modules showing up on this page, my critical ones will be scattered all over the place. The image above shows red rows for the security and the unsupported modules, and light blue for the recommended module. Clicking the checkbox in any row changes that row's background color to dark blue. Ok, fine.

    I clicked the 'continue' button, which I agree needs to be renamed. I don't have a good name suggestion, though, because the more I look at this two-page thing the more I realize that we're just standing in the way of administrative progress by requiring people to click a 'proceed' button which hides all the stuff I've just selected for update and takes me to a page with nothing but a form on it which is totally disconnected from what I was just doing. It's a usability leviathan, but hey, we can't fix it because it's been decreed to be this way, so we press forward in hope of a second patch to make this thing user-friendly.

    Screen #2

    Now that I'm on the second screen, I see a Connection Method of either 'FTP' or 'FTP using file streams'. What are these file stream thingies? Why am I being asked about using them, and if they're better than regular FTP, why isn't it defaulting to file streams instead? I'd better not use file streams then. I'll stick to FTP.

    I enter my info and get ready to click the 'put site into maintenance mode and install updates'. Whaa? I don't want to take my site offline! I am only updating one module with a single-line security fix that adds a filter_xss check. You're going to force me to take my 25,000 member site offline for an undetermined amount of time just to update a security fix this small? I stand to loose $12000 dollars of revenue by clicking this button, so NO!

    Patch Review Summary
    1. Sort the rows of this table by their critical-ness, with critical updates at the top.
    2. Rename the 'continue' button to something more meaningful and lead-me-by-the-hand
    3. Don't force me to take my site offline. That's not what the mockups say to do.
    4. We're missing a column for release notes.
    5. We're missing a column for the distinction of themes vs. modules.

    dmitrig01’s picture

    For the table, http://drupal.org/node/517318 will put them at the correct places in the table. I can reroll the patch with a 'Continue' button.

    Paul Natsuo Kishimoto’s picture

    @Senpai: you raise a good point about maintenance mode, but I don't see an easy solution. To be sure, maintenance mode is not a strict requirement for every single update. But to identify the situations where it can be avoided, either:

    • The user has to review not just the changelog but the patchset related to a particular update and determine whether they'd want to take the risk of an online update, or
    • The developer has to test, (correctly) identify and somehow flag each update as "online-safe," and this somehow has to be communicated to the client.

    Options like having the updater disable each module as updated, so Drupal doesn't attempt to load old and new source files from a half-updated module, are only useful if the site behaviour degrades gracefully, which is in no way guaranteed. In any case, online updating seems like something that needs its whole own set of patches. Maybe just some explanatory text? "Updating these modules requires placing your site into maintenance mode. Leaving the site online during updates could result in database inconsistencies and data loss" or something.

    Re: your #4, the "Recommended version" column seems to contain links...aren't those to release notes? What would you prefer—maybe my suggestion in #48? :)

    Also, I'm getting 403 errors at http://drupalhead.transparatech.com

    Senpai’s picture

    Wow, it seems there's nothing left in my / directory except for an index.php, an empty sites/ dir, and an empty updates/ dir. Even the CVS/ folder is missing?! Everything was there and working this morning when I applied the patch. The screenies from #140 are from the latest patch on this sandbox server too.

    I don't know what went wrong, but chx might be right.

    Senpai’s picture

    Status: Needs review » Needs work

    Chx was right. This patch deletes the vast majority of core site files whenever you submit the FTP form on screen 2. I've restored my sandbox, and made a duplicate dir of the patched core code, just in case. :)

    Bojhan’s picture

    @Senpai

    1. Yes, thought it was like this.
    2. What about update?
    3. Yhea, I don't see the necessity in that either - unless you are updating like 20 critical big ass modules.
    4. No, the latest version should link to release notes.
    5. No, (theme) - it doesn't occur that often that you need to update your theme.

    tstoeckler’s picture

    6. I think dww's concern still stands. What if Earl decides to release Views 2.6 and shortly after, the new kick-ass Views 3.0. I should get to choose which version I want to install, and as far as I know the current UI doesn't account for that.

    JacobSingh’s picture

    @Senpai: Personally, I agree with you on almost every point, and that is how I planned to build it (as you see in the mockups). However, You need to read the entire issue in full. I understand it's almost impossible because it's gone in a lot of directions, and a *lot* of cooks want to add spice at various points, but everything you mentioned has been discussed, and I believe what we had a little while ago has been okayed by Dries. Opening up a whole can of worms right now will almost certainly stop this feature from getting into D7.

    1. Sort the rows of this table by their critical-ness, with critical updates at the top.
    This is a regression I guess, it was like this in my last patch.
    2. Rename the 'continue' button to something more meaningful and lead-me-by-the-hand
    I don't have hands, I am a 'puter I don't think this needs to take up mind space, there is only one button, I think it's pretty clear.
    3. Don't force me to take my site offline. That's not what the mockups say to do.
    See the comment thread (for the next week of your life).
    4. We're missing a column for release notes.
    Dries approved the design where the version number links off, I'd rather not deviate, lest it get all over the place again.
    5. We're missing a column for the distinction of themes vs. modules.
    I also wanted this there, but Bohjan decided to remove it, and Dries okay'd that design, although I don't think that was a particular issue. Anyway, we are showing (theme) there, which is good enough for now I suppose.

    Also:
    I didn't want two FTP options, see Part 1 for my 30 comment long fight with chx and eventually Crell on that who convinced me that it was not a UX sin. It sucks, but we've got to live with it now, because I have a job and couldn't spend 2hrs a day on IRC fighting about it every day.

    Best,
    Jacob

    catch’s picture

    Completely agreed on not taking site offline - link to the page, sure, but for many people having the site in maintenance mode is rendering it as unusable as if it's hosed (and maintenance mode doesn't stop anything from getting hosed unless you're getting hammered by traffic, in which case you shouldn't be updating via this anyway).

    FTP options - followup patch.

    (theme) and the release notes link - I think these are complely fine as they are, and easy to change in a followup patch if needed.

    I think dww's main points that need to be dealt with are these:

    We're losing some of the finer grained reasons for upgrading - i.e. I'm on Views 2.6, and Views 3.0 UNSTABLE-1 comes out - the current UI tries to suggest some caution about upgrading to unstable 1 (or alpha, beta, rc), if we only have red and blue, and no descriptive text then we're losing some of that.

    There's also the issue of being on 2.5 and having the option of 2.6 OR 3.0-UNSTABLE-1 which the current UI doesn't support. i.e. does it default to 2.6 or 3.0-unstable1. If it defaults to 2.6, will I get nagged to upgrade to 3.0-unstable-1 as soon as I've updated to 2.6? Or do we only ever recommend stable releases from this interface? What if I'm running dev and there's a newer dev build.

    There are also other statuses for currently installed modules which are critical but aren't a security update - the main two is when a module author unpublishes a release because it's unsupported - i.e. sites running panels-6.x-2.x which should be prompted to upgrade to panels-6.x-3.x - IMO these should just be red as well.

    Then there's the situation where rather than a new security release, modules are so broken that we recommend uninstalling them completely - see http://drupal.org/node/318746 - this status is 'revoked' in the current UI. As far as I can see, the current UI wouldn't show those modules at all, because they don't have updates as such.

    Note, I don't think we need to allow for fine-grained updates of contrib modules via this UI, if you want that, use tarballs, drush or cvs - but these particular sitautions where either uipgrading, or not being prompted to do something else can result in a hosed site ought to be dealt with I think - even if it's this patch going in as a secondary interface from the current one and then trying to unify them later.

    Everett Zufelt’s picture

    Took a look at the test site:

    1. It may be helpful to use a summary attribute for the table to provide screen-reader users a description of the data in the table.

    2. I didn't realize that release notes were available from the new version link until I returned to read through the issue queue, so it might be useful to include a Release Notes column.

    3. There is colour (red) being used to communicate information that is not available in semantic markup. This information needs to be available semantically. Two suggestions for doing this are:

    a. Sort the table by type (security, unsupported, etc.) and use a row to communicate where one section starts.

    b. (Recommended) Provide some form of language e.g. "(Security)" and hide it off screen using a similar method to that recommended by w3c wcag 2.0 for links at http://www.w3.org/TR/2008/NOTE-WCAG20-TECHS-20081211/C7 . An issue discussing the best method of hiding text off screen in Drupal is being discussed at http://drupal.org/node/473396#comment-1810228

    4. Provide a more meaningful value for the "Continue" button, continue to what?

    dmitrig01’s picture

    Status: Needs work » Needs review

    Table sorting - I've addressed this issue. I don't want to kill too many kittens with this patch. If you apply this patch and the patch at http://drupal.org/node/517318, then the table will be sorted.

    Unsupported is red, not sure about unpublished. Also, I'd like to hear your suggestions about how to convey multiple versions with this UI.

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    Senpai’s picture

    Items On The Agenda

    A) We still need to sort the rows of this table by their critical-ness, with critical updates at the top.

    JacobSingh: This is a regression I guess, it was like this in my last patch.

    DmitriG: Table row sorting will by handled by #517318: Tableselect needs to be able to give options weights

    Senpai: In that case, I consider point A addressed and fixed.

    Point A is fixed.

    B) Rename the 'continue' button to something more meaningful

    JacobSingh: There is only one button, I think it's pretty clear.

    Everett Zufelt: Please provide a more meaningful value for the "Continue" button, continue to what?

    Senpai: Let's call the button "Install these updates", and then re-roll the patch.

    Point B requires a reroll in order to rename the submit button.

    C) Don't force me to take my site offline. That's not what the mockups say to do.

    JacobSingh: Agreed. See the comment thread (for the next week of your life).

    Catch: Agreed. Do not take the site offline automatically - link to the page, sure, but for many people having the site in maintenance mode is rendering it as unusable as if it's hosed.

    Senpai: Agreed. I think we need to re-work this patch and remove the automatic site offline thing. We'll continue discussion and a possible UI for this in #519422: Plugin Manager in Core: Part 2-C (toggle site online/offline)

    Point C requires a reroll to remove the automatic site offline feature.

    D) We're missing a column for release notes.

    JacobSingh: Dries approved the design where the version number links off, I'd rather not deviate, lest it get all over the place again.

    Everett Zufelt: I didn't realize that release notes were available from the new version link, so it might be useful to include a Release Notes column.

    Senpai: If Everett couldn't find the release notes easily, and I feel that the new version number being the only link is not clear enough, then we'll handle this in another issue so as not to hold this one back.

    Point D will be fixed by #519402: Plugin Manager in Core: Part 2-B (links to project's release notes).

    E) We're missing a column for the distinction of themes vs. modules.

    JacobSingh: I also wanted this there, but Bohjan decided to remove it, and Dries okay'd that design, although I don't think that was a particular issue. Anyway, we are showing (theme) there, which is good enough for now I suppose.

    Senpai: Consider this one closed. The whole distinction of Module vs. Theme has slipped through the cracks in the last 200 comments above, but I have an idea for solving it that I'll propose in another issue along with the primary concern that DWW has (and that Catch so elegantly conveyed). See Point F below.

    Point E will be fixed in point F.

    F) Plugin Manager is not communicating the subtle differences between multiple releases of the same project.

    Let me explain... No, there is too much. Let me sum up.

    Point F will be fixed by #519470: Distinguish between released versions of the same project when selecting projects to upgrade with Update manager.

    G) We need to distinguish between critical and non-critical modules with more than just a color scheme on the various table rows.

    Everett Zufelt: There is colour (red) being used to communicate information that is not available in semantic markup. This information needs to be available semantically. Two suggestions for doing this are...

    Senpai: I will make a new issue for this, and we'll solve it over there, because it's important.

    Point G will be fixed by #519460: Plugin Manager in Core: Part 2-D (distinguish between critical & non-critical).

    H) Multiple FTP Options in a dropdown menu are way too confusing for new users.

    JacobSingh: I didn't want two FTP options, see #395472: Plugin Manager in Core: Part 1 (backend) for my 30 comment long fight with chx and eventually Crell on that who convinced me that it was not a UX sin. It sucks, but we've got to live with it now...

    Senpai: We need to fix this so that new users aren't confused by arcane FTP choices. I'll make a followup issue for this.

    Point H will be fixed by [#000000].

    I) Running this updater will delete the site's core files.

    CHX: Beware. This patch does hose your site. This is not the usual 'it might kill', it just deleted themes/ and misc/ and a few assorted files from my computer.

    Senpai: Uhh, guys? Do we solve this in this issue, or another one? Help me out here.

    Point I still needs a solution.

    JacobSingh’s picture

    Okay, I'm totally for this course of action. Very much appreciate the herding @Senpai.

    I think the things you've identified as follow-up are indeed follow-up. I have another 3-4 that are probably even more critical, but would prefer to wait and not add additional confusion here.

    I've got my community day on Thursday, so unless Dmitri gets to these first, I'll make the small changes required and try to debug why people are getting their DRUPAL_ROOT killed :)

    If anyone wants to debate these points, please consider if what you are commenting on is going to block consensus, and if so, be careful. We all want something in before DC.

    Let's go!
    Jacob

    JacobSingh’s picture

    FileSize
    20.07 KB

    Okay, the patch from #136 doesn't apply anymore. I'm not sure if I got it all right, but here is an update with nothing changed that just fixes the problem applying to update.php

    JacobSingh’s picture

    Yeah, latest patch in #136 also destroys my site. I'll figure it out and fix today.

    PLEASE, DO NOT APPLY ANY PATCHES AFTER #135 if you care about your code base.

    JacobSingh’s picture

    FileSize
    13.07 KB

    Okay, now 100% less DRUPAL_ROOT destroying :)

    There was a misnamed variable in the #136 patch and no handling of a null value.

    list() doesn't work w/ associative arrays AFAICT on my mac.

    Anyway, this should be better now. I also fixed up update.php (my #153 patch broke it) and paved the way for installation of new mods (not just updates).

    I'm going to start on the changes above now.

    btw, this was caused by a conflict of the naming convention we started with "extension" which is now called "project" in half the places. I don't want to bike shed about it, we can switch to project. I suppose this is preferred, because it is a d.o. standard. I will try to refactor that during my next patch.

    JacobSingh’s picture

    FileSize
    20.44 KB

    Okay, here's a couple more changes:

    A) Rename the 'continue' button to something more meaningful
    Done. Went with Senpai's suggestion. Let's run with it.

    B) Don't force me to take my site offline. That's not what the mockups say to do.
    Not Done - Yet. I'm going to salvage something from my original patch to handle this. I know there will be complaints all around. I can't remember who the first wanted it auto, but I always thought it wasn't a great idea.

    C) We're missing a column for release notes.
    Done - Was simple, just added it.

    D) Running this updater will delete the site's core files.
    Done.

    So ther remaining stuff from Senpai's post are all in separate tickets. All we've got left after this patch AFAICT in terms of feature changes is the auto offline thing.

    Senpai’s picture

    Status: Needs work » Needs review

    Let's see if the AutoBot likes it...

    catch’s picture

    OK so dww mentioned this and it has been discussed a bit, I don't see anything here to deal with branches of contrib modules.

    +/**
    + * Get the latest version of a project.
    + */
    +function _update_get_latest_version($name) {
    +  if ($available = update_get_available(FALSE)) {
    +    module_load_include('inc', 'update', 'update.compare');
    +    $extension_data = update_calculate_project_data($available);
    +    $project = $extension_data[$name];
    +    return $project['releases'][$project['latest_version']];
    +  }
    +}
    

    So I have somemodule-7.x-1.x installed, this gets up to 7.x-1.3 then the maintainer abandons it an a new one takes over. The new maintainer releases 7.x-2.x which has huge API changes, a schema update, a syntax error and breaks integration with 5 other modules I have installed - but the update manager never, ever warned me I was upgrading to a new branch except in the release notes, and more importantly, didn't offer an option to stay on the older branch, but upgrade to the 7.x-1.4 security release.

    I think for the first run of this UI, we should only allow for upgrades to the same branch as the current one installed for a module - so I can upgrade Views-7.x-2.6 to 2.7, but never to 3.0. Then we can open a separate issue to try to work out a suitable ui for branch updates - I'd suggest something like 'additional updates available' and link to a separate screen - so it's very clear to the user that they're doing something out of the ordinary.

    We can't stop bad releases within branches, but we can at least protect contrib developers from having thousands of sites automatically upgraded to new branches which may well have non update.php task you need to carry out.

    JacobSingh’s picture

    But didn't the previous update status just link you off to the "recommended" release?

    I can hear where you're coming from, but also, I don't think we should change it. The discussion around this could throw us into another tailspin and time is short, so could we keep it how it is, and make the discussion happen on the separate issue which has been created especially for it?

    #519470: Distinguish between released versions of the same project when selecting projects to upgrade with Update manager

    I'll provide some comments there.

    Best,
    J

    JacobSingh’s picture

    FileSize
    20 KB

    Summary of updates:

    Fixed a few regressions and some things which look like regressions that are in #136 and perhaps earlier:

    1. The information on the 2nd form about backups, etc was using #value = not markup = and was not showing up.

    2. The help for the report page was showing on every page. Added back a menu handler for 2nd and 3rd page. This will also be re-used for installer.

    3. Report page was totally broken, renamed variable for session not changed in both places.

    4. Report page showed the success and failed errors! wtf...?

    5. The actual log of update operations is empty due to the logging lines being removed.

    New stuff:

    Added back a manual device to take the site offline. I kinda prefer my original UI (see earlier movies and screenshots), but it wasn't appreciated by others. Hopefully this is good enough. At least it is simple to implement.

    Weirdness though. Default for Radio just not working... Either I am insane, or this awful bug is still present:
    #208179: Default value ignored for radios in tables due to faulty JS

    What's left:
    Schema updates. I think there are some issues here, but haven't tested.

    This looks really stoopid:
    Updating your site | dev7

    Not sure how we can fix it though. Some way to gag the former?

    Best,
    Jacob

    JacobSingh’s picture

    @catch: Come to think of it, what you are saying does make sense. As much as I dread having this become another free for all, when I just cleaned up shop, and we're very close, it does need something.

    PEAR uses preferred_state which is kinda nice, because if I want alphas to show up, I can set this. Maybe we can figure out something similar?

    Another option: provide minor if available, major if not, and warn when they check a major upgrade.

    Something like this: say there are 2 1.x version of panels and 1 2.x version

    If i have panels 1.0 i see 1.1 on my screen. After upgrading 1.1, when I go back, I see 2.0. If I click 2.0, I get a big fat warning. In the future, perhaps we could add the plumbing to d.o. to let the author add a "buyer beware" type message for branch -> branch upgrades.

    What you think? I'm going to get Dries to weigh in on this ASAP so we get a definitive call and we don't go in circles. In the future, I am all for complicating it to the degree necessary to support both cases.

    Best,
    Jacob

    Status: Needs review » Needs work

    The last submitted patch failed testing.

    JacobSingh’s picture

    FileSize
    21.06 KB

    I found another regression:

    The (security update) notice text which so hotly debated was actually not showing since #136, the switch statement there was backwards (working with vars that didn't exist, also through E_NOTICE errors.

    I fixed that and also renamed everything which was called extension to project.

    Sorry, last patch was out of date on arrival. This one should be cleaner.

    catch’s picture

    On maintenance mode, any boolean choice should be a checkbox, not two radios - so "Take site offline while updating" then a checkbox. This would take about as much space as help text recommending to take the site offline, so checkbox seems appropriate here.

    I think #162 works for the first commit, I'd also been thinking along the lines of the normal vs. LTS vs. none options in ubuntu update manager but that's not for this issue.

    JacobSingh’s picture

    @catch: Cool, I'll work that up.

    Is the radio thing a bug? It's seems quite strange, but maybe I'm just tired, I didn't try to follow it back.

    Best,
    jacob

    JacobSingh’s picture

    FileSize
    21.68 KB

    Okay, more changes:

    There was a bug in the code which was supposed to process updates, had a misnamed variable, and I couldn't figure out how to add tasks to the batch the way it was being done.

    To rectify it, I am just calling update_do_one directly. @TODO: This should return finished in parts, and should set messages, but it does work AFAICT.

    Changed the radio for site offline to a checkbox ala @catch's recommendation.

    We're very close here

    Here's the agenda for getting this committed (not 100% happy for everyone), let's try to reduce it. :)

    - Dmitri's patch for table sorting needs to be committed:
    #517318: Tableselect needs to be able to give options weights

    - We should implement as in #162 where we offer minor upgrades instead of major upgrades, and provide a warning when doing a major upgrades.

    - Running updates with schema changes needs to show more feedback.

    - The report page should really use the title of the module, not the name (i.e. Panels, not panels).

    If anyone can please contribute on any of these counts, it will make a HUGE difference. Otherwise, I won't be able to roll a new patch for close to week.

    Best,
    Jacob

    yched’s picture

    Running all updates for a given project in one single batch op won't cut it. They really need to be performed in separate operations to avoid timeouts.

    I suspect the previous patches didn't work because update.php is not loaded on batch execution and thus the update_do_one() function is not found.
    Adding 'file' => './update.php' to the $batch definition should work. Or the batch op could be a wrapper function that includes update.php and calls update_do_one()
    [edit: heh, the patch actually adds that function, it's update_run_update(), it just doesn't call it ;-)]

    On a related note, the various batch functions (update_get_project(), update_copy_project(), update_update_project()) do not need to clutter the main update.module file, they could be moved to a different .inc file using the 'file' property on the main batch (which I can't find in the patch BTW, but I'm probably missing something).
    Additionally, I think existing batches in core (er - OK, except update.php :-p) follow a practice of having the word 'batch' in the batch callback names, just for clarity.

    Stumbled on several instances of 'an project' in comments and PHPdocs - brute search / replace, I guess ?

    JacobSingh’s picture

    yeah good catch, was %s/extension/project/ - if you want to clean these up, be my guess :)

    Okay, so for the updates, I don't think we can add to a running batch. It just doesn't seem to work. I don't know what was going on in the previous patch, I didn't write that part, but it didn't seem to work for a few reasons, one of them was includes of install.inc. I would think batch would throw an error if you refer to an undef function, but perhaps not.

    All that aside, I don't think we don't need to. The way I've got it is fine, we just need to make a small change to store the list of updates in the sandbox, and, then returning a fraction in #finished every load. Should do the trick, right? Same functionality, exactly the flow we want and less roundabout code.

    In terms of abstracting out the batch functions to yet another file, I don't really care. Since they rely on some helper functions which are sorta general to update, I thought keeping them together would be simpler. But yeah, not important to me. If you want to roll a patch, please do it quickly with just that change before people start working off the current one as is.

    Renaming them, I agree.

    Thanks,
    Jacob

    yched’s picture

    Nope, updates need to be performed each in a separate batch op, like update.php does it. Modules authors write their updates making sure that each *separate* one can reasonably run in a single HTTP request without hitting PHP timeout (or else they take the extra burden to make it a multipass update). We cannot bypass that choice and hit PHP timeout by running all of them together.

    I can try to help on that part, but I'm completely fresh to the patch, and I welcome any hints on how to test this. I don't think there are D7 contrib modules with several actual releases yet ?

    + from #156 on, the patch is missing the update.admin.inc file it had until #136.

    dmitrig01’s picture

    I was out of town, but I'll roll a new patch today.

    dmitrig01’s picture

    I'll roll a new patch today.

    yched’s picture

    FileSize
    32.09 KB

    Rerolled just for the missing update.admin.inc and admin.js files.

    JacobSingh’s picture

    FileSize
    34.65 KB

    D'oh!! I can't believe I didn't have the -N on the whole time I was fixing the hell out of that file :)

    Anyway, here is a real patch

    "Nope, updates need to be performed each in a separate batch op, like update.php does it. Modules authors write their updates making sure that each *separate* one can reasonably run in a single HTTP request without hitting PHP timeout (or else they take the extra burden to make it a multipass update). We cannot bypass that choice and hit PHP timeout by running all of them together."

    I think we're talking about the same thing. I'm saying though, we can have one op, and then do something like this:

    function do_updates($project, $context) {
    
    if (!$context['sandbox']['updates_to_do']) {
     $context['sandbox']['updates_to_do'] = _update_....
     $context['sandbox']['updates_done'] = 0;
    }
    
    if ($update = array_shift($context['sandbox']['updates_to_do'])) {
     $context['sandbox']['updates_done']++;
     update_do_one($project, $number, $context, etc...)
    }
    
    $context['finished'] = $context['sandbox']['updates_to_do'] / $context['sandbox']['updates_done']
    

    That will allow it to operate in batch mode without any issues. Is there a problem w/ this?

    yched’s picture

    Oh, OK, gotcha - one multipass op that runs all updates. I guess this could work, but we'd need to check that it doesn't mess up updates that are themselves multiple - can get mind blending pretty fast...

    For now I'm stuck testing the patch, looks like I can't seem to configure my ftp server and ftp_chdir($this->connection, $directory) fails in FileTransferFTPExtension::removeDirectoryJailed() - Windows setup here. I'm wondering if it's because I have a space in a folder name in the path to my drupal setup...

    yched’s picture

    Patch in #174 does not apply, not rolled from the root ;-)

    JacobSingh’s picture

    FileSize
    42.47 KB

    Ugh.. sorry, it's really late here... sorry

    yched’s picture

    FileSize
    42.25 KB

    Did not touch the update part, since I still cannot have the FTP layer access my drupal dir on my windows setup.
    It tries to cwd to 'D:/foo/bar/baz/drupal7/sites/all/modules/recommended' and fails - I don't think the FTP server likes the D:/

    Did the other changes mentioned in #168:
    - renamed batch ops to include the word 'batch'
    - moved them (and their helper functions) to update.admin.inc, along with the form callback and submit handler
    - fixed 'an project'.

    JacobSingh’s picture

    Re: The windows issue

    I think the original connection classes were tested on a windows box previously, but obviously not enough. Can you specify the FTP Server you are using and the windows version?

    Thanks for changing the batch stuff, etc. Were you able to test it in your condition?

    Best,
    Jacob

    yched’s picture

    The FTP server is Filezilla 0.9.23, win XP pro sp3. Home directory for the FTP user is the drupal root, all file permissions granted.
    A cwd with an absolute path starting with a drive letter doesnt seem to be accepted.

    Yes, I've tested the batch changes to work (batch starts and packages are downloaded, failure happens in the 'copy' step).

    JacobSingh’s picture

    yched: Ugh... My windows admin skills are truly awful.

    I tried to get this setup this morning to no avail and now have other Sunday stuff to do.

    1. Installed DAMP. It was nice, and very quick way to get going on windoze (which I haven't used for any development for over 7 years)

    2. Shared my D7 setup from my mac on a shared network drive, got apache pointed over there

    3. downloaded and configured the PDO extensions to get PHP to work with D7

    FireZilla is another matter... I can't seem to get it to receive FTP connections. I created a user, made its home dir Z:. WHen I login, the user runs ls and gets nothing back, but no error. Can't see any files.

    If you have any tips, I'd be happy to gelp debug this.

    Best,
    Jacob

    int’s picture

    I test Filezilla Server now and work's fine..

    -The steps that I do:
    Install, Execute.
    Edit Users, Add User, define one password.
    Shared Folders, Add, Give permision to do everything (Read, Write, Delete, Append, Create Directories, Delete Directories, List Directories,Directories + Subdirs.
    Press Ok

    (000001) 19-07-2009 11:52:23 - (not logged in) (127.0.0.1)> Connected, sending welcome message...
    (000001) 19-07-2009 11:52:23 - (not logged in) (127.0.0.1)> 220-FileZilla Server version 0.9.32 beta
    (000001) 19-07-2009 11:52:23 - (not logged in) (127.0.0.1)> 220-written by Tim Kosse (Tim.Kosse@gmx.de)
    (000001) 19-07-2009 11:52:23 - (not logged in) (127.0.0.1)> 220 Please visit http://sourceforge.net/projects/filezilla/
    (000001) 19-07-2009 11:52:28 - (not logged in) (127.0.0.1)> USER int
    (000001) 19-07-2009 11:52:28 - (not logged in) (127.0.0.1)> 331 Password required for int
    (000001) 19-07-2009 11:52:29 - (not logged in) (127.0.0.1)> PASS ******
    (000001) 19-07-2009 11:52:29 - int (127.0.0.1)> 230 Logged on
    (000001) 19-07-2009 11:52:31 - int (127.0.0.1)> PORT 127,0,0,1,193,90
    (000001) 19-07-2009 11:52:31 - int (127.0.0.1)> 200 Port command successful
    (000001) 19-07-2009 11:52:31 - int (127.0.0.1)> NLST
    (000001) 19-07-2009 11:52:31 - int (127.0.0.1)> 150 Opening data channel for directory list.
    (000001) 19-07-2009 11:52:31 - int (127.0.0.1)> 226 Transfer OK
    

    make sure that you install Filezilla Server as a Service, that you don't have one Firewall to block that connection.

    JacobSingh’s picture

    @int: That's good news.

    Are you saying filezilla works or the patch works? :)

    If the later, which patch?

    Best,
    Jacob

    int’s picture

    @Jacob, I'm working with Filezilla with a normal ftp client. Not with the drupal.

    I want to test this with Ubuntu but I can't test this patch because I can't apply the patch.

    patch -p0 < updates_395474-178.patch
    patching file update.php
    patching file modules/update/update.module
    patching file modules/update/update.report.inc
    patching file modules/update/update.info
    Hunk #1 FAILED at 7.
    1 out of 1 hunk FAILED -- saving rejects to file modules/update/update.info.rej
    patching file modules/update/update.css
    patching file modules/system/system.module
    Hunk #1 succeeded at 1279 (offset 11 lines).
    patching file modules/update/update.admin.inc
    patching file modules/update/update.js
    

    And them when I go to admin/reports/updates give this error:
    Fatal error: require_once(): Failed opening required '/persistent/home/int/sites/drupal7/modules/update/update.report.inc' (include_path='.:/usr/share/php:/usr/share/pear') in /persistent/home/int/sites/drupal7/includes/bootstrap.inc on line 1769

    int’s picture

    Status: Needs work » Needs review
    JacobSingh’s picture

    Funny, I didn't get that with that patch, neither did the bot... at any rate, I can't re-roll at the moment and I don't know what is wrong, but here's how you can get it working in 1 minute:

    open update.info

    change update.report.inc to update.admin.inc

    add update.js to the list of files.

    Should work then.

    dmitrig01’s picture

    you do not want the parser parsing js files. don't add update.js to the list.

    int’s picture

    Status: Needs review » Needs work

    Thanks,

    When want to update one contrib modules give me this error:

    An error occurred.
    Path: /batch?id=3&op=do
    Message: FileTransferException: Unable to change to directory @directory in FileTransferFTPExtension->removeDirectoryJailed() (line 101 of /persistent/home/int/sites/drupal7/includes/filetransfer/ftp.inc).
    # Unable to change to directory /persistent/home/int/sites/drupal7/sites/all/modules/update_test_module
    # Update failed! See the log below for more information. Your site is still in maintenance mode
    dmitrig01’s picture

    I think that's the error that yched was getting. It has to do with not liking X:\

    int’s picture

    proftpd log:

    Jul 19 17:31:58 domU-12-31-38-01-D5-73 proftpd[6785] domU-12-31-38-01-D5-73.compute-1.internal (localhost.localdomain[::ffff:127.0.0.1]): FTP session opened.
    Jul 19 17:31:58 domU-12-31-38-01-D5-73 proftpd[6785] domU-12-31-38-01-D5-73.compute-1.internal (localhost.localdomain[::ffff:127.0.0.1]): USER int: Login successful.
    Jul 19 17:31:58 domU-12-31-38-01-D5-73 proftpd[6785] domU-12-31-38-01-D5-73.compute-1.internal (localhost.localdomain[::ffff:127.0.0.1]): Preparing to chroot to directory '/persistent/home/int'
    Jul 19 17:32:01 domU-12-31-38-01-D5-73 proftpd[6785] domU-12-31-38-01-D5-73.compute-1.internal (localhost.localdomain[::ffff:127.0.0.1]): FTP session closed.
    
    JacobSingh’s picture

    Status: Needs work » Needs review

    Looking into WP for how they handle it. btw, that code is hard to read... like, really out there. Anyway:

    Looks like they are doing this.

    $folder = preg_replace('|^([a-z]{1}):|i', '', $folder); //Strip out windows driveletter if its there.
    $folder = str_replace('\\', '/', $folder); //Windows path sanitiation
    

    This should be a separate issue.

    #524404: FileTransfer classes need to support windows paths being thrown at them

    Windows testers: The other issue is a blocker for getting the job done and into 7, but this issue won't be about it for now. We'll sort that out separately. Please try to find a posix machine to test on in the meantime until I (or someone else) sorts out the other one.

    Best,
    Jacob

    int’s picture

    @Jacob I'm testing in Ubuntu Linux and this error happen also with Ubuntu, with proftpd.

    JacobSingh’s picture

    @int: I'll need more detail then... Can you connect via a normal FTP client and do the same operations? (remove the dir and copy a new one in?)

    My guess is you can't chdir because of a permissions issue, but I can't confirm it. Others have confirmed this patch as working on Ubuntu, but I haven't tested it on my ubuntu box yet, so I don't know.

    Best,
    Jacob

    int’s picture

    @Jacob I have proftpd already chrooted in my home dir..
    So, when I connected I'm in "/persistent/home/int/" so I don't have access to other folders.

    I have this path to drupal7 /persistent/home/int/sites/drupal7/ and because I'm chroot to Int's HomeDir, I can't execute this:
    cd /persistent/home/int/sites/drupal7

    I can only execute this to change the dir:
    cd sites/drupal7

    I have all others permissions(delete,create,open,append,...)

    Note: I have this in my /etc/proftpd.conf (chrooted ftp)
    DefaultRoot ~
    And I think that must of the shared host have this option on for security.

    JacobSingh’s picture

    Okay, so it sounds like we need to check for a chroot when making the connection and adjust. Unfortunately, I'm not sure how to handle this. Right now, we say, "Upload to DRUPAL_ROOT . '/sites/all/modules'" so, if half of your DRUPAL_ROOT is in the chroot and half is not, it's a little hard to figure out unless you provide the chroot in some setting, right?

    Can you do some research and find out how we can accurately determine what the chroot is? I suppose worst comes to worst we could remove one part of the path at a time and try to recurse down until we find some known file, but would like to avoid this.

    Best,
    Jacob

    Anonymous’s picture

    JacobSingh: Good luck with the chroot thing. The 'exploding path' method is the only one we ever came up with. That method isn't always accurate though. Users still sometimes needed the ability to specify it manually.

    int’s picture

    We can expload the drupal path by '/' char . and give all options to the users, and the last option is the manual path:

    Path to drupal: (ComboBox)
    - persistent/home/int/sites/drupal7
    - home/int/sites/drupal7
    - int/sites/drupal7
    - sites/drupal7
    - drupal7
    - Custom Path

    In my case the corrrect one is this: sites/drupal7

    (we can detected the must probably, by get the list of the corrent directories (in the second page or with AJAX with one button at the last of the from saying test connection)
    if you get list my ftp root directory is only this folder return: "sites", so probably we can give set the option selected: sites/drupal7

    So after the user acept the selection(automaticly or no) we can make sure that is correct. Find some drupal files in "sites/drupal7" or for more security we can make one temp file in sites/default/files/update/32423j4khd8ishf.tmp

    chx’s picture

    we can make one temp file in sites/default/files/update/32423j4khd8ishf.tmp aye. that's the way forward. and then you try. so you have DRUPAL_ROOT as /var/www/drupal_7 then you try first /var/www/drupal_7/sites/default/files/update/32423j4khd8ishf.tmp and then www/drupal_7/sites/default/files/update/32423j4khd8ishf.tmp and then drupal_7/sites/default/files/update/32423j4khd8ishf.tmp and then sites/default/files/update/32423j4khd8ishf.tmp and when all of those fail then you ask the user, wtf sucker?

    int’s picture

    FileSize
    48.54 KB

    See the attach image.

    I think that the auto find ftp "drupal path", should be only if the user want's, if push the button 'Auto Selected'.

    Edit:
    So if you want, we can continue with the manual ftp drupal path, and make the 'Auto Selected' button other issue..

    So when the user push "Install updates" we create one random file in sites/default/files/update/(with drupal), and we check with ftp if the path choise of the user(or auto selected) is correct.

    JacobSingh’s picture

    FileSize
    47.09 KB

    Okay, here is another update. I made this one from @yched's renaming / organization patch in #178

    I wasn't as careful with this one as I was with the previous ones where I was rooting out bugs. There may be issues here, but not DRUPAL_ROOT destroying issues. Apologies in advance if something is borked.

    I attempted to do the following:

    1. Provide some error handling in the batch ala update.php, re factored bits of update.php to make it flexible. I really don't like it, but it's important we have *something* in place.

    2. Start paving the way for using the same plumbing for fresh installs. This is by no means done, but hopefully on the right track. I added a way to ascertain the project type by reading its info file post download.

    We still have a lot of work to do here :(

    I think top 3 priorities are:

    A). Fix the windows paths issue

    B). Fix the chroot condition for FTP.

    C). Implement @catch's suggesting to prefer minor upgrades over major ones, provide an error for major upgrades.

    I think we start parallel work on Part 3, if nothing else, just a simple form which allows you to type the name of a module or theme and it will install it. The plumbing is almost ready for that.

    Let's GO!

    Jacob

    int’s picture

    Status: Needs review » Needs work
        * Operating in maintenance mode. Go online.
        * Update was completed successfully! Your site has been taken out of maintenance mode.
    

    I think that Operating in maintenance mode. Go online., should not show (because isn't in maintenance mode after the second line) :)

    The update was completed successfully! Isn't true..

    Update log
    update_test_module
    Update #1
    
        * Failed: Unable to change to directory /persistent/home/int/sites/drupal7/sites/all/modules/update_test_module
    JacobSingh’s picture

    FileSize
    47.09 KB

    For your first point:

    See #161. I don't know how to solve this unfortunately. We might have to do some trickery with drupal_set_message and the session.

    For the 2nd point:

    d'oh. At least it made some progress. I see it, just a typo really.

    This patch should fix it (cause it to actually look more broken).

    yched’s picture

    I think the 'Operating in maintenance mode. Go online' message that's displayed is the one set when displaying the batch page, which gets delayed until the 'finished' page (the batch page skips message rendering, so they are displayed on the next page display).
    I'd say it would make sense to adjust the code that displays the message to not display it when path is /batch.

    All the more that in non JS mode, the batch page auto-reloads itself, so it seems we'll get one 'Operating in maintenance mode' message per reload.

    int’s picture

    202 patch don't fix the "Update was completed successfully! Your site has been taken out of maintenance mode." when exists errors.

    JacobSingh’s picture

    FileSize
    47.1 KB

    arghh... sorry, missed another one.

    int’s picture

    With 205 patch, it's work's:
    Update failed! See the log below for more information. Your site is still in maintenance mode

    So, for me to continue to test, I need to set the ftp path to drupal(197 and 199).

    I'm wondering if we resolve the Fix the chroot condition for FTP. we will also fix this issue: Fix the windows paths issue

    bjaspan’s picture

    subscribe

    Anonymous’s picture

    FileSize
    46.72 KB

    Moved a few functions into helper functions. This should allow much easier integration of Part 4. (Rather than having to rewrite a large form.)

    yoroy’s picture

    Status: Needs work » Needs review
    JacobSingh’s picture

    Please, noone cross-post. I"m about to push a huge patch up which solves a bunch of bugs.

    JacobSingh’s picture

    Okay, I take it back. Everything is in:
    #528326: FTP FileTransfer classes need to be able to establish if they are chrooted

    For people who had problems with chroot before, please try this patch!

    int’s picture

    Now with the chroot fix, the update work's fine.

    A) When I update everything, still show this:
    There are security updates available for one or more of your modules or themes. To ensure the security of your server, you should update immediately! See the available updates page for more information.

    I have to refresh the page to show correctly.

    This error is the same?
    Operating in maintenance mode. Go online.

    B) And about Drupal Core? Isn't possible to upgrade automatically right? but I think that should also list there if is outdated.. At the top, without the checkbox.

    C) In Drupal 6 I use the update module also to see all the modules active, and find the project page. With this path we lose this option..
    The Modules page is slow and have lot of modules without use.. So I think that in the "Available updates" page we can have the list of others active modules here but hidden by default.. (without the checkbox)

    JacobSingh’s picture

    Yep, same problem. Anyone interested in taking a whack at it?

    -J

    JacobSingh’s picture

    Okay, I think the chroot problem has been rooted.

    We've got a couple left before we can get this committed.

    1. Broken for windows installs (I don't have a windows environment. Please, need some help here).
    2. Contrary to what was stated earlier, I don't think the settings form is at all accessible. Dmitri: If you could demonstrate how this works w/o JS it would be great, because I'd love to avoid the work, but as it is, it shows every form and can't submit.

    I only see one option for this, which I stated earlier (having a link to "change" the connection type, and only showing radios for the type of connection when a non-JS browser first goes to the page.

    Best,
    J

    dww’s picture

    Been out of town (and sick) for a while, just had a chance to catch up and read all 83 new comments since I last looked here. ;) Yikes.

    Frankly, at least for now, I think we need to keep the existing available updates report, exactly as-is. We should add this other page as another page. Trying to have a single page that does everything the existing available updates report does, and that works for this new feature, is going to be extremely difficult (if not impossible before freeze). I'm a little frustrated that I've spent a very long time on update_status, and put a lot of thought into it, and a lot of that is being ripped out with no alternative except a series of follow-up issues which are sure to die before code freeze. This patch is turning this page into something that only works for this feature (which makes sense). But not everyone wants this feature. Some people are still going to deploy via drush, cvs, svn, aegir, etc. They still want their available update status report, they just don't want to update in place to act on the results. Let's not make life suck for them, just so that life is better for the people who want to use this.

    I don't want to kill this feature for D7, but I'd rather it didn't kill a bunch of my prior work, either. With code freeze looming, I propose a compromise: add this as a separate page and leave the existing available updates report basically alone. But honestly, I don't have the energy, health, or time to keep fighting...

    JacobSingh’s picture

    @dww: Okay, I totally hear where you are coming from. I think we're all in agreement, the new updates form is what 90% of people using Drupal will want, so we don't want to jeopardize it. That being said, the other 10% who want a more detailed view are a very important bunch and we shouldn't throw out all that code.

    I'm strapped for time to get the updates done, mainly because of the 500 directions this thread has gone in and the amount of time I've had to spend on it. If you can help me (or someone else can help me) get the original page back in shape, share APIs and get it into the patch, I'd have no problem with it. (but please wait a day, I'm still solving the accessibility issue on the connection settings form - @Dmitri and @cwgordon, you both said you could make this form degrade, or it already did - now is the time to let me know what you are talking about).

    My other gripe is that this is going to set off another huge discussion about where menu items go, "usability" disputes, and the like which will kill this project, and I don't want to see that happen. So as we re-incorporate the older stuff, let's try to stay on task with the goal of getting the new updater in (working on all platforms and working w/o JS) as a must have, and anything else as a "nice-to-have".

    Best,
    Jacob

    JacobSingh’s picture

    FileSize
    41.82 KB

    Okay, now the page is functional for non-JS users and usable.

    I know it was mentioned earlier in #100 that it works without JS, but I'd say that if it is totally confusing and would make no sense to any user, it isn't working. Plus it doesn't work if someone turns off JS after visiting the site. That is the difference IMO between "graceful degradation" and "progressive enhancement" - although it's mostly just semantic BS.

    FYI, this is what it used to look like when someone didn't have JS:

    Available updates | dev7

    Also, if someone shut off JS after accessing the site once, it would screw up the batch and not work at all. This is a known bug
    #229825: backport "$_COOKIE['has_js'] must die" patch to 7.x

    Here is a video of what it looks like with this patch (sorry, will get a license / get a buddy to export tomorrow):
    http://pajamadesign.com/update_status/update_accessible.mov

    This was a PITA and complicates the flow a bit, but I feel that Drupal core should be 100% accessible. It is important not just because it's the right thing to do (it is), but if it is not, it will not be allowed in many governments, many NGOs and certain corporates as well.

    It also works in all cases - in spite of #229825: backport "$_COOKIE['has_js'] must die" patch to 7.x.

    Last tasks:

    1. Add back the old complex update_status report (hopefully without ALL the css that went with it). Dries - is this a blocker?

    2. Get it tested and working in Windows. Would really love a volunteer here to figure this out.

    Best,
    Jacob

    JacobSingh’s picture

    FileSize
    44.8 KB

    Okay! Implemented #162. I haven't *really* tested this yet, just fake testing, but finished the front-end.

    Here is what it looks like when you try to upgrade from to a new major version:
    Available updates | dev7

    And if you don't have have JS, you see this:
    Available updates | dev7

    Enjoy!

    New List:

    1). Fix working with Windows and windows FTP - actually, probably more related to #395472: Plugin Manager in Core: Part 1 (backend)(still would love a volunteer here).

    2). Get some good reviews and get this thing in!

    -J

    dww’s picture

    @Jacob: Apologies in advance if this sounds bitter, but what's wrong with the current UI for this? It already interfaces with d.o projects so maintainers can set a recommended version and specify all the supported versions. Update status gives you the best recommendation for your current branch, tells you if other branches are available, tells you what the maintainer says is recommended, etc. Why ditch all of that and reinvent the wheel? This is the sort of thing I've been trying to say since #114. I don't understand the process of this issue at all. It seems like all of the prior thought on update_status has been sent to /dev/null, and then y'all are iteratively trying to re-implement it as if for the first time... *shrug*. That said, it's nice having an extra warning about jumping major versions, instead of just saying "Also available: ...".

    JacobSingh’s picture

    @dww: Sorry man. Not intentional.

    I didn't realize that, I think I misunderstood @catch's post. I didn't know d.o. did that, just ignorance, nothing else. Changing the code to just use ['recommended'] is trivial and better. I can update the patch.

    The UI is kosher though?

    catch’s picture

    @dww: I think the problem is how/whether you give people a choice between a point release and a branch update when tableselect gives you one checkbox per row - if we have radios, or two checkboxes for the same project, it'd all start getting very messy. I don't see how that problem goes away with either the UI in this patch or the existing one. The least worst option I could think of was keeping sites on point releases until there aren't any left, then if there's a new branch, start offering that, but with a warning, which afaik was implemented in the latest revision of the patch.

    I agree with the niceties mentioned about the existing interface, if everything's up to date, it's nice to have a big green list of projects instead of an empty space, and there's not currently anywhere else you get a list of installed modules in any kind of useful state because the modules page is such a disaster. afaik there's an issue somewhere to divide the modules page up into installed an uninstalled modules, which might help the latter, but for now update status has worked for two purposes - what updates do I have to install, and what's the general status of my installed modules, and by design this issue would only deal with half of that.

    JacobSingh’s picture

    Hey guys,

    Time is really scarce. We can't get into this for too long if we want to see GUI based updates in 7 (and I think we all do). I agree w/ @catch, the UI we've got now is the simplest, most usable solution to minor / major updates. Personally, I don't think we need another screen, I think we should fix the modules screen, but I'm fine implementing till that gets done (if it ever gets done).

    In terms of a full listing of status, how about if we do this:

    Show a local task which goes to a page which is something like the old update status, but following a similar pattern (table) of the current one. This allows us to re-use many components of the current implementation to maintain consistency in look and feel and avoid adding back a huge amount of custom CSS and custom markup. If possible, we should add a sort to it too.

    Basically, the same list, but with no checkboxes and no filter, including core, and themed with table instead of tableselect. It's not about ditching the old code, most of it is the same, just trying to have consistent interfaces without custom markup and css as much as possible.

    Will that do the trick? Hell, we can even throw in an accordion(harmonica) with more information if we want to once that is finalized.

    I'm the main guy pushing out patches for this and responding to a lot of people, and I've got to get started on the install profile and installer tasks, and I'm behind on my day job, so please cut me some slack here and go easy on me here. Let's get specific, and get done.

    Best,
    Jacob

    -J

    int’s picture

    FileSize
    118.87 KB

    To get this path in, we can now, make this easy.. Just add one the old feature in one new page (comment #215), like tabpages.
    (others pages have this see the red box)

    (see the image)

    We can have: (one of this)
    Outdated modules - List modules
    Outdated modules - List active modules
    Auto Update modules - List active modules
    ...

    JacobSingh’s picture

    Yeah, that's exactly what I"m saying.

    We have to build that page of course, but it's probably just a 1-2hr job (as long as no one wants to debate over the color of backgrounds, icons, etc).

    int’s picture

    We need to keep the existing available updates report, exactly as-is (...) as another page. (dww #215)

    So just add the new page (as a tab), without any major change.

    catch’s picture

    Having it as a tab on the same page, +1, can't really visualize Jacob's suggestion vs. the current UI without seeing them side by side but I'm assuming dww would just like the functionality to be preserved rather than the exact look and feel. I do'nt really see why keeping the existing page is so hard though - since that code is already in core now.

    Oh, and no-one should go easy on core patch authors, that's why we have peer review in the first place :p

    JacobSingh’s picture

    Okay, so unless someone else is volunteering, I need exact specifics of what will stop this from being a block.

    And, I think you should go easy on everyone as much as you can. If they are core authors, or kindergarden students, or Siberian tigers :) Not that I always do. But besides that, what I meant was not "don't peer review" what I meant was, let's focus on getting something in. It's not fair to make me run up and down the stairs to get this in when people keep chiming in with new or old requirements throughout the process. If you think I'm bullshitting, look at the last 250 comments. Now if I could commit, I would have committed something functional as soon as it was decent and code reviewed and then started new issues for each specific gripe, but that's a pipe dream in this case it seems.

    So in terms of existing functionality that *needs* to be preserved, what is it?

    How about:

    A row which looks just like the current table.
    If a user clicks on the row (or a more link), they will get an accordion (non-JS get it open by default).
    What should go in the detail section? The dev release link? The "includes" section?
    Anything else?

    -J

    catch’s picture

    Spoke to Jacob in irc, here's what I'd suggest:

    We keep the existing update status, as is, but probably moved to a tab - pretty much what int described.

    The automatic updates page is now the default tab (it's useful even if you don't use automated updates since you get a single glance view of which modules are out of date).

    If we want to make the pages look more similar, or try to unify them back into one, or try to have 'module status' (as opposed to update status) merged into the new modules page, we can do that in other issues. That way we get the new feature without losing what's there currently. This isn't perfect, but IMO the two pages are actually offering very different things, even if there's overlap - but there's overlap with the module page as well, and it's not going to be possible to fix all of this in one patch.

    int’s picture

    FileSize
    106.93 KB

    I'm not sure what we are asking for..

    Se old feature is this (see the attach image), have the list of all active modules (outdated or not), with the project/download/release notes link, but also all the modules of that module.

    So we have:

    Drupal Core
    Modules
    Themes

    adrian’s picture

    Status: Needs review » Needs work

    I just completed a very critical review of this implementation of plugin manager which is far too in depth to post here :

    please check out http://groups.drupal.org/node/24709

    catch’s picture

    Highlights from that post which affect this issue:

    If you hand off a site to client, which is managed via version control, it becomes extremely easy for them to break your entire code deployment process by updating modules via the UI. So it's not just about advanced users ignoring this, they need to easily be able to disable it too (to the extent that it's hard for someone to re-enable again). Comments at #211747: Allow specifying version information as part of module dependencies suggest making update.module required, that'd make this very hard to switch off on sites which absolutely must not have automatic updates run on them ever, which is a fair proportion of sites.

    As such, this shouldn't be implemented in update.module. adrian has other reasons why it shouldn't be a module at all, but integrating into update.module itself seems like a definite no-go to me at this point.

    dww’s picture

    Status: Needs work » Postponed

    I agree. Adrian has much wisdom in his post. Everyone should read it, and no one should re-roll this until we get a decision from Dries on the direction to head. I'm all in favor of leaving update.module alone, and adding this as a completely separate system (which is easily disable-able).

    Anonymous’s picture

    After reading the post, or small novel ;), I'm inclined to agree with Adrian and dww. This present direction seems like it will be more of a headache both in the long and short term.

    Anonymous’s picture

    Okay, so from IRC and the aforementioned post it seems that the suggested direction is this:

    * PM should exists OUTSIDE of drupal (like update.php.) Otherwise, if an update kills Drupal, there might be no recovery available.
    * PM can interact with Drupal at any stage lower than DRUPAL_BOOTSTRAP_FULL. DRUPAL_BOOTSTRAP_FULL risks the equivalent of rootkits to be developed for PM.
    * PM should require authentication to update sites. To update sites/all/* the user needs to be able to authenticate with the installation at sites/default (or provide another means of proof of ADMIN-ADMINness...)
    * PM should only use FTP to change permissions. Actual updates will be done by the webserver. If we take care with file permissions this will likely be safer than sending FTP/SSH creditentials in plain-text.
    * PM should attempt to move files instead of replacing them. This allows a recovery to be made if needed.
    * PM should be easy to disable / opt out of.
    * (Optional) PM should be repository agnostic. It should work the same for drupal.org and timsdrupalmodulesandcrabshack.com.
    * (Optional) PM should check for modifications before replacing files.

    I must say that the arguments are quite compelling, even for the PHP writing PHP part. I approve of this new direction. Anyone else?

    JacobSingh’s picture

    Yeah, I'm more or less on board.

    However, if we can retain what we've for for D7 code freeze (with a kill switch), I'd prefer to do start with that. Too much work has gone into this to just toss it out, and a "newer-shinier-better" system may never materialize, and if it does, it may have new flaws.

    Further, tons of very good developers and Dries have watched this project progress and never questioned it fundamentally, that must mean that it's not "totally bad", right?

    I advocate ditching Part 3 for now, and getting updates and install profiles in, since they are almost done, and then revisiting it.

    What we've built is any day better than what is there (nothing) and I think it is a waste to lose it.

    -J

    adrian’s picture

    What people don't seem to get is that what we're going to be doing is taking the user interface and backend of this patch, and putting them in a new home (plugin.php) to begin with.

    The forms, the javascript etc will still be there, it just won't be part of a module but part of a clean white box, using the drupal maintenance theme and operating on a lower bootstrap.

    The backend changes involve simplifying existing code, and the current filetransfer system can stay in place for now.

    int’s picture

    It's a litle bit disapointing postponed this, when core reviews should allready have the architecture reviewed one month ago. :=)

    So lot's of hard work for almost nothing...

    So will we implement this as a new file? autoupdate.php, or this is for d8?

    adrian’s picture

    int: i think we can still get this in

    This code was written making some assumptions that nobody thought to question thoroughly, and adds complexity where none was needed for no benefit. This in no way makes it bad code.

    With some of the relatively simple changes in approach I am suggesting we will end up having a simpler system, that is more error tolerant and that allows more people to use it. We could get it working without needing to run a ftp or ssh server, by simply asking the user to change the permissions on a single directory manually, for instance.

    JacobSingh’s picture

    Adrian, let's pick a time (see my mail) to sit down and hatch a battle plan to get something done before code freeze.

    re: the last comments:

    1. As long as we do a partial bootstrap, we can salvage a lot of code probably.

    2. I have only 1 ground rule for this project: In no way should a user have to learn how to use FTP or SSH.

    -J

    Senpai’s picture

    Status: Postponed » Active

    Nor should they have to change the permissions on a single directory. Hear hear!

    Dries has given his input at http://groups.drupal.org/node/24709#comment-85433 and http://groups.drupal.org/node/24709#comment-85474, and Adrian + Jacob have their brains wired together in an IRC back room somewhere.

    This is no longer postponed, so it's going back to active, and don't anybody roll another patch for it until we have a revised plan of attack with some elementals, wireframes, and a workflow.

    dww’s picture

    Status: Active » Postponed

    That's why this is postponed. There are lots of things that need to happen before it's worth working directly on this issue and these patches again...

    JacobSingh’s picture

    Status: Postponed » Closed (duplicate)

    This issue is getting a new home:

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

    Best,
    Jacob

    dww’s picture

    Status: Closed (duplicate) » Closed (won't fix)

    Actually, I think "won't fix" is more appropriate, since we decided not to integrate PM with update.module. But yeah, either way. This issue is dead (although it contains lots of effort and wisdom, none of which will be lost/forgotten in the new effort). Thanks!

    p.s. I'm going to preemptively lock comments on this issue, since there's no more to discuss here...

    Senpai’s picture

    Status: Closed (won't fix) » Closed (fixed)

    Changing to fixed/closed, since this is really a redirection rather than a wont fix, and we need to be accurate rather than obtuse. Everyone who wants to help , please see #536630: [Meta issue] Plugin manager for D7 code freeze spec, but don't forget about all the discussion in this thread since there's been many, MANY hours of free work invested into this.