On #2299715: [meta] Move core hooks from system.api.php to core.api.php or other files an inventory was done of hooks in core/modules/system/system.api.php that are not related to the System module per se.

The following hooks are related to the theme and rendering system, and should be moved to theme.api.php:

hook_js_alter(), hook_library_info_alter(), hook_library_alter(), hook_css_alter()
hook_element_info(), hook_element_info_alter()
hook_page_build(), hook_page_alter()
hook_theme(), hook_theme_registry_alter(), hook_template_preprocess_default_variables_alter()

Also, the following two hooks are duplicated in system.api.php and theme.api.php. They should be removed from system.api.php:
hook_themes_installed(), hook_themes_uninstalled()

CommentFileSizeAuthor
#30 2307859-30.patch38.43 KBrpayanm
#30 2307859-interdiff.txt736 bytesrpayanm
#24 2307859-24.patch38.17 KBrpayanm
#19 move_hooks_to_theme_api-2307859-18.patch41.05 KBshumer
#15 move_hooks_to_theme_api-2307859-14.patch40.79 KBshumer
#9 2307859-9.patch42.27 KBdankh
#5 2307859-5.patch42.76 KBdankh
#1 2307859.patch41.2 KBjhodgdon

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2307859.patch41.2 KB

Here's a patch. Note that theme.api.php did not have a @file doc block, nor were its hooks being added to the Hooks group/topic on api.drupal.org. So this has also been remedied.

So, for the hooks... I just used copy/paste, so the hook function bodies and documentation should be identical between the files.

To review:
- Did I remove the right hooks from system.api.php
- Did they all make it into theme.api.php
- Are the choices good (should these hooks be moved).

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I reviewed the patch and it seems quite reasonable to me. All 11 hooks were moved over. I think any issues with these hooks would probably end up in the 'theme system' component so theme.api.php seems like a good fit! Thanks @jhodgdon :)

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Although it does need a reroll… :)

dankh’s picture

dankh commented
Assigned: Unassigned » dankh

I'm re-rolling it.

dankh’s picture

dankh commented
Assigned: dankh » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2307859-5.patch42.76 KB

Patch re-rolled. There were couple of conflicts in the comments, I kept those from head. In theme.api.php I have also checked and updated the links in the overview docblock :

https://www.drupal.org/documentation/theme - generic entry page, link left as is
https://www.drupal.org/node/722174 - Theme API, entry page which needs to be updated, link left as is
https://drupal.org/node/933976 - Using the theme layer (Drupal 7.x), D7 only link removed
https://drupal.org/node/930760 - Render Arrays in Drupal 7, D7 only link removed

Added the link : https://www.drupal.org/theme-guide/8

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

dankh: Um. ... please, this issue is *just about moving the hooks*. We do not want those links removed, actually -- until we have new Drupal 8 documentation for those areas, we should continue to link to the Drupal 7 documentation, which is better than nothing. Also, fixing the links would be a separate issue.

So can you please make another patch that just moves the hooks, as they are now in head, from one file to the other? Or I can do it... but that is all we want in the patch.

Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +needs rerorll
dankh’s picture

dankh commented
Assigned: Unassigned » dankh

I'm re-rolling it.

dankh’s picture

dankh commented
Assigned: dankh » Unassigned
Status: Needs work » Needs review
Issue tags: -needs rerorll
StatusFileSize
new 2307859-9.patch42.27 KB

Patch re-rolled, no changes to links were made. Sorry about that I'm fairly new contributor.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Not a problem! Sorry if I came across as angry or annoyed in any way in my earlier review. Thanks for the new patch!

Hm, the patch came out a bit weird (silly diff util!). So... a bit hard to review. Can you confirm that you started from an updated D8 repository and just did cut-and-paste to make it?

dankh’s picture

dankh commented
Assigned: Unassigned » dankh

You're right the patch doesn't look good. I followed this guide https://www.drupal.org/patch/reroll .

But when I look again I see that :

+++ b/core/modules/system/system.api.php
@@ -119,157 +119,32 @@ function hook_queue_info_alter(&$queues) {
+function callback_queue_worker($queue_item_data) {

doesn't exist in the current codebase.

I'll do it again. Sorry about that.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I don't think a reroll is really what we need here. We probably just need a new patch that starts over with a clean "git pull" on Drupal 8, then opens system.api.php, and cut/pastes the desired hooks into theme.api.php.

star-szr’s picture

star-szr
he/him
Primary language English
commented

Agreed, a reroll is just going to make things harder in this case. Thanks for working on this @dankh!

jhodgdon’s picture

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

For now...

shumer’s picture

shumer commented
Status: Needs work » Needs review
Issue tags: +#amsterdam2014, +sprint Amsterdam2014
StatusFileSize
new move_hooks_to_theme_api-2307859-14.patch40.79 KB

Here is new patch for test

jhodgdon’s picture

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

Looks good to me, thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

See also #2349221: No @addtogroup hooks in theme.api.php

shumer’s picture

shumer commented
Issue tags: +Amsterdam2014
shumer’s picture

shumer commented
Issue tags: +Adyax
StatusFileSize
new move_hooks_to_theme_api-2307859-18.patch41.05 KB

Patch updated with @addtogroup hook

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

OK, still RTBC then. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll and what about hook_themes_installed and hook_themes_uninstalled - I think they are currently in both places :) - They should only be in theme.api.php (i think).

opdavies’s picture

opdavies
He/him
Primary language English
Location Wales
commented
Issue tags: -#amsterdam2014, -sprint Amsterdam2014

Removing duplicate tags.

jhodgdon’s picture

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

Hah, didn't notice that about hook_themes_installed() and hook_themes_uninstalled(). That would indeed be a good addition to this patch. I took a look at the documentation/body in system.api.php and theme.api.php, and I think the versions in theme.api.php are slightly better. So let's remove hook_themes_installed() and hook_themes_uninstalled() from system.api.php.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new 2307859-24.patch38.17 KB

rerolled and removed from system.api.php:
hook_themes_installed(), hook_themes_uninstalled()

I have a question hook_themes_installed() in system.api.php should not be equal to those of theme.api.php

Here the comparison:
system.api.php:

/**
 * Perform necessary actions when themes are installed.
 *
 * @param array $themes
 *   An array of theme names which are installed.
 */
function hook_themes_installed(array $themes) {
  // Add some state entries depending on the theme.
  foreach ($themes as $theme) {
    \Drupal::state()->set('example.' . $theme, 'some-value');
  }
}

theme.api.php:

/**
 * Respond to themes being installed.
 *
 * @param array $theme_list
 *   Array containing the names of the themes being installed.
 *
 * @see \Drupal\Core\Extension\ThemeHandler::install()
 */
function hook_themes_installed($theme_list) {
  foreach ($theme_list as $theme) {
    block_theme_initialize($theme);
  }
}
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Um... Not sure what you are asking in #24. In #23, I said " I took a look at the documentation/body in system.api.php and theme.api.php, and I think the versions in theme.api.php are slightly better. So let's remove hook_themes_installed() and hook_themes_uninstalled() from system.api.php." Does that answer your question?

Also... When you say you "rerolled" the patch, did you reroll or do a clean cut-and-paste? Because rerolling in the traditional way is **not** the right thing to do in this patch. You need to start from a clean git pull, and cut/paste the current hooks from one way to another, in order to preserve the current status of those hook docs.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented

Yes, that I repond to my question :)

And I start from a clean git pull and then cut-and-paste the current hooks from one way to another, the only thing I have left is @addtogroup hook, I do not where to put it :(

Greetings.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

If you look at some of the other *.api.php files with hooks in them, you may be able to see what to do in this file. The idea is that the @addtogroup hooks {} section starts before the first hook in the file, and ends after the last hook, so that all the hooks are added to the "hooks" topic.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented

Then everything should be surrounded by @addtogroup hooks {}? because all function are:

function hook_*

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Right, but just the hooks, not the @defgroup pieces. Thanks!

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
StatusFileSize
new 2307859-interdiff.txt736 bytes
new 2307859-30.patch38.43 KB

Now with @addtogroup hook :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Fixed

Thanks! I committed this to 8.0.x. On commit, fixed the missing newline in theme.api.php and moved a couple more hooks over.

  • jhodgdon committed 7bd652e on 8.0.x 
    Issue #2307859 by rpayanm, dankh, shumer: Move theme-render hooks from...

Status: Fixed » Closed (fixed)

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