Function update_manager_access() in update.module refers to update_menu() which no longer exists.

Further, the description of this function describes it as an Access callback which was true when it was invoked from hook_menu() but has been replaced by src/Access/UpdateManagerAccessCheck.php. Though not used as an access callback anymore, it is invoked directly in many places as an access helper function.

Also, update_manager_access() clashes with hook_ENTITY_TYPE_access() and therefore should be renamed to _update_manager_access() and usages should be internalised to the update module.

CommentFileSizeAuthor
#9 drupal-cleanup_update-2551267-3.patch9.33 KBjeremy
#5 drupal-cleanup_update_documentation-2551267-3.patch894 bytesmandar.harkare
#2 drupal-cleanup_update_documentation-2551267.patch894 bytesjeremy

Comments

Jeremy created an issue. See original summary.

jeremy’s picture

jeremy commented
Status: Active » Needs review
StatusFileSize
new drupal-cleanup_update_documentation-2551267.patch894 bytes

Attached patch removes reference to update_menu and better describes function as an access helper.

albertski’s picture

albertski commented
Status: Needs review » Reviewed & tested by the community

Change looks good and makes sense. Ran the patch without any issues.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: drupal-cleanup_update_documentation-2551267.patch, failed testing.

mandar.harkare’s picture

mandar.harkare commented
Status: Needs work » Needs review
StatusFileSize
new drupal-cleanup_update_documentation-2551267-3.patch894 bytes

Adding the same patch again.

albertski’s picture

albertski commented
Status: Needs review » Reviewed & tested by the community

Re-ran patch again. Still looks good!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: -Quickfix

Can we also rename this method to _update_manager_access. update_manager_access is in danger of clashing with hook_ENTITY_TYPE_access if a module creates an entity type called manager. Also we should move some of the help text from system.module to the update module. system_help should only display the message when update module is not enabled. The update module should display the message when it is.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Access helper: can be removed too.

jeremy’s picture

jeremy commented
Status: Needs work » Needs review
StatusFileSize
new drupal-cleanup_update-2551267-3.patch9.33 KB

Updated patch to address all feedback:

  • Renames update_manager_access to _update_manager_access
  • Cleans up comments, including removing a reference to the no-longer existing update_menu()
  • Move help text displayed when update module is enabled from system.module to update.module
albertski’s picture

albertski commented
Status: Needs review » Needs work

Ran the patch without any issues.
Manually reviewed patch and everything looks good.

I grepped "update_manager_access" just to make sure no other module calls it and it looks like the migrate_drupal module calls it. I think it should be updated there too.

jeremy’s picture

jeremy commented
Status: Needs work » Needs review

What you're seeing there is a Drupal 7 database dump -- it's not affected by this change.

albertski’s picture

albertski commented
Status: Needs review » Reviewed & tested by the community

Ahh got it! Okay so this patch is good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Remove incorrect documentation in update.module » Rename update_manager_access() to _ update_manager_access()
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Rename update_manager_access() to _ update_manager_access() » Rename update_manager_access() to _update_manager_access()
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Looks good. There is one module I'm aware of that calls this method http://cgit.drupalcode.org/upgrade_status/tree/upgrade_status.module#n46, so wrote a brief change notice for it.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 6dd0d5a on 8.0.x 
    Issue #2551267 by Jeremy, mandar.harkare, alexpott, albertski: Rename...

Status: Fixed » Closed (fixed)

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