Currently the update script only allows the first user to run the update script. A lot of drupal sites have more than one administrator. I think it's best to change the permission check from

if (($access_check == FALSE) || ($user->uid == 1)) {

to something like

if (($access_check == FALSE) || user_access('update')) {

Files: 
CommentFileSizeAuthor
#96 67234-update-permission-D7.patch15.73 KBDave Reid
Passed: 13619 passes, 0 fails, 0 exceptions
[ View ]
#93 67234-update-permission-D7.patch15.75 KBDave Reid
Passed: 13557 passes, 0 fails, 0 exceptions
[ View ]
#89 67234-update-permission-D7.patch30.22 KBDave Reid
Passed: 13580 passes, 0 fails, 0 exceptions
[ View ]
#87 67234-update-permission-D7.patch25.95 KBDave Reid
Failed: Invalid PHP syntax in modules/update/update.test.
[ View ]
#86 67234-update-permission-D7.patch24.83 KBDave Reid
Failed: 13689 passes, 35 fails, 0 exceptions
[ View ]
#81 drupal.update-access-RTBC.patch15.71 KBsun
Passed: 13593 passes, 0 fails, 0 exceptions
[ View ]
#80 67234-update-permission-D7.patch12.02 KBDave Reid
Passed: 13543 passes, 0 fails, 0 exceptions
[ View ]
#79 drupal.update-access-getabsoluteurl.patch14.03 KBsun
Failed: 13701 passes, 3 fails, 0 exceptions
[ View ]
#74 67234-update-permission-D7.patch13.75 KBDave Reid
Failed: 13684 passes, 37 fails, 50 exceptions
[ View ]
#68 67234-update-permission-D7.patch11.15 KBDave Reid
Failed: 13733 passes, 3 fails, 1 exception
[ View ]
#62 67234-update-permission-D7.patch11.2 KBDave Reid
Failed: 13710 passes, 3 fails, 1 exception
[ View ]
#60 67234-update-permission-D7.patch11.05 KBDave Reid
Failed: 13712 passes, 3 fails, 1 exception
[ View ]
#58 67234-update-permission-D7.patch11.04 KBDave Reid
Failed: 13727 passes, 9 fails, 1 exception
[ View ]
#56 67234-update-permission-D7.patch11.04 KBDave Reid
Failed: 13735 passes, 9 fails, 1 exception
[ View ]
#50 67234_administer_site_updates_50.patch8 KBDavid_Rothstein
Passed: 13566 passes, 0 fails, 0 exceptions
[ View ]
#47 67234_administer_site_updates_47.patch6.41 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#46 67234_administer_site_updates_46.patch7.08 KBDavid_Rothstein
Failed: Failed to apply patch.
[ View ]
#41 67234_administer_site_updates.patch5.27 KBmikejoconnor
Failed: Failed to apply patch.
[ View ]
#39 67234-39.update_permission.d7.patch5.26 KBdww
Failed: Failed to apply patch.
[ View ]
#35 67234.patch5.16 KBRobLoach
Failed: Failed to apply patch.
[ View ]
#31 67234-31.update_permission.d7.patch4.19 KBdww
Failed: Failed to apply patch.
[ View ]
#30 67234-30.update_permission.d7.patch4.2 KBdww
Failed: Failed to apply patch.
[ View ]
#25 67234-update-permission-D7.patch8.88 KBDave Reid
Failed: 10720 passes, 3 fails, 0 exceptions
[ View ]
#16 67234-update-permission-D7.patch9.85 KBDave Reid
Failed: 10595 passes, 9 fails, 1 exception
[ View ]
#14 67234-update-permission-D7.patch9.84 KBDave Reid
Failed: 10653 passes, 9 fails, 1 exception
[ View ]
#13 67234-update-permission-D7.patch9.84 KBDave Reid
Failed: 10653 passes, 9 fails, 1 exception
[ View ]
#7 update-script-access-rights_3.patch1.63 KBRalf
#5 update-script-access-rights_2.patch596 bytesRalf
#3 update-script-access-rights.patch571 bytesRalf

Comments

Version:4.7.2» x.y.z
Category:bug» feature

-1 though, I do not like the idea. Others might.

make sense

but it should be user_access('administer site configuration')

because users who have permission to enable and disable modules should be authorized to run the update script too.

Status:Active» Needs review
StatusFileSize
new571 bytes

I think this change would be logical and user friendly. Why not allow uid = 1 to give this permission to others?

StatusFileSize
new596 bytes

patch re-rolled.

updating the site is irreversible and serious. i don't think 'administer site configuration' is enough ... personally, i would like to see a built in superuser role which behaves just like uid=1. this idea was dismissed years ago but we can still try again :)

StatusFileSize
new1.63 KB

Added new permission 'administer modules configuration' to patch.

Only users with this permission can enable modules and run the update script.

in my opinion this change is warrented is some way or the other. Simply because it has to be down very often because many modules now also use the updat system. It is *not* like before when an update was only done when a new version of core appeared. Also you need ftp access to really change anything, without file changes running update will have no consequences.

Version:x.y.z» 6.x-dev
Status:Needs review» Needs work

patching file update.php
Hunk #1 succeeded at 768 with fuzz 1 (offset 71 lines).
(Stripping trailing CRs from patch.)
patching file modules/system/system.module
Hunk #1 FAILED at 52.
Hunk #2 FAILED at 187.

I'm also not sure of this. Given that update.php touches the database (and sometimes in destructive ways), it is quite reasonable to demand that the person who runs update.php have enough permissions to ssh into the server and change $access_check to FALSE if they don't have the uid 1 credentials. Plus, if the database is totally fubar (as it can be during a major version update) I'm not sure the user_access() check would necessarily always work anyway.

Version:6.x-dev» 7.x-dev

bumping. access check was moved to settings.php right? maybe this should be won't fix with that change.

Just marked #382874: Add permission to run update.php as a duplicate and would like to re-roll for 7.x.

Adding tags.

Status:Needs work» Needs review
StatusFileSize
new9.84 KB
Failed: 10653 passes, 9 fails, 1 exception
[ View ]

Re-rolled patch from #382874: Add permission to run update.php, and adding a few thing I thought were useful.

1. Sends a 403 denied PHP header and logs the access denied in watchdog if a user is denied access to update.php.
2. Wrote TESTS! YAY!
3. Added a simple update_l() function so it's easier to write links in update.php, and also gets rid of writing un-clean urls (http://mysite.com/?q=admin) when it should just be linking to http://mysite.com/admin.

StatusFileSize
new9.84 KB
Failed: 10653 passes, 9 fails, 1 exception
[ View ]

Teeny tiny revision renaming the test class from UpdateFunctionalTest to UpdateScriptFunctionalTest.

Should be ready for review!

Assigned:Unassigned» Dave Reid
Issue tags:-Needs tests

StatusFileSize
new9.85 KB
Failed: 10595 passes, 9 fails, 1 exception
[ View ]

Fudging the order of update_access_denied_page() just a bit. Last revision before bed I promise.

Status:Needs review» Needs work

The last submitted patch failed testing.

Sasha Chua brought up an issue related to this recently in her blog: Drupal Gotcha: Watch out for $user during update.php. It isn't really related to update.php, but Drupal's dependency on $user->uid == 1.

All the Drupal websites I create, I always end up hacking Drupal's update.php and replace this check with a user_access('administer site configuration')-like statement, so I'd love to see this go in. I like where the patch at #16 is going.

Status:Needs work» Needs review

Resubmitting to the testing bot, I'm getting passes.

Status:Needs review» Needs work

The last submitted patch failed testing.

Oh! That's more about if you set the access check to false, you might not have $user->uid 1... =)

Status:Needs work» Needs review

Wow I cannot duplicate these testbot results. :/

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.88 KB
Failed: 10720 passes, 3 fails, 0 exceptions
[ View ]

Testing a simplified test version.

Status:Needs review» Needs work

The last submitted patch failed testing.

Yep. That confirms it. I apparently can't test users accessing update.php.

Yeah update.php is untestable currently. But that means we can get this in with no tests as long as someone runs it manually quickly.

In an attempt to get some of the hacks we run on d.o into drupal core, long ago I submitted #104868: allow array of update.php super users. That ended up being won't fixed in favor of this issue, in the hopes this would make it into D5... 2 major release later... ;)

Of course we need this patch -- this is one of the most frequently hacked lines of core. We do it on d.o. For (hopefully) obvious reasons, we never want to just disable the access check on d.o/update.php by editing the file via the shell, but it's probably been 2+ years since uid 1 ran any DB updates on this site. So, we've got an array of uids that are also allowed. As the comments here suggest, lots people hack this line of core for the obvious reason that the functionality core provides out of the box is far too limited to be useful in many cases.

If "don't hack core" is supposed to be such an important slogan/mantra for our project, then we have to get rid of stupid code that everyone has to hack to get their work done...

So, looking at the patch:

A) "Added a simple update_l() function..." That's a totally separate feature request. Makes this patch harder to review, and less likely to get in. Can we move that to a separate issue and simplify this patch? #include "kittens.h";

B) "Administer site updates" probably shouldn't be wrapped in code tags in the update_access_denied_page() text, should it? Don't we usually use em for things like that in other parts of core? Also, semantically, it's not code...

C) Seems like we should remove the tests from this patch.

This patch really only needs to be about 10 lines long. That'd help get it in quicker.

Thanks,
-Derek

Status:Needs work» Needs review
StatusFileSize
new4.2 KB
Failed: Failed to apply patch.
[ View ]

StatusFileSize
new4.19 KB
Failed: Failed to apply patch.
[ View ]

Whoops, forgot 29.B.

subscribing - thanks, catch! :)

Status:Needs review» Reviewed & tested by the community

Looks great to me.

Status:Reviewed & tested by the community» Needs work

+    'administer site updates' => array(
+      'title' => t('Administer site updates'),
+      'description' => t('Run the update.php script.'),
+    ),

Hm. There is not much to "administer" with this permission. I would rather opt for:
+    'access site updates' => array(
+      'title' => t('Execute site updates'),
+      'description' => t('Run the update.php script.'),
+    ),

...or similar.

- * If you are not logged in as administrator, you will need to modify the access
- * check statement inside your settings.php file. After finishing the upgrade,
- * be sure to open settings.php again, and change it back to its original state!
+ * If you are not logged in as administrator or do not have the
+ * 'administer site updates' permission, you will need to modify the
+ * access check statement inside your settings.php file. After
+ * finishing the upgrade, be sure to open settings.php again, and
+ * change it back to its original state!

Wraps at 70 instead of 80 chars.

-  return '<p>Access denied. You are not authorized to access this page. Please log in as the admin user (the first user you created). If you cannot log in,
+  return '<p>Access denied. You are not authorized to access this page. Please give your user the <em>Administer site updates</em> permission, or log in as the admin user (the first user you created). If you cannot log in,

Please grant your user the..., I think.

Status:Needs work» Needs review
StatusFileSize
new5.16 KB
Failed: Failed to apply patch.
[ View ]

This patch takes the recommendations from sun as well as updates default.settings.php to include documentation about the new "access site updates" permission.

As a side note, trying not to bikeshed everything, you kind of do "Administer site updates" when you visit update.php, because you choose which database updates you want to perform. Any thoughts?

A while ago, we got rid of the ability to choose which updates to perform.

Thanks for re-rolling.

+ <li>To avoid having this problem in future, remember to log in to your website as a user with the <em>Administer site updates</em> permission, or as the admin user (the user you first created) before you backup your database at the beginning of the update process.</li>
...
+$update_access_allowed = !empty($update_free_access) || user_access('administer site updates');

Two remaining instances of 'administer site updates'.

Please also note that

+    'access site updates' => array(
+      'title' => t('Execute site updates'),

was a quick proposal only. Actually, I don't know whether we usually keep the internal and human readable names consistent or not. If we do, "Access site updates" might sound a bit strange as permission title, while the internal name probably makes most sense to developers. Bah. Better suggestions welcome.

UX team-compatible output of this permission:

[ ] Access site updates
Run the update.php script.

Let's point someone of them here. (Bojhan/yoroy/...)

.

StatusFileSize
new5.26 KB
Failed: Failed to apply patch.
[ View ]

I'm still in favor of "Administer site updates". It's possible this permission (c|sh)ould be reused over at #395472: Plugin Manager in Core: Part 1 (backend). It's also more consistent with other core permissions, and self-documents itself as an "admin" thing not to give out lightly. If I was brand new, and wasn't closely reading the help text, and I just saw "execute site updates" I might think that gives people the permission to "update" the site by changing or adding new content, etc.

The previous patch didn't complete the conversion, anyway, and still had a few word-wrap errors...

So, here's a re-roll incorporating the documentation for default.settings.php, leaving everything as "Administer site updates", but otherwise addressing sun's previous concerns and Rob Loach's improvements.

@UX reviewers, the question is which of these do you like on the permission settings page:

A) "Administer site updates" (this patch)

B) "Execute site updates" (patch #35, but needs work)

C) "Access site updates" (proposal from #38, no patch)

D) Something better y'all propose. ;)

I'd got for A for the reasons dww gives - lets us use it more flexibly later on.

StatusFileSize
new5.27 KB
Failed: Failed to apply patch.
[ View ]

I'm for "Administer site updates". Here's a new patch to correct a stray "access site updates" in the comments, and changes an "administer" to "Administer".

I agree that "Administer site updates" is best. But ... how about "Perform Drupal upgrades"?

That is, how about (i) administer vs perform, (ii) updates vs upgrades and (iii) Drupal vs site? :)

A "site update" could potentially be very confusing -- site owners update their site all the time ... in their minds, adding content might be updating their site.

Status:Needs review» Needs work

Running user_access() inside update.php currently results in a fatal error on 6-7 updates because the {user_roles} table name has changed.

That update probably needs to be moved to update_fix_d7_requirements() anyway in case an enabled module calls user_access() in hook_boot() or something, but makes sense to do it here.

I don't think we should do iii (Drupal vs. site) because we generally go way out of our way to not embed the word "Drupal" in anything user-facing to allow for white-label distributions. Esp. permissions which, although they are now translatable in D7, are hard-coded strings in the code.

I don't mind (ii) "upgrades" not "updates", but I'm opposed to (i) and (iii). As I wrote in comment #39:

I'm still in favor of "Administer site updates". It's possible this permission (c|sh)ould be reused over at #395472: Plugin Manager in Core: Part 1 (backend). It's also more consistent with other core permissions, and self-documents itself as an "admin" thing not to give out lightly. If I was brand new, and wasn't closely reading the help text, and I just saw "execute site updates" I might think that gives people the permission to "update" the site by changing or adding new content, etc.

So, "Administer site upgrades" would be ok, and further differentiate this permission from "updating (content on) your site"... However, if we're going to reuse this in update.module, it's a little weird that update.module uses a permission called "upgrade". /me shrugs...

Status:Needs work» Needs review
StatusFileSize
new7.08 KB
Failed: Failed to apply patch.
[ View ]

This issue seems worth reviving - it really should be fixed.

Here is an updated patch. For the permission name, I went with "administer software updates" -- it's simple, clear, and could easily be extended to the Plugin Manager if necessary.

Notes:

  1. To deal with #43, I wrapped the call to user_access() in a try/catch block. This isn't likely to actually work right now, and D6->D7 upgrades are broken at an earlier stage of the bootstrap anyway (without this patch), both due to #517742: Upgrade failing if blocked_ips doesn't exist. Also note #524710: role_permission required to upgrade from D6 to D7... overall, this is kind of a mess that needs to be cleaned up later anyway, but I think this patch is fine for now.
  2. The lines in the patch that change from using drupal_get_token() to drupal_valid_token() are necessary to support assigning the new permission to anonymous users. That's a rare use case, I suppose, but if it's a real permission, we probably should be supporting it.

StatusFileSize
new6.41 KB
Failed: Failed to apply patch.
[ View ]

OK, never mind what I said about drupal_valid_token(). That is actually a separate bug (#567826: $update_free_access does not always allow access to update.php) and it's not at all clear that the method used above is the right way to fix it.

So, here's a new patch that does not include the token validation changes.

Status:Needs review» Reviewed & tested by the community

Tested and works.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8 KB
Passed: 13566 passes, 0 fails, 0 exceptions
[ View ]

Hi,

I just stumbled on this topic, and I am very interested in getting this patch working!

Our problem arises from the fact that we use the Webserver authentification module (driven by an LDAP directory) and we cannot create the "admin" account on this LDAP directory, so running updates is impossible...

Status:Needs review» Needs work

Huh. I totally thought I'd already committed this patch. Anyway, I'd like to. ;)

Please also make visibility of the update status error messages contingent on this new permission. In other words, marked #332796: Add permissions to the update.module to hide warnings as a duplicate.

I can get this rerolled in the next hour.

@Dave Reid? Anyone? Re-roll?

Ok, going to stop promising that I can do this within the hour, but on my MUST-DO-LIST for today.

Status:Needs work» Needs review
StatusFileSize
new11.04 KB
Failed: 13735 passes, 9 fails, 1 exception
[ View ]

Re-rolled with PASSING TESTS! YAY!

Status:Needs review» Reviewed & tested by the community

Lovely little patch. Please wait for green before commit.

StatusFileSize
new11.04 KB
Failed: 13727 passes, 9 fails, 1 exception
[ View ]

Trivial revised patch that fixes two instances of "administer software updates" to "Administer software updates" in the user interface.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new11.05 KB
Failed: 13712 passes, 3 fails, 1 exception
[ View ]

Trying again not using url() to get the path to update.php.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.2 KB
Failed: 13710 passes, 3 fails, 1 exception
[ View ]

Trying to write tests for update.php is apparently a major pain with this test bot. Revised patch that sends me debugging e-mails. DO NOT COMMIT!

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

I could really use the help of a testbot admin to help figure out what is going wrong here. I just spend the last half hour trying to get my own local pifr install so I could maybe debug it, but I am getting way too frustrated at the lack of documentation, discrepancies between versions of SimpleTest, etc.

+++ modules/system/system.test 7 Oct 2009 03:44:16 -0000
@@ -1290,3 +1290,61 @@ array_space[a b] = Value';
+    $this->drupalGet($update_url);

$this->drupalGet($update_url, array('external' => TRUE));

I'm on crack. Are you, too?

So what you have to take into consideration:

- Tests run with clean URLs disabled.

- url() builds a URL to 1) a virtual system (Drupal) path, or 2) an external URL.

- update.php is not a system path, so it is an external URL.

- You may think that you could also use the 'absolute' property, but that does not work, because language rewriting, URL aliasing, and other funky stuff happens.

You don't need to install PIFR to replicate this. Just use your install of HEAD, disable clean URLs, and run that test.

Is there any chance to see this patch land in Drupal 6?

StatusFileSize
new11.15 KB
Failed: 13733 passes, 3 fails, 1 exception
[ View ]

@sun: Awesome. Revised patch posted that will hopefully pass. It was too late for me to be rolling this patch. Doesn't SimpleTest change the clean_url variables when it runs?

@sebos69: Since this is a feature request, no.

Status:Needs review» Needs work

The last submitted patch failed testing.

Ok still had 3 fails and 1 exception. Any other ideas?

ARG. Because the update form doesn't set an exclipit action in its form tag, drupalPost() uses getAbsoluteUrl() on 'update.php' which then tries to attempt to POST to 'http://mysql.drupal7dev.local/?q=update.php&op=selection&token=32b071f90...' which of course is a 404. ARG.

Could we add in an $absolute = TRUE parameter to drupalPost()?

Status:Needs work» Needs review

The problem is getAbsoluteUrl(). I'm trying to solve that nightmare... it's the second time that this ugly code doesn't work at all. (the lengthy comments in there are mine)

StatusFileSize
new13.75 KB
Failed: 13684 passes, 37 fails, 50 exceptions
[ View ]

Revised patch with a small change to DWTC->getAbsoluteUrl() that gets the $options for url() from it's caller. Tested and passes locally. Let's give this a shot with the test bot now.

This could have been fixed just by manually adding $base_url to update.php form action to make it a full URL, but that didn't seem right since we'll likely encounter this again later.

Regardless of that getAbsoluteUrl() problem, your first assertions for 403 in the tests are failing.

Test bot passed them all and they pass locally:
Update.php functionality 50 0 0

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

#$^# I give up on this for now.

StatusFileSize
new14.03 KB
Failed: 13701 passes, 3 fails, 0 exceptions
[ View ]

Let's see what breaks.

StatusFileSize
new12.02 KB
Passed: 13543 passes, 0 fails, 0 exceptions
[ View ]

Ok until we can get getAbsoluteUrl() working, this should pass.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new15.71 KB
Passed: 13593 passes, 0 fails, 0 exceptions
[ View ]

There you go. :)

Holy dear lord, that was way too frustrating and huge thanks to sun for helping figure that out. Everything looks good, it passes locally, and for the first time the fracking test bot likes it.

RTBC'd!

So the issue was that the drupalPost() function had to extract the 'action' URL from the form that it is submitting. In this case the action in update.php was 'update.php?stuff'. When that is run through url() it gets converted into http://mysite.com/?q=update.php, which is wrong. So now we have proper external and absolute handling for drupalPost, which is a Good Thing(tm).

Status:Reviewed & tested by the community» Needs work

Excellent, great work y'all!

However, I hate to do this, but in all the excitement about fighting the test infrastructure, y'all forgot webchick's request in #52 to solve #332796: Add permissions to the update.module to hide warnings while we're at it. ;) Seems like she's saying that the 'Administer software updates' permission should determine access to all of Update status's menu items and to the warning messages that are set via hook_requirements() in admin/* when the site is missing security updates. A very quick skim of this patch shows no such changes. Anyone up for a re-roll? Sorry!

I'm on it.

Status:Needs work» Needs review
StatusFileSize
new24.83 KB
Failed: 13689 passes, 35 fails, 0 exceptions
[ View ]

Revised patch that changes all of update.module's menu permissions from 'administer site configuration' to 'administer software updates' as well as the notifications from update_requirements (also moved to update.install to shrink down the update.module file size a little).

StatusFileSize
new25.95 KB
Failed: Invalid PHP syntax in modules/update/update.test.
[ View ]

Forgot to adjust update.test.

Status:Needs review» Needs work

+    return ($user->uid === 1);

Why not simply return $user->uid == 1; here?

+    // Proceed through the update process without any pending updates.
+    $this->drupalPost(NULL, array(), t('Continue'), array('external' => TRUE));
+    $this->assertText(t('No pending updates.'));
+
+    // Click through back to the administration page.
+    $this->clickLink('Administration pages');
+    $this->assertResponse(200);

The function is called testUpdateAccess() but this part is trying to do a bit more, and in a very fragile way. If we're going to try to test that update.php works correctly, I suggest we do that in a separate issue.

The "click back to the administration page" part seems especially out of place and fragile... though it does point out a (very minor) bug with this patch. In order to get the test to pass, you needed to grant the 'access administration pages' permission in addition to 'administer software updates'. So someone who only has the latter permission would get access denied after clicking on this link.

**

Finally, I was about to point out the same thing as @dww did, but honestly, this patch has already expanded from a simple permission to fixing all of SimpleTest :) I would suggest that #332796: Add permissions to the update.module to hide warnings be reopened as a followup to this instead. It's not totally clear to me that someone without the 'administer software updates' permission should never see any kind of warning message at all - it's equally possible that they should see a message with different text (if, for example, they have 'administer site configuration' and thus are still partially responsible for the overall maintenance of the site).

Status:Needs work» Needs review
StatusFileSize
new30.22 KB
Passed: 13580 passes, 0 fails, 0 exceptions
[ View ]

Moved _update_requirement_check() to update.install as well since it is only called by update_requirements().

Status:Needs review» Needs work

Nicely done!

+    'administer software updates' => array(
+      'title' => t('Administer software updates'),
+      'description' => t('Run the update.php script.'),
+    ),

That description isn't sufficient anymore. ;)

(BTW, I agree with David_Rothstein in #88 -- it's not clear to me why we're trying to tackle all the update status parts at the same time. Guess that's what webchick wanted, but it sure seems harmful to kittens).

Status:Needs work» Needs review

What should be put for the permission description since it adds features when update.module is installed?

The tests for testing the click-through of update.php found bugs in SimpleTest so I think we should just keep those small ones. Tests aren't a bad thing.

StatusFileSize
new15.75 KB
Passed: 13557 passes, 0 fails, 0 exceptions
[ View ]

Re-rolling a simplified patch *just* for permission to update.php. No changes to update.module (split back to #332796: Add permissions to the update.module to hide warnings). This is essentially the same patch as #81 but with only a test for access to update.php, and not the click-through test. There are a few other 403 links on the update results page that I want to fix so I'll create a separate issue for that.

Component:update system» update.module

Moving to proper component.

Component:update.module» update system

Arg...wrong patch. Sorry everyone.

StatusFileSize
new15.73 KB
Passed: 13619 passes, 0 fails, 0 exceptions
[ View ]

Left in an accidental private $admin_user in the test.

Status:Needs review» Reviewed & tested by the community

The patch looks great now, and I confirmed by hand that it "works" with D6->D7 updates also (which is to say, it doesn't break D6->D7 updates any more than they were already broken).

Now that we have the ability - thanks to the SimpleTest changes - I'll create a followup issue to do a test of running through update.php and checking that it performs/skips updates correctly.

I found an existing issue at #383196: update.php tests, so I added a comment there instead.

Status:Reviewed & tested by the community» Needs work

Did you remove the debug email lines?
Or does DREditor have a bug?

@spatz4000: Please don't mark issues "needs work" if you don't even look at the patch. ;) http://drupal.org/files/issues/67234-update-permission-D7_14.patch from comment #96 includes no stray debugging messages. It's still RTBC...

Status:Needs work» Fixed

Yeah, when I first came across #332796: Add permissions to the update.module to hide warnings, it seemed a natural extension of this, but there are compelling arguments over there for making it a separate permission.

In the meantime, this looks great, and since I already told all of DC Paris that I committed this patch anyway, committed to HEAD. ;)

Please follow-up with an issue in the Documentation queue about this, since this change will affect several handbook pages.

Status:Fixed» Closed (fixed)
Issue tags:-Needs usability review, -permissions, -update.php

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