This issue has novice tasks. If you are an experienced core developer and have multiple commit mentions, please review novices' work on these tasks rather than doing them yourself. Feedback from experienced contributors is valued.

Problem/Motivation

Preprocess function documentation standards do not state that @ingroup themeable should be included in preprocess function docblocks. That group should only be included for functions or templates that actually produce markup and can be overridden in a theme.

A number of Twig conversions got in that converted a theme_ function to a template_preprocess_ function and left in the @ingroup themeable from the theme function.

This is messing up the listing on https://api.drupal.org/api/drupal/core%21modules%21system%21theme.api.ph... which should only show theme functions and template files.

Proposed resolution

Remove @ingroup themeable from all preprocess function docblocks.

Remaining tasks

User interface changes

n/a

API changes

n/a

CommentFileSizeAuthor
#8 drupal.remove-ingroup-themeable-from-preprocess-function-docblocks.2219617-08.patch3.01 KBgalooph
#5 drupal.remove-ingroup-themeable-from-preprocess-function-docblocks.2219617-05.patch2.58 KBvisabhishek
#5 interdiff-2219617-02-05.txt448 bytesvisabhishek
#2 remove-ingroup-themeable-2219617-2.patch2.26 KBjoshtaylor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joshtaylor’s picture

joshtaylor CreditAttribution: joshtaylor commented
Assigned: Unassigned » joshtaylor

I will be writing a patch to fix this issue.

joshtaylor’s picture

joshtaylor CreditAttribution: joshtaylor commented
FileSize
remove-ingroup-themeable-2219617-2.patch2.26 KB

This patch removes the @ingroup themeable from the following functions:

  • template_preprocess_details
  • template_preprocess_input
  • template_preprocess_form_element
  • template_preprocess_admin_block_content
  • template_preprocess_admin_page
  • template_preprocess_system_themes_page

I couldn't find the @ingroup themeable in the following functions, as per the https://api.drupal.org/api/drupal/core%21modules%21system%21theme.api.ph... page, as I think they might have been removed already?:

  • template_preprocess_container
  • template_preprocess_datetime
  • template_preprocess_feed_icon
  • template_preprocess_item_list
  • template_preprocess_links
  • template_preprocess_status_messages
  • template_preprocess_status_report
joshtaylor’s picture

joshtaylor CreditAttribution: joshtaylor commented
Status: Active » Needs review
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work

Thanks @joshtaylor! I think the only one that was missed is template_preprocess_status_report().

Edit: Which I see is in the list of ones you couldn't find but is there I promise :)

visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
Status: Needs work » Needs review
FileSize
interdiff-2219617-02-05.txt448 bytes
drupal.remove-ingroup-themeable-from-preprocess-function-docblocks.2219617-05.patch2.58 KB

updated patch as per #4. Please review

galooph’s picture

galooph CreditAttribution: galooph commented

Patch from #5 looks good. Verified that @ingroup themeable was removed from:

  • template_preprocess_details()
  • template_preprocess_input()
  • template_preprocess_form_element()
  • template_preprocess_admin_block_content()
  • template_preprocess_admin_page()
  • template_preprocess_system_themes_page()
  • template_preprocess_status_report()

and that it was already missing from:

  • template_preprocess_container()
  • template_preprocess_datetime()
  • template_preprocess_feed_icon()
  • template_preprocess_item_list()
  • template_preprocess_links()
  • template_preprocess_status_messages()
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work

Sorry I missed this because I just did grep before, but we need to remove the line above the @ingroup themeable as well.

So instead of:

+++ b/core/includes/form.inc
@@ -1068,7 +1068,6 @@ function template_preprocess_fieldset(&$variables) {
  *     Properties used: #attributes, #children, #open,
  *     #description, #id, #title, #value, #optional.
  *
- * @ingroup themeable
  */
 function template_preprocess_details(&$variables) {
   $element = $variables['element'];

Should be more like:

+++ b/core/includes/form.inc
@@ -1068,7 +1068,6 @@ function template_preprocess_fieldset(&$variables) {
  *     Properties used: #attributes, #children, #open,
  *     #description, #id, #title, #value, #optional.
- *
- * @ingroup themeable
  */
 function template_preprocess_details(&$variables) {
   $element = $variables['element'];
galooph’s picture

galooph CreditAttribution: galooph commented
Status: Needs work » Needs review
FileSize
drupal.remove-ingroup-themeable-from-preprocess-function-docblocks.2219617-08.patch3.01 KB

Here's a revised patch as per the comments in #7.

Status: Needs review » Needs work

The last submitted patch, 8: drupal.remove-ingroup-themeable-from-preprocess-function-docblocks.2219617-08.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review

8: drupal.remove-ingroup-themeable-from-preprocess-function-docblocks.2219617-08.patch queued for re-testing.

star-szr’s picture

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

Thanks @galooph!

webchick’s picture

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

Nice work!

Committed and pushed to 8.x. Thanks!

  • Commit 627b7bb on 8.x by webchick: 
    Issue #2219617 by visabhishek, galooph, joshtaylor, Cottser: Remove @...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Oh, also? Thanks for making our docs clearer. :)

Status: Fixed » Closed (fixed)

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