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

RobLoach’s picture

Issue tags:+Novice

It's an easy patch review.

Devin Carlson’s picture

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.

Devin Carlson’s picture

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.

Devin Carlson’s picture

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

dcam’s picture

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.

dcam’s picture

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

dcam’s picture

Status:Needs review» Reviewed & tested by the community
webchick’s picture

Status:Reviewed & tested by the community» Fixed

Nice clean-up!

Committed and pushed to 8.x. Thanks!

dcam’s picture

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

Rerolled for D7.

dcam’s picture

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

It helps to actually attach the patch.

Devin Carlson’s picture

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.

webchick’s picture

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.

David_Rothstein’s picture

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.

David_Rothstein’s picture

Component:update.module» database update system
dcam’s picture

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.

David_Rothstein’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me.

webchick’s picture

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.

David_Rothstein’s picture

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

David_Rothstein’s picture

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.