This is best practice for all Drupal modules.

The two hooks provided by Content Access are:

  1. hook_user_acl($settings)
  2. hook_per_node($settings, $node)

As noted in #2581121-4: $node not passed as argument in "per_node" hook, these really should be renamed, because they are very generic. It has been standard practice since Drupal 7 to name hooks using the module's machine name as a prefix, that way namespace conflicts are unlikely (Hooks are in a flat namespace.) Real example: early in D7 two major modules both defined hook_date(), and many sites started to have errors. Once the hooks were renamed to conform to hook_<modulename>_date(), the problems went away.

I suggest the following:

  1. hook_content_access_user_acl($settings)
  2. hook_content_access_per_node($settings, $node)

These names are not only more descriptive but will protect us from namespace conflicts. For backwards compatibility, we can keep the old hooks but @deprecate them for removal in Content Access 8.x-2.x.

CommentFileSizeAuthor
#7 3226834-7.patch974 bytessourabhjain
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

TR created an issue. See original summary.

sourabhjain’s picture

Assigned: Unassigned » sourabhjain

.

sourabhjain’s picture

Assigned: sourabhjain » Unassigned
gisle’s picture

Assigned: Unassigned » gisle

Sounds sensible.

gisle’s picture

Assigned: gisle » Unassigned
Status: Active » Postponed (maintainer needs more info)

Need some more information. Sounded trivial to rename these, but I am not even able to locate them.

tr’s picture

Status: Postponed (maintainer needs more info) » Active

A module defines a hook by (hopefully) documenting that hook then using the hook - i.e. calling ModuleHandler::invokeAll() and collecting the results. So search the codebase for invokeAll and you will find the two hooks.

sourabhjain’s picture

Status: Active » Needs review
StatusFileSize
new974 bytes

Updated the hooks name. Please review.

tr’s picture

Status: Needs review » Needs work

That's a good start but the old hooks need to be properly deprecated. See https://www.drupal.org/about/core/policies/core-change-policies/drupal-c...

gisle’s picture

gisle’s picture

Version: 8.x-1.x-dev » 2.0.x-dev
Status: Needs work » Postponed

All feature requests go into the most recent branch.
Postponing until #3226837: Document this module's hooks in content_access.api.php is fixed.

steven jones’s picture

Version: 2.0.x-dev » 2.1.x-dev
Assigned: Unassigned » steven jones
Status: Postponed » Needs work

We can do this now.

steven jones changed the visibility of the branch 3226834-rename-hooks-to to hidden.

steven jones changed the visibility of the branch 3226834-rename-hooks-to to active.

steven jones’s picture

Status: Needs work » Needs review

Re-worked the code in a proper MR, and created the change record: https://www.drupal.org/node/3586991

Will merge and publish shortly.

steven jones’s picture

Status: Needs review » Fixed

Added to the merge train then.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • steven jones committed 7e4ea357 on 2.1.x
    refactor: #3226834 Rename hooks to include 'content_access' prefix
    
    By:...