Taking a look at update_helpful_links(), you see it mentions:

we can't use l() here because the URL would point to 'core/update.php?q=admin'.

  1. This is actually very untrue. Using "absolute", you can reference direct pages
  2. This is also broken since we switched to index.php/%page, so ?q=admin is obsolete
  3. Hard-coding HTML is ew, let's switch to theme_links()
Files: 
CommentFileSizeAuthor
#18 update-comment-1648004-18.patch573 bytesdcam
PASSED: [[SimpleTest]]: [MySQL] 40,097 pass(es).
[ View ]
#13 improve-helpful-links-1648004-13.patch1.76 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 39,380 pass(es).
[ View ]
#6 before.png16.33 KBdcam
#6 after.png16.4 KBdcam
#2 improve-helpful-links-1648004-2.patch1.79 KBDevin Carlson
PASSED: [[SimpleTest]]: [MySQL] 40,345 pass(es).
[ View ]
unhelpfullinks.patch1.84 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 37,010 pass(es).
[ View ]

Comments

Issue tags:+Novice

It's an easy patch review.

StatusFileSize
new1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 40,345 pass(es).
[ View ]

The patch in the original issue looks good but I think that we could also change $form['links'] from "#markup" to "#theme => 'links'" to make it easier to override.

Also, I don't see any reference to "absolute" in the theme_links API docs. However, I tested the update page without "absolute" and the links continued to work (even without clean URLs in Drupal 7).

Status:Needs review» Needs work

The last submitted patch, improve-helpful-links-1648004-2.patch, failed testing.

Status:Needs work» Needs review

Hmm, one test failure in comment module? The test is passing locally for me so trying once more before digging deeper.

#2: improve-helpful-links-1648004-2.patch queued for re-testing.

StatusFileSize
new16.4 KB
new16.33 KB

The patch looks good to me. It was tested by running update.php before and after applying, both with and without updates to apply. Screenshots from the tests without updates are attached.

#2: improve-helpful-links-1648004-2.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Nice clean-up!

Committed and pushed to 8.x. Thanks!

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

Rerolled for D7.

StatusFileSize
new1.76 KB
PASSED: [[SimpleTest]]: [MySQL] 39,380 pass(es).
[ View ]

It helps to actually attach the patch.

Status:Needs review» Reviewed & tested by the community

The patch in #13 is a backport of the patch for D8 in #2 and makes the same changes. The patch applied cleanly and resulted in proper links being generated to both the front and administration pages after an update was performed.

Assigned:Unassigned» David_Rothstein

Hm. Typically we can't make data structure/markup changes like this in a stable release. Assigning to David to get his thoughts.

Assigned:David_Rothstein» Unassigned
Status:Reviewed & tested by the community» Needs work

Yeah, I would say that although people aren't that likely to do hook_form_alter() or custom theming on update.php, we still shouldn't change the data structures and markup if the only reason is code cleanup (i.e., not fixing an actual bug).

However, this part of the patch:

function update_helpful_links() {
-  // NOTE: we can't use l() here because the URL would point to
-  // 'update.php?q=admin'.
-  $links[] = '<a href="' . base_path() . '">Front page</a>';

seems like it might be OK to backport (at the very least getting rid of the inaccurate code comment is obviously OK). I've run across that code comment before and had the sneaking suspicion it was all a complete lie; I'm glad someone discovered that it actually was :)

Even switching the actual base_path() stuff out with a call to l() instead might be fine. Although, calling l() has the effect of initializing the theme system, and it's scary to change the time/place we do that when something as fragile as update.php is running... so yeah, maybe better safe than sorry here.

Component:update.module» database update system

Status:Needs work» Needs review
StatusFileSize
new573 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,097 pass(es).
[ View ]

Following David_Rothstein's recommendation in #16, this patch removes the inaccurate comment with no other changes.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Hm. Not sure.. I still think that hunk could use a comment (though an accurate one ;)) as to why we're not using l() here, since someone's likely to try and "fix" this later.

My understanding is that there is no real reason (which is why the Drupal 8 patch changed it)...

"We are afraid to initialize the theme system at this point in update.php in a stable release" but not sure fear belongs in code comments :)

Status:Reviewed & tested by the community» Fixed

Alright, no one has moved it back from RTBC and no one else has commented either, so committing it to 7.x for now... thanks! http://drupalcode.org/project/drupal.git/commit/c6a5487

Feel free to reopen if you have ideas for a better code comment to add there, though.

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