Problem/Motivation

The purpose of hook_library() and hook_library_alter() is not clear from their names.

Proposed resolution

Per sun: Rename these functions to follow existing patterns:

  • hook_library() should be renamed to hook_library_info().
  • hook_library_alter() should be renamed to hook_library_info_alter().

Remaining tasks

TBD

User interface changes

None.

API changes

  • hook_library() will be renamed to hook_library_info().
  • hook_library_alter() will be renamed to hook_library_info_alter().
CommentFileSizeAuthor
#9 rename-hook-library-1286868-5023772.patch8.18 KBrickmanelius
#6 rename-hook-library-1286868-5020994.patch7.71 KBrickmanelius
#3 rename-hook-library-1286868-5020458.patch5.24 KBrickmanelius

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Thanks! :)

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Rename hook_library() and hook_library_alter() to standard naming conventions » Rename hook_library() and hook_library_alter() to standard pattern
rickmanelius’s picture

rickmanelius commented
Status: Active » Needs review
StatusFileSize
new rename-hook-library-1286868-5020458.patch5.24 KB

patch attached using search replace for both functions

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

We need to actually change the name of the hook where it gets called as well (e.g. in drupal_get_library()). Look for module_invoke() and drupal_alter() with 'library' as an argument

rickmanelius’s picture

rickmanelius commented

xjm. Apologies for that oversite. Will fix and submit another patch.

rickmanelius’s picture

rickmanelius commented
Status: Needs work » Needs review
StatusFileSize
new rename-hook-library-1286868-5020994.patch7.71 KB

Try this one. I also realized I missed a few implemented hook_library hooks. I manually set them after the larger search/replace changes.

I only found the instances of module_invoke and drupal_alter instances mentioned above. I also checked invoke_all and didn't find anything. So unless there is something else, this should be set.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +API change, +API clean-up

#6 is RTBC after someone else than @rickmanelius applied the patch and verified/double-checked/grepped that all instances are covered indeed.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

Looks like we still missed a few references to module implementations. In common.test, there is a missed reference to common_test_library(), and in system.api.php, there is a missed reference to system_library(). (I just grepped for _library( with the patch applied and skipped lines with drupal_add_library() and drupal_get_library().)

Might want to go through all instances of library to be sure we have everything.

rickmanelius’s picture

rickmanelius commented
StatusFileSize
new rename-hook-library-1286868-5023772.patch8.18 KB

Thanks again xjm for your patience and your feedback. I did a search for both _library and then library to make sure. I found the ones you described and I think there were 1-2 others. Let's try this one more time.

rickmanelius’s picture

rickmanelius commented
Status: Needs work » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I read the patch closely and checked all instances of library with the patch applied. Assuming testbot comes back green, I think this is RTBC.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Can't believe I almost missed that one. :)
Cool stuff, patch looks awesome!

rickmanelius’s picture

rickmanelius commented

Awesome. Thanks for all the help/instructions, xjm.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Good clean-up. Committed to 8.x. Thanks.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Fixed » Needs work
Issue tags: +Needs change record

This patch is marked "fixed", and "api change", but there is no change node. Needs one I think?

rickmanelius’s picture

rickmanelius commented

Hi jhodgdon.
New contributor here. How would one do that? Can you provide a link/example? I can address if it's within my skill set.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

See
http://drupal.org/node/1203498
(there's a section on change notifications in the Docs section at the top) (and ask again if that didn't answer your question)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Rename hook_library() and hook_library_alter() to standard pattern » API change notice for Rename hook_library() and hook_library_alter() to standard pattern
Priority: Minor » Critical
Status: Needs work » Active
gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Active » Needs review

http://drupal.org/node/1294416

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: API change notice for Rename hook_library() and hook_library_alter() to standard pattern » Rename hook_library() and hook_library_alter() to standard pattern
Priority: Critical » Minor
Status: Needs review » Fixed

Looks good. Thanks!

Restoring previous status.

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Minor » Normal
Issue tags: -Novice, -Needs change record
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Updated issue summary.