API page: http://api.drupal.org/api/drupal/includes--form.inc/function/drupal_retr...

The function contain the following comment:

  // $menu_get_item() is not available at installation time.

$menu_get_item() is really the menu_get_item() function, but the comment seems referring to the function whose name is contained in the variable $menu_get_item.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
Issue tags: +Quick fix
FileSize
661 bytes
jhodgdon’s picture

The code right after that is checking for maintenance mode, not "installation time". So is the new comment still a bit misleading too?

apaderno’s picture

The constant is used both in install.php, as define('MAINTENANCE_MODE', 'install'), and in update.php, as define('MAINTENANCE_MODE', 'update').
drupal_retrieve_form() is called by drupal_build_form(), which is then called by install_run_task(); also update_selection_page() indirectly calls drupal_retrieve_form(). The difference is that in the latter case, Drupal is fully bootstrapped, while in the first case, Drupal is not full bootstrapped.

Finding out that is not that immediate; maybe the comment should be less "cryptic" about that point.

jhodgdon’s picture

Title: Documentation problem with drupal_retrieve_form() » drupal_retrieve_form() has wrong code comment
Priority: Normal » Minor

Well, the check is just defined('MAINT...') -- it isn't checking what the value is. So it seems like if we're fixing the comment, it should reflect what the code does?

Tor Arne Thune’s picture

Changing tags so webchick will get it displayed when searching for "needs backport to D7."

jhodgdon’s picture

#1: drupal_retrieve_form_doc.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Needs work

patch actually is needs work, see comments above.

valthebald’s picture

Issue tags: +Novice

Sounds like an good first issue for someone with good English

Kevin Morse’s picture

Status: Needs work » Needs review
FileSize
886 bytes

Figured I'd give this a try... Not sure if I fully understand what's going on but we shall see.

valthebald’s picture

Status: Needs review » Needs work

#9: Looks good, but can you please remove extra spaces in the emd of comment lines?

Kevin Morse’s picture

Status: Needs work » Needs review
FileSize
884 bytes

Try this...

valthebald’s picture

Looks fine for me, let's see if that sounds clear enough.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Thanks! This looks fine to me to. I'll get it committed soon, unless someone else beats me to it.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks again! Committed to 8.x. The patch does not apply to 7.x and needs a reroll.

bdgreen’s picture

Status: Patch (to be ported) » Needs review
FileSize
863 bytes

Patch rerolled for 7.x

Status: Needs review » Needs work
Issue tags: -Needs backport to D6, -Quick fix, -Novice, -Needs backport to D7

The last submitted patch, 1204784-includes-form-15.patch, failed testing.

bdgreen’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Quick fix, +Novice, +Needs backport to D7

#15: 1204784-includes-form-15.patch queued for re-testing.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

Testbot is happy, and change matches D8 version

bdgreen’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @valthebald ;)

Similar comments do not appear near function drupal_retrieve_form in D6 form.inc so haven't rolled a patch for D6. OK?

valthebald’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D6, -Needs backport to D7

Right.
Removind 'Needs backport...' tags (as far as we're already at 7.x)

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Agreed, the patch is not needed for Drupal 6.x. I'll get the 7.x patch committed soon. Thanks!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x.

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