Helpful links use base_path() when they don't have to

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).

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.

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.

Nice clean-up!

Committed and pushed to 8.x. Thanks!

Rerolled for D7.

It helps to actually attach the patch.

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.

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

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.

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

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 :)

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.