The recent plugin manager work broke the base drupal install and the main admin/ menu. A few people noticed while Drupal.org and update.drupal.org were sending 503 errors that we had XML errors while installing and the admin/ menu.

The bug is kind of hard to reproduce, because you need update.drupal.org to return something that is not valid XML while still returning something.

The solution is to not flush the caches so aggressively, but only if we can actually fetch the updates from update.drupal.org. We double-check we receive a proper status code from the server before treating its data as proper.

Files: 
CommentFileSizeAuthor
#15 565288-15-tests-and-simple-fix.patch2.78 KBbfroehle
PASSED: [[SimpleTest]]: [MySQL] 31,572 pass(es).
[ View ]
#13 565288-error-503-test.patch2.25 KBbfroehle
FAILED: [[SimpleTest]]: [MySQL] 31,565 pass(es), 2 fail(s), and 6 exception(es).
[ View ]
#6 common_inc_fix.patch2.82 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 common.inc_.patch2.42 KBp.brouwers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch common.inc__23.patch.
[ View ]
#2 update_xml.patch2.63 KBanarcat
Failed: Failed to apply patch.
[ View ]
update_xml.patch2.47 KBanarcat
Failed: Failed to apply patch.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch failed testing.

anarcat’s picture

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

Last patch was relative, fixed the paths... sorry about that.

Status:Needs review» Needs work

The last submitted patch failed testing.

p.brouwers’s picture

StatusFileSize
new2.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch common.inc__23.patch.
[ View ]

Your patch was bases on an old version of D7.
This one should do the trick

sun.core’s picture

Component:install system» update.module
Priority:Critical» Normal

Looks like you didn't manage to attract the right folks to this issue due to a wrong component.

aspilicious’s picture

StatusFileSize
new2.82 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Did a reroll don't know if it's still relevant.

aspilicious’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, common_inc_fix.patch, failed testing.

aspilicious’s picture

hmm bot glitch?

Dave Reid’s picture

In general coding and documentation standards need a lot of work. Comments need to be wrapped at 80 characters, functions have starting curly brace on the same line as function name, etc. We also probably need a unit test for this function to ensure it works properly, since I'm guessing that's why we're getting the test failure.

+++ includes/common.inc 23 Mar 2010 12:33:54 -0000
@@ -6124,6 +6124,46 @@
+ * @param array $array1
+ * @param array $array2
+ * @return array
+ * @author Daniel <daniel (at) danielsmedegaardbuus (dot) dk>
+ * @author Gabriel Sobrinho <gabriel (dot) sobrinho (at) gmail (dot) com>

Does not follow standards for @param or @return. We don't also put credits in functions like this.

Powered by Dreditor.

aspilicious’s picture

True, I don't know how the function works, just wanted to push this but I admit I didn't look at the awful comment style!

dww’s picture

#4 and #6 seem to be completely unrelated to this bug, unless someone can explain how they're connected.

Note, it should be possible to reproduce this via creative use of the 'update_fetch_url' variable.

bfroehle’s picture

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
FAILED: [[SimpleTest]]: [MySQL] 31,565 pass(es), 2 fail(s), and 6 exception(es).
[ View ]

Here's a test ...

Status:Needs review» Needs work

The last submitted patch, 565288-error-503-test.patch, failed testing.

bfroehle’s picture

Assigned:anarcat» Unassigned
Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new2.78 KB
PASSED: [[SimpleTest]]: [MySQL] 31,572 pass(es).
[ View ]

I added the following bit of code which prevents XML parsing if drupal_http_request had any sort of error, like not being able to successfully fetch the xml data.

--- modules/update/update.fetch.inc
+++ modules/update/update.fetch.inc
@@ -144,7 +144,7 @@ function _update_process_fetch_task($project) {

   if (empty($fail[$fetch_url_base]) || $fail[$fetch_url_base] < $max_fetch_attempts) {
     $xml = drupal_http_request($url);
-    if (isset($xml->data)) {
+    if (!isset($xml->error) && isset($xml->data)) {
       $data = $xml->data;
     }
   }

I didn't touch any cache clearing code (as in #2, for example), as that seems to be working okay for the moment.

Changed the relevant test slightly to also look for the text

'Failed to get available update data for one project.'

bfroehle’s picture

Version:7.x-dev» 8.x-dev
Issue tags:-Needs tests+needs backport to D7

Tests are in #13 and the patch+tests are in #15.

The actual change to runtime code is small:

     $xml = drupal_http_request($url);
-    if (isset($xml->data)) {
+    if (!isset($xml->error) && isset($xml->data)) {

The remainder of the patch just adds a test case as evidenced in #13.

claudiu.cristea’s picture

Version:8.x-dev» 7.x-dev
Status:Needs review» Reviewed & tested by the community

#16 tested against 7.x and is working.

bfroehle’s picture

Version:7.x-dev» 8.x-dev
Dries’s picture

Status:Reviewed & tested by the community» Fixed

Reviewed this patch and decided to commit it to 7.x and 8.x. Good catch! :)

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

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