Clean up stale references to theme('foo') in documentation

Tagging for rocketship purposes. Thanks @xjm!

Ran /\*.+theme\(\).+\*/ and \btheme\(\) on code base - no occurrences found.

I did a slightly less restrictive grep for _theme(), and found some things that probably should be cleared up -- used:

grep -R "theme(" * | grep -v hook_theme | grep -v js | more

I am not sure what your regexp was using, but it didn't find these references?

Here are some examples:

core/modules/user/user.api.php: * user.html.twig. See drupal_render() and _theme
() documentation
Several spots in theme.api.php @defgroup themeable section [see below]
core/modules/system/theme.api.php: * For more detailed information, see _theme() [occurs a couple of times]
core/modules/system/tests/modules/theme_test/theme_test.module: * _theme() on th
e top-level element to prevent infinite recursion.
core/modules/system/system.api.php: * - They can return HTML for default calls t
o _theme().
core/modules/system/system.api.php: * - They can return HTML for calls to _theme
() for a theme suggestion.

etc.

So... In the places where it says to see the _theme() documentation, we should put in a link to the "themeable" defgroup that exists in theme.api.php -- the docs that used to be on theme() that told how theme preprocessing etc. was done have been moved there. This can be done with one of the following:

@link themeable Default theme implementations topic @link
@see themeable

depending on whether the link needs to be in-text (use @link) or in a separate See Also section at the end (use @see and put it at the end of the doc block).

In theme.api.php in the "themeable" defgroup, it looks like someone did a search-and-replace for theme() to _theme(), and moved the docs over from what used to be the theme() function, but it doesn't explain what _theme() really is in the "Suggesting Alternate Hooks" section. It just starts talking about what _theme() will do. Probably these references should be either prefaced with an explanation that all theming goes through the internal _theme() function, or rather than mentioning _theme() itself, we should use "the theme system" or something like that.

Anyway, I didn't go through the whole grep output, but there are definitely some things to be done.

Thanks, jhodgdon. Ran another regex: \b_theme\(\), found multiple references. Patch attached. Please review and comment if you find instances that I should include/correct.

Thanks!

A few notes:

a) common.inc -- In the docs header for drupal_render(), I think we actually want to leave all that explanation in there about the internal calls to _theme(). It's an explanation of how drupal_render() works internally, and one of the few appropriate places to refer to _theme(), since that is really the only place it should be called.

b) In the in-code comment in drupal_render(), I think we actually do want to leave in the bit about _theme() returning FALSE, for much the same reason as (a) -- it's an in-code comment explaining what the next line means.

c) theme.inc

  * preparing variables for a template. See _theme() for more details about the
  * full sequence.
  *
- * @see _theme()
+ * @see themeable
  */
 function template_preprocess(&$variables, $hook, $info) {

This is good, but just above there is a reference to _theme() that should just go away. Probably the whole sentence can be removed.

d) block.api.php

- * for details.
+ * for details. @see themeable

You cannot put @see in the middle of text. Your choices are:
1. Put @see lines at the end of the doc block. On api.drupal.org, those will turn into a "See Also" section on the page.
2. Use @link to put a link in the text. See comment #5 for an example of how to do this.

This applies to most of the rest of the patch. I'll wait until that is cleared up before I review. Thanks!

e) When you upload a patch on an issue, set the status to "Needs review" to alert people (and test bots) that there is a patch to review.

Thank you. I have applied the changes - please review the attached patch.

Thanks @alexrayu! Just want to note that interdiffs are very helpful for reviewers when you are making changes to an existing patch :)

I actually revisited the places I found form the last time, and revised each separately. Let's treat it as a new patch, if ye please.

The interdiff.

This looks mostly good, thanks!

A few little things to clear up:

a)

+ * block.html.twig. See drupal_render() documentation respectively
+ * for details. @link themeable Default theme implementations topic @link

This doesn't read well... I think it should say "See the drupal_render() documentation and the @link ... @endlink for more details." This applies elsewhere in the patch as well, to similar text. Keep in mind that @link... @endlink is going to display on Drupal.org as a link, with everything except the initial "themeable" as the link text. So if you use @link, it should be embedded in a sentence and make sense in context.

b) You need to do @link ... @endlink, not @link ... @link ... Oh gracious, I messed up in comment #5. Sorry about that! Documentation on how to do @link *correctly* (again, sorry!) is at:
https://drupal.org/node/1354#link

c) @see lines always need to go at the very end of the documentation block. See
https://drupal.org/node/1354#order

d) The hook_theme() changes need more thought. You've taken out some information that I think we still need... I'm unfortunately really tired right now and do not have time to provide more concrete suggestions, but it definitely needs some more thought.

Thanks jhodgdon. I have corrected the issues you indicated, and have also reviewed the hook_theme() docs for some things that got deleted that could still be needed. Attaching patch and interdiff. Please see if anything else needs to be restored in hook_theme docs. My logic has been to remove all references to calling hook_theme for theming. Maybe some of the items I removed are actually still needed for internal use?

For your convenience, removed items were:
the @return array:
- variables: Used for _theme() call items only...
- function: If specified, this will be the function name to invoke for this implementation...
- base hook: Used for _theme() suggestions only: the base theme hook name...
- pattern: A regular expression pattern to be used to allow this theme implementation to have a dynamic name...

I will unassign for now, because I will be travelling in the next few days. Hopefully, my patch will be of use. If There are comments/corrections, and no one has taken it over, I will get back to it.

Ummm... I obviously didn't make things clear, let me try again.

In block.api.php in the latest patch, there is still this:

+ * block.html.twig. See drupal_render() documentation respectively
+ * for details. @link themeable Default theme implementations topic @link

You can't say "respectively" if there are not two choices -- this sentence doesn't make sense.

And on api.drupal.org, this will render as "See drupal_render documentation respectively for details. Default theme implementations topic". That isn't right either.

We want it to say something like "See drupal_render() documentation or the Default theme implementations topic for details", which you would accomplish by:

See drupal_render() documentation or the @link themeable Default theme implementations topic @endlink for details.

Notes:
- Embed @link...@endlink within a sentence/paragraph. It doesn't stand on its own.
- At the end of the link, you need @endlink, not @link

And then, I am seeing this in node.api.php:

+ * node.html.twig. See drupal_render() documentation respectively for details.
+ * @see themeable
  *
  * @param $build

Any use of @see needs to go at the very end of the documentation block, after all the @param, @return, etc. Not in the middle like this one is. And the "respectively" also doesn't make sense in this sentence.

Please try again with another patch. Thanks!

Note: I didn't read past block.api.php and node.api.php in this patch.

I see. I should probably leave it up to a native speaker to phrase then.

OK @alexrayu -- thanks for your help anyway! There are some easier issues in the "Documentation" component that are not as complex writing, if you would like to help out on a different issue.

Thank you. Sure, I will look through the issue queue and will help where I can. Just don't want to take your time to correct my english - that would be too abusive on my side, I think. I have written some docs as nodes at d.o., where my wording can be easily corrected. In a patch, it's a bit different story. ;)

This still needs to be fixed. I just ran into
#2262891: hook_theme 'pattern' docs are misleading
which has a small component of this problem in it.

I can take it up again, but I feel bad about taking your time, you having to review not only semantics, but my English as well.

Patch in #14 no longer applies. I'll take up this one.

Rerolled the patch in #14. Attaching the rerolled patch as well.

Tried to fix the patch according to the suggestions by @jhodgdon.

Attaching the patch and its interdiff with the rerolled patch. (Interdiff with the original patch didn't made sense because it longer applies.)

RE #21 - PLEASE do not feel bad about any English language reviews you might get!!!! We welcome any help with making patches, and since Drupal is an international community, some people need a little help with English after making a patch. That is not a problem. Please keep contributing! Actually, I end up correcting English spelling/grammar/punctuation in native English speakers' patches fairly often too. Really, don't fee bad!

A few notes on the new patch... and this time I have finally read the whole patch this time:

a) block.api.php:

+ * block.html.twig. See drupal_render() documentation or the @link themeable
+ * Default theme implementations topic @endlink for details.

Our standard for @link requires the whole @link ... @endlink to be on the same line. So move this down to the next line please. This also occurs in user.api.php.

b) Several places in the patch, there were notes that said essentially "...see _theme() for details...", and I believe most of them were referring to the big list of preprocesing functions that in 7.x is shown in the docs for the theme() function. These references, in the current patch were simply removed... One example:

- * See drupal_render() and _theme() for details.
+ * See drupal_render() for details.

So now, instead of telling people where to go to find out about the sequence of preprocessing, we're not providing them a reference at all. That doesn't seem great?

This documentation is now on
https://api.drupal.org/api/drupal/core!modules!system!theme.api.php/grou...
So I think in this case, we should say something like:
... see @link themeable the Default theme implementations topic for details @endlink ...
I think this link is useful to have in the text, even if there is an @see themeable in the same doc block, because it tells you *why* you might want to go to that link.

Note: It looks like taxonomy.api.php has the correct fix; other places in the patch don't.

c) the hook_theme() docs:

+++ b/core/modules/system/system.api.php
@@ -976,13 +976,8 @@ function hook_permission() {
 
 /**
  * Register a module or theme's theme implementations.
- *
- * The implementations declared by this hook have several purposes:
- * - They can specify how a particular render array is to be rendered as HTML.
- *   This is usually the case if the theme function is assigned to the render
- *   array's #theme property.
- * - They can return HTML for default calls to _theme().
- * - They can return HTML for calls to _theme() for a theme suggestion.
+ * The implementations declared by this hook specify how a particular render
+ * array is to be rendered as HTML.

There should still be a blank line between the first-line summary and the next paragraph here.

d) still in hook_theme() docs:

...
+ *   on the outer array are known as "theme hooks". For theme suggestions,
+ *   instead of the array key being the base theme hook, the key is a theme
+ *   suggestion name with the format 'base_hook_name__sub_hook_name'.

So the paragraph I noted in (c) took out mention of theme suggestions, and now in this paragraph, it's back... I guess we should put something into the paragraph in (c) about theme suggestions? Or else take it out here? Not sure which is appropriate. Actually, I don't think we implement hook_theme() to do anything for suggestions, but maybe it would be useful to look at Core and see if there are any hook_theme() implementations that include suggestions?

e)

+ *   The array values are themselves arrays containing information about the
+ *   theme hook and its implementation. Each information array must contain
+ *   a 'render element' element.

Huh? This is not right. Not all theme hooks have render elements. Some theme hooks are just for #theme in render arrays, not for render elements.

f) Related to (e), there are several sections in the hook_theme() docs that are removed that shouldn't be:
- pattern
- variables
etc.
These are still valid. The examples in them need to be changed so that instead of referring to calling _theme() they refer to using a #theme element in the render array, but there are still theme hooks that do not refer to render elements.

As far as I can tell, the only part of this that should be removed is 'function'. And I think the docs for 'template' refer to 'function', and shouldn't?

- *   - pattern: A regular expression pattern to be used to allow this theme
- *     implementation to have a dynamic name. The convention is to use __ to
- *     differentiate the dynamic portion of the theme. For example, to allow
- *     forums to be themed individually, the pattern might be: 'forum__'. Then,
- *     when the forum is themed, call:
- *     @code
- *     _theme(array('forum__' . $tid, 'forum'), $forum)
- *     @endcode

This part is removed. Why? Patterns are still valid. It's just that the example needs to use a #theme in a render array and not a call to _theme().

Fixed (a), (b), (d), (e), (f). Sorry, I completely missed (f) them in the previous patch because I just bluntly used the previous patch without actually verifying all the changes. Some sentences where '#theme' is used may be ill-formed, so keep an eye out for that.

I'll check what's appropriate regarding (c). Apart from (c), please let me know if this patch has any other issues.

So... This is looking pretty good! Sorry I didn't review this sooner... life/work have been busy lately...

A few issues still:

a) I think I may have said this in a previous review, but:

  * first in the sequence of preprocessing functions that are called when
- * preparing variables for a template. See _theme() for more details about the
- * full sequence.
+ * preparing variables for a template.
  *
- * @see _theme()
+ * @see themeable
  */
 function template_preprocess(&$variables, $hook, $info) {

The previous text said, in D7, "See theme() for more details about the full sequence". This text doesn't have that sentence, but only a @see themeable below -- which has that information, but the sentence telling you *why* you would want to read that topic has been removed.

So I think in these cases, we should replace this with:

See the @link themeable Default theme implementations topic @endlink for more details about the full sequence.

or something like that. Oh, I see that has been done in other spots in the patch, this one was just missed.

Here are other places this was missed:

+++ b/core/modules/system/entity.api.php
@@ -480,7 +480,7 @@ function hook_entity_view(array &$build, \Drupal\Core\Entity\EntityInterface $en
  * structured content array, it may use this hook to add a #post_render
  * callback. Alternatively, it could also implement hook_preprocess_HOOK() for
  * the particular entity type template, if there is one (e.g., node.html.twig).
- * See drupal_render() and _theme() for details.
+ * See drupal_render() for details.

b) In the hook_theme() docs, it ends up saying:

+ *   - variables: Only used for #theme in render array: an array of variables,
  *     where the array keys are the names of the variables, and the array
+ *     values are the default values if they are not passed to #theme in render
+ *     array. Template implementations receive each array key as a variable in

Um. You don't pass anything to #theme... I think maybe this should say "... the array keys are the names of the variables, and the array values are the default values if they are not given in the render array" ?

c) in hook_theme():

+ *     Function implementations are passed the variables in a single $variables
+ *     function argument.

But lower down, the patch removes the section about functions entirely? This is inconsistent. If functions are allowed, we need to keep the functions section; if they aren't, we need to remove this section too, and also we don't need to say things like "For template implementations..." any more. I think that changing the docs to remove the functions references is out of scope for this issue though, so we should just leave all the stuff about function implementations alone for this patch.

d) in hook_theme():

  *     suggestion, then this suggestion's function or template will be used to
- *     generate the output for _theme().
+ *     generate the output for #theme in render array.

This is also... weird. There is no output for #theme. What it does is generate the rendered output. Just say that.

e) in hook_theme():

  *     when the forum is themed, call:
  *     @code
- *     _theme(array('forum__' . $tid, 'forum'), $forum)
+ *     $render_array = array(
+ *       #theme => array('forum__' . $tid, 'forum'), $forum,
+ *     )
  *     @endcode

The code change here is good, but the line just above says "call" and "themed", when it should be talking about rendering and render arrays.

Removing 'Needs reroll' tag as patch applies cleanly. Retesting to be sure.

26: drupal-theme_cleanup-2191115-26.patch queued for re-testing.

Tried to fix the issues mentioned in #27. Sorry, I couldn't work on this earlier as I was caught up in some other work.

Interdiff attached here is of the new patch vs. the old patch so keep this mind while checking interdiff. I'm sorry for this but interdiff command was giving some errors (Error applying patch1 to reconstructed file) while doing the other way round.

This looks pretty good!

I only have two concerns now:

a)

 * @see _theme()
+ * See the @link themeable Default theme implementations topic @endlink for details.

These "See the ..." lines are regular documentation. They should not go in the @see section of the doc blocks, but up with the regular documentation (and probably in most cases in the paragraph that this is providing the "details" for). This needs to be fixed in several places in the patch. And there should always be a blank line between regular documentation and the @see section.

b) I would really like it if someone who knows a lot about how hook_theme(), _theme(), and drupal_render() work these days could look over the hook_theme() docs after this patch and make sure it is all correct though. I'm still not sure this patch has it all correct.

My main concern is this line:

+ *   For render elements, the key is the machine name of the render element.

This seems to imply that you can get a theme hook to be called by just having a theme hook in hook_theme whose name matches a render element type name... Is that true?

The drupal_render() docs disagree with this:

If this element has #theme defined and #theme is an implemented theme hook/suggestion then _theme() is called
...
If this element does not have a defined #theme, or the defined #theme hook is not implemented, or #render_children is set, then drupal_render() is called recursively on each of the child elements ... [_theme() is not called in this case]

So I think we should take this line out? It seems like if you want to specify theming for a render element, you have to define a theme hook, and then put #theme in the render array using that theme hook name?

My other concern in the hook_theme() docs is:

 *  .... For theme suggestions,
+ *   instead of the array key being the base theme hook, the key is a theme
+ *   suggestion name with the format 'base_hook_name__sub_hook_name'.

Is this accurate? Do we declare theme suggestions in hook_theme() as outer array keys like this? How is that actually used, as compared to the 'pattern' return value element in hook_theme()?

I understand (a) and I'll provide a patch for it later today but I'm not sure about (b). We can remove the line:

 For render elements, the key is the machine name of the render element

Though, I'm not sure about your other concern. It'll be better if someone having better of understanding of this can verify the this part.

About (b) -- I'm not sure either. We either need to dig into the code and figure this out, or get one of the Theme system maintainers to answer the question. If you're in Austin, maybe you can get someone there to help? If not, maybe you or I can look in MAINTAINERS.txt, figure out who to ask, and ask them in IRC?

Sorry, the recent updates to this issue passed me by (I'm one of the theme system maintainers). All I can say is Austin was a blur :). I've added the patch here to my list to review.

In the meantime I wanted to note that there may be a bit of overlap with #2388247: Documentation refers to _theme() function, which has been removed which I just created and this issue.

Agreed with #32.a. Regarding b, I'm fairly sure that the render element doesn't have to match with the key given in hook_theme(), since you can specify the render element manually. So I would vote to remove that line also.

Unassigning because it's been a while.

This needs a reroll or redo, if the reroll steps get you stuck in conflict land the patch is small enough to be redone manually as well.

1. +++ b/core/modules/block/block.api.php
   @@ -20,8 +20,8 @@
   + * block.html.twig. See drupal_render() documentation or the
   
   +++ b/core/modules/node/node.api.php
   @@ -817,8 +817,8 @@ function hook_node_view(array &$build, \Drupal\node\NodeInterface $node, \Drupal
   + * node.html.twig. See drupal_render() documentation or the
   

   Several spots in the patch: the documentation previously at drupal_render() is now at \Drupal\Core\Render\RendererInterface::render() but it's possible another issue has updated these references. Overall it looks like things have moved around quite a bit, for example hook_node_view() seems to be gone in favour of hook_entity_view().

2. +++ b/core/modules/system/system.api.php
   @@ -1013,19 +1009,18 @@ function hook_permission() {
   + *   on the outer array are known as "theme hooks". For theme suggestions,
   + *   instead of the array key being the base theme hook, the key is a theme
   + *   suggestion name with the format 'base_hook_name__sub_hook_name'.
   

   This is valid.

3. +++ b/core/modules/system/system.api.php
   @@ -1053,7 +1048,7 @@ function hook_permission() {
   - *   - base hook: Used for _theme() suggestions only: the base theme hook name.
   + *   - base hook: Used for #theme suggestions only: the base theme hook name.
   

   This should probably not have a "#", so: "Used for theme hook suggestions only" or something along those lines. We could consider referencing the 'suggestions' section of https://api.drupal.org/api/drupal/core%21modules%21system%21theme.api.ph....

4. +++ b/core/modules/system/system.api.php
   @@ -1063,14 +1058,16 @@ function hook_permission() {
   - *     _theme(array('forum__' . $tid, 'forum'), $forum)
   + *     $render_array = array(
   + *       #theme => array('forum__' . $tid, 'forum'), $forum,
   + *     )
   

   This is not a valid render array conversion, see https://www.drupal.org/node/2195739 for some documentation.

   First of all (I could go either way), we may want to use an existing theme hook, 'forum' doesn't exist. Either way…

   1. The variables need to be split out as separate array keys, something more like:

   $render_array = array(
     '#theme' => array('forum__' . $tid, 'forum'),
     '#forum' => $forum,
   );
   

   2. Missing quotes around '#theme'.

   3. Missing closing semicolon for the array.

As mentioned in #36, this had gone beyond a reroll.

I've attempted to recreate the original patch intent, added new locations where appropriate while also including comments from #36. Due to the structure and changes an interdiff isn't very valuable here, so I haven't attached one.

This certainly needs review.

Good... I think the decisions/content here is all good.

Just some formatting/standards stuff to fix. First off, check out:
https://www.drupal.org/node/1354#order
Basically, @see should be at the end, and lines like "See foo bar for details" should be before you get to the @param/@return/@see stuff in doc blocks.

And there should always be a blank line between any docs and the first @see.

So, a few things need to be rearranged.

And one other nitpick, in the hook_theme() docs:

+ *     $render_array = array(
+ *     '#theme' => array('forum__' . $tid, 'forum'),
+ *     '#forum' => $forum,
+ *     );

The two inner lines here should be indented 2 spaces, just like you would if they were regular code.

Thanks for the review!

Enclosed are the requested changes with interdiff. Regards.

Great! Only a few things to fix:

1. +++ b/core/includes/theme.inc
   @@ -1162,10 +1162,9 @@ function template_preprocess_maintenance_task_list(&$variables) {
   + * See the @link themeable Default theme implementations topic @endlink for details.
     */
    function template_preprocess(&$variables, $hook, $info) {
      // Tell all templates where they are located.
   

   This line is no longer an @see, so it needs to be moved up into the main body of the documentation, before the @param and @return sections. At least, I'm assuming there are @param and @return sections for this function?

   Also the added "See the ..." goes over 80 characters and needs rewrapping.

   One or both of these comments apply in several places in the patch, please check.

2. +++ b/core/modules/system/entity.api.php
   @@ -1312,7 +1312,9 @@ function hook_ENTITY_TYPE_view(array &$build, \Drupal\Core\Entity\EntityInterfac
   - * See drupal_render() and _theme() for details.
   + * See drupal_render() for details.
   + *
   + * See the @link themeable Default theme implementations topic @endlink for details.
     *
   

   Probably these two "See ... for details" sentences should be combined into one?

3. +++ b/core/modules/system/entity.api.php
   @@ -1348,7 +1350,9 @@ function hook_entity_view_alter(array &$build, Drupal\Core\Entity\EntityInterfac
   - * See drupal_render() and _theme() for details.
   + * See drupal_render() for details.
   + *
   + * See the @link themeable Default theme implementations topic @endlink for details.
     *
   

   Here too, one sentence would be better.

Thanks for the review jhogdon!

I have corrected all issues as requested - apologies for the 80 character breaking on the patch.

Re: #1 - there are no @param and @return sections for that function, as you suspected. If you like, I could flesh some out - or open a new issue to address it. I checked again and believe the order is fine in #43 in the other patch locations.

Regards.

Looks good to me now, thanks!

Back to RTBC as per #46, random testbot failure.

Committed a92f929 and pushed to 8.0.x. Thanks!