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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeremy created an issue. See original summary.

Jeremy’s picture

Status: Active » Needs review
FileSize
894 bytes

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

albertski’s picture

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

Status: Needs work » Needs review
FileSize
894 bytes

Adding the same patch again.

albertski’s picture

Status: Needs review » Reviewed & tested by the community

Re-ran patch again. Still looks good!

alexpott’s picture

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

Access helper: can be removed too.

Jeremy’s picture

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

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

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

Status: Needs review » Reviewed & tested by the community

Ahh got it! Okay so this patch is good.

alexpott’s picture

Title: Remove incorrect documentation in update.module » Rename update_manager_access() to _ update_manager_access()
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Title: Rename update_manager_access() to _ update_manager_access() » Rename update_manager_access() to _update_manager_access()
webchick’s picture

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.