Documentation refers to _theme() function, which has been removed

Current state is

$ git grep -E '[^a-z]_theme\('
core/includes/common.inc:469: * on normal pages is up through the preprocess step of _theme('html'). Adding
core/includes/pager.inc:39: * Initializes a pager for _theme('pager').
core/includes/pager.inc:42: * to _theme('pager') will render a pager that correctly corresponds to the
core/includes/theme.inc:689: *     calling _theme('image') to pass a meaningful value for this variable.
core/includes/theme.inc:1750:      // _theme('image') to pass explicit NULL for it to be omitted. Usually,
core/includes/theme.inc:1753:      // _theme('image') to pass a meaningful value for the alt variable.
core/lib/Drupal/Core/Render/theme.api.php:41: * _theme() function, which operates on the concept of "theme hooks". Theme
core/lib/Drupal/Core/Render/theme.api.php:462: * For more detailed information, see _theme().
core/lib/Drupal/Core/Render/theme.api.php:510: * For more detailed information, see _theme().
core/lib/Drupal/Core/Theme/Registry.php:50:   *      during _theme(), available as
core/lib/Drupal/Core/Theme/Registry.php:55:   *     hook is executed by _theme().
core/lib/Drupal/Core/Theme/Registry.php:389:   *   - 'preprocess functions': See _theme() for detailed documentation.
core/lib/Drupal/Core/Theme/Registry.php:515:            // hooks implemented as templates. See _theme().
core/lib/Drupal/Core/Theme/Registry.php:551:          // implemented as templates. See _theme().
core/lib/Drupal/Core/Theme/ThemeManager.php:151:      throw new \Exception(t('_theme() may not be called until all modules are loaded.'));
core/lib/Drupal/Core/Theme/ThemeManager.php:189:        // the function calling _theme() can differentiate between a hook that
core/lib/Drupal/Core/Theme/ThemeManager.php:242:    // If _theme() was invoked with a direct theme suggestion like
core/lib/Drupal/Core/Theme/ThemeManager.php:259:    // use it instead of the hook that _theme() was called with. For example, a
core/lib/Drupal/Core/Theme/ThemeManager.php:260:    // function may call _theme('node', ...), but a module can add
core/lib/Drupal/Core/Utility/ThemeRegistry.php:29:   * This is only allowed if all modules and the request method is GET. _theme()
core/modules/image/image.module:100:      // _theme('image_style') to pass explicit NULL for it to be omitted.
core/modules/image/image.module:103:      // _theme('image_style') to pass a meaningful value for the alt variable.
core/modules/image/image.module:264: *     _theme('image_style') to pass a meaningful value for this variable.
core/modules/simpletest/src/KernelTestBase.php:510:    // Ensure isLoaded() is TRUE in order to make _theme() work.
core/modules/simpletest/src/KernelTestBase.php:543:    // Ensure isLoaded() is TRUE in order to make _theme() work.
core/modules/simpletest/src/Tests/KernelTestBaseTest.php:284:   * Tests that _theme() works right after loading a module.
core/modules/simpletest/src/Tests/KernelTestBaseTest.php:293:    // _theme() throws an exception if modules are not loaded yet.
core/modules/system/src/Tests/Theme/ThemeTest.php:42:   *   - $variables['attributes'] as passed in to _theme()
core/modules/system/src/Tests/Theme/ThemeTest.php:59:   * Test that _theme() returns expected data types.
core/modules/system/tests/modules/theme_test/theme_test.module:79: * Theme function for testing _theme('theme_test_foo').
core/modules/system/tests/modules/theme_test/theme_test.module:86: * Theme function for testing _theme('theme_test_function_template_override').
core/modules/system/tests/modules/theme_test/theme_test.module:110: * _theme() on the top-level element to prevent infinite recursion.
core/modules/update/src/Form/UpdateManagerUpdate.php:201:        // rendered by _theme('table'), not _theme('tableselect'). Since the data
core/modules/update/src/Form/UpdateManagerUpdate.php:203:        // _theme('table').
core/themes/engines/twig/twig.engine:39: * If the Twig debug setting is enabled, HTML comments including _theme() call

Thanks for that! We definitely need to fix these, especially the ones that say "For more details, see _theme()" and things like that.

The ones that say things like ... for testing _theme('foo')... we could probably change to saying ... for testing the 'foo' theme hook...

Good change to make during RC, moving to task though.

Either way. It seems to me that it's a bug that we have docs referring to a function that doesn't exist, but it's definitely a task to fix them.

If there's any useful distinction between bug and task, it's that 'bug' describes code executing incorrectly in some way.

Tasks can definitely be for things that are wrong, as well as things that could be better.

Are you saying that no issue in the documentation component should be a bug? Incorrect documentation has always been a bug in the past. https://www.drupal.org/node/314328 just says "Bug is for errors".

I think we need to distinguish more between 'really bad code' and 'really bad user-facing error'. Right now we have 460 issues in the major bug queue against 8.x, and it can be hard to pick out the should-have-been-critical like #2172843: Remove ability to update entity bundle machine names from that long list.

Things like removing usage of deprecated functions are also tasks - but in terms of overall project health they're major because they do actually affect developer experience in a big way.

Ah sorry, I didn't notice this was marked Major. I would think it's an ordinary Normal documentation bug, and actually not any higher priority than other docs bugs to fix, really.

Here's my take. May not be all correct conversions but I think they are. Feel free to correct me. I tried using other examples from core depending on the context it was being used.

This looks mostly great! I have a few mostly minor suggestions:

1. +++ b/core/includes/pager.inc
   @@ -35,11 +35,11 @@ function pager_find_page($element = 0) {
   + * Initializes a pager for pager.html.twig templates.
   

   So in D8 we would actually tend to use #type => 'pager' not #theme => 'pager' to get a pager into a render array. So I'm wondering if rather than referring to pager.html.twig, we should refer to the pager element class instead?

2. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -489,7 +489,7 @@ function hook_form_system_theme_settings_alter(&$form, \Drupal\Core\Form\FormSta
   - * For more detailed information, see _theme().
   + * For more detailed information, see the 'theme.manager' service.
   

   So this is telling people where to look for more documentation.

   I doubt that there is documentation in the services.yml file for the 'theme.manager' service... The documentation that used to be in theme() in D7 that is being referred to here is the list of what all the processing and preprocessing functions are for a given theme hook.

   This is now in
   https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!theme.api....

   which you can reference by saying:

   @link themeable Theme system overview topic @endlink

3. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -537,7 +537,7 @@ function hook_preprocess(&$variables, $hook) {
   - * For more detailed information, see _theme().
   + * For more detailed information, see the 'theme.manager' service.
   

   Same here.

4. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -47,12 +47,12 @@ class Registry implements DestructableInterface {
   +   *      to the respective theme.For templates, it should point to the
   

   Typo: needs space after the .

5. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -389,7 +389,8 @@ protected function build() {
   +   *   - 'preprocess functions': See the 'theme.manager' service for detailed
   +   *     documentation.
   

   See comment above about the theme.manager service.

6. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -530,7 +531,7 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
   +            // hooks implemented as templates. See the 'theme.manager' service.
   

   see above, although in a // comment we probably don't want @link so you could probably say something like

   See the @defgroup themeable topic

   or something like that.

7. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -566,7 +567,7 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
   -          // implemented as templates. See _theme().
   +          // implemented as templates. See the 'theme.manager' service.
   

   see above

8. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -138,11 +138,11 @@ public function render($hook, array $variables) {
   -      throw new \Exception(t('_theme() may not be called until all modules are loaded.'));
   +      throw new \Exception('The theme implementations may not be rendered until all modules are loaded.');
   

   Um. Doesn't this message still need to be wrapped in t()?

9. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -26,9 +26,9 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
   +   * This is only allowed if all modules and the request method is GET.
   

   This line existed before this patch, but it doesn't make any sense?

   "if all modules" what?

10. +++ b/core/modules/image/image.module
    @@ -96,16 +96,15 @@ function image_theme() {
    -      // - http://dev.w3.org/html5/spec/Overview.html#alt
    -      // The title attribute is optional in all cases, so it is omitted by
    -      // default.
    +      // - http://dev.w3.org/html5/spec/Overview.html#alt The title attribute is
    +      //   optional in all cases, so it is omitted by default.
    

    I don't think the sentence starting with "The title attribute" is actually part of this bullet?

@jhodgdon thank you very much for the review. I wasn't sure about some of those. I'll work on this with someone at badcamp in a couple of days.

Regarding #12.8 I removed the t() from the Exception because I was under the impression we shouldn't be putting t() in to Exceptions. I just confirmed with @alexpott on IRC because we were discussing this a bit in Barcelona.

Oh really? They are shown to the user, in many cases... we're not translating the exception messages? Has that been written down somewhere?

Oh yeah it is written over here: https://www.drupal.org/node/608166
And in progress here: https://www.drupal.org/node/2055851

As a docs bugfix, this one doesn't need any special committer signoff, so moving to the new "rc eligible" tag. (Not that it makes much difference in practice, but I think it helps contributors to understand which to use for what.)

It's not quite all API documentation comments (there is one Exception message included), so I'm not sure "rc eligible" is right.

@jhodgdon I can leave that Exception exception _theme() reference out of this if you feel uncomfortable committing that. Just move it to a follow-up or the progress issue I mentioned in #15

Edit: it really is exceptional;)

Yes, we should probably move that to a separate issue, and then this would be all documentation and all rc eligible for sure.

I tried to addres most of the points mentioned by @jhodgdon at #12 except 1st point
@joelpittet : Hope you don't take it as an offense, since you said you will follow on your work in next badcamp.

Comments :
#12.1.: I have no idea

#12.9.: At least this truncated comment comes from theme.inc from d7 !
This as been introduced there https://www.drupal.org/node/1011614#comment-4914268 and committed on 29/10/2011 !
Have no idea what it means, I suggest "This is only allowed if the request method is GET"

#17 : I removed the Exception message part to be "rc eligible"

For the pager part, there is also #2530296: Fix up docs in core/includes/pager.inc looking into that. Can we leave the changes to there, or take the piece below (which I understand @jhodgdon agrees upon) from there?

+++ b/core/includes/pager.inc
@@ -36,11 +36,11 @@ function pager_find_page($element = 0) {
 /**
- * Initializes a pager for _theme('pager').
+ * Initializes a pager.
  *
- * This function sets up the necessary global variables so that future calls
- * to _theme('pager') will render a pager that correctly corresponds to the
- * items being displayed.
+ * This function sets up the necessary global variables so that the render
+ * system will correctly process #type 'pager' render arrays to output pagers
+ * that correspond to the items being displayed.
  *

I think we can leave the exception message change in - probably we should make exception messages rc eligible too.

Looking pretty good! Thanks!

So, a bit more to do on this one:

1. RE pager, yes let's just work on it in one issue not two. So let's just take it out of this patch. If someone wants to go work on that other issue (which may be stalled), that would be a bonus.
2. And as per #22, add back in the Exception message.

3. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -489,7 +489,8 @@ function hook_form_system_theme_settings_alter(&$form, \Drupal\Core\Form\FormSta
   + * For more detailed information, see @link themeable Theme system overview
   + * topic @endlink.
   

   @link ... @endlink all needs to stay on the same line. This line can go over 80 characters if necessary, but if that is happening, move @link to the start of the next line so the @link ... @endlink is the only thing on the line.

4. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -537,7 +538,8 @@ function hook_preprocess(&$variables, $hook) {
   + * For more detailed information, see @link themeable Theme system overview
   + * topic @endlink.
     *
   

   same here

5. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -389,7 +389,8 @@ protected function build() {
   +   *   - 'preprocess functions': See the 'theme.manager' service for detailed
   +   *     documentation.
   

   Um. I think the detailed docs on preprocess functions are actually on the "themeable" topic, right? They most certainly are not on the "theme.manager" service.

   So this would need to say:

   See the @link themeable [link text] @endlink topic for detailed documentation.

6. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -180,9 +180,10 @@ public function render($hook, array $variables) {
   +        // the function calling
   +        // \Drupal\Core\Theme\ThemeManagerInterface::render() can differentiate
   +        // between a hook that exists and renders an empty string and a hook
   +        // that is not implemented.
   

   I think maybe a comma before the last "and" here would make this sentence easier to parse?

7. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -233,7 +234,7 @@ public function render($hook, array $variables) {
   +    // If theme implementation was invoked with a direct theme suggestion like
   

   needs "the" before "theme implementation"

8. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -250,10 +251,10 @@ public function render($hook, array $variables) {
   +    // use it instead of the base hook. For example, a
   +    // function may use '#theme' => 'node', but a module can add 'node__article'
   +    // as a suggestion via hook_theme_suggestions_HOOK_alter(), enabling a theme
   +    // to have an alternate template file for article nodes.
   

   Can this comment be rewrapped to nearer 80 character lines please?

9. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -26,9 +26,9 @@ class ThemeRegistry extends CacheCollector implements DestructableInterface {
       * Whether the partial registry can be persisted to the cache.
       *
   -   * This is only allowed if all modules and the request method is GET. _theme()
   -   * should be very rarely called on POST requests and this avoids polluting
   -   * the runtime cache.
   +   * This is only allowed if the request method is GET.
   +   * \Drupal\Core\Theme\ThemeManagerInterface::render() should be very rarely
   +   * called on POST requests and this avoids polluting the runtime cache.
       */
      protected $persistable;
   

   So... I cannot see that $persistable is used or set anywhere in this class or in the one class that uses it, which is

   \Drupal\Core\Theme\Registry

   Probably this whole doc block and the variable it documents should just be removed. As far as I can see it is dead.

   Then we don't have to worry about what this cryptic previous documentation means ;)

10. +++ b/core/modules/image/image.module
    @@ -96,16 +96,15 @@ function image_theme() {
    +      // - http://dev.w3.org/html5/spec/Overview.html#alt ¶
    

    extra space end of line

11. +++ b/core/modules/image/image.module
    @@ -96,16 +96,15 @@ function image_theme() {
    +      // The title attribute is optional in all cases, so it is omitted by default.
    

    This line needs to be wrapped to 80 characters, as it was in the original before this patch. Really the original lines should just stay as they were.

12. +++ b/core/modules/system/tests/modules/theme_test/theme_test.module
    @@ -88,14 +88,14 @@ function template_preprocess_theme_test_function_suggestions(&$variables) {
    + * Theme function for testing ThemeManager::render('theme_test_function_template_override').
    

    This line is too long...

    but we don't want to wrap it either since it is the first line docs for a function.

    How about:

    Test theme function for 'theme_test....'.
    (don't put in ... put in the actual name of this theme hook).

@pguillard no offense at all, thank you very much for helping this! I'm working on lots of issues and welcome any help I can get as most of us do. Please if you have time, continue fixing the notes from @jhodgdon and others.

Applied suggestions from @jhodgdon #23.
I guess this is it.

Getting closer... Still some things to address:

1. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -489,7 +489,8 @@ function hook_form_system_theme_settings_alter(&$form, \Drupal\Core\Form\FormSta
   + * For more detailed information, see @link themeable Theme system overview topic
   + * @endlink.
   

   @link ... @endlink all needs to be on the same line.

   So start the @link on the next line.

2. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -537,7 +538,8 @@ function hook_preprocess(&$variables, $hook) {
   + * For more detailed information, see @link themeable Theme system overview topic
   + * @endlink.
   

   Same here.

3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -389,7 +389,8 @@ protected function build() {
   +   *   - See the @link themeable [link text] @endlink topic for detailed
   

   Um. You took me a bit too literally. You need to actually provide some link text here. ;)

   The topic is called:
   Theme system overview

4. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -530,7 +531,7 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
   +            // hooks implemented as templates. See the @defgroup themeable topic.
   

   This line exceeds 80 characters. Rewrap.

5. +++ b/core/modules/system/tests/modules/theme_test/theme_test.module
   @@ -88,14 +88,14 @@ function template_preprocess_theme_test_function_suggestions(&$variables) {
   - * Theme function for testing _theme('theme_test_foo').
   + * Theme function for testing ThemeManager::render('theme_test_foo').
     */
    function theme_theme_test_foo($variables) {
   ...
   - * Theme function for testing _theme('theme_test_function_template_override').
   + * Theme function for testing theme_test_function_template_override.
     */
    function theme_theme_test_function_template_override($variables) {
   

   These were not changed in parallel ways, so they are kind of inconsistent.

   And I guess they are both a bit inaccurate...

   Let's change both of them to say:

   Theme function for hook [name of the theme hook].

   The theme hook names are:
   theme_test_foo
   theme_test_function_template_override

Unable to apply patch. Need reroll.
Here is the output of my git apply command.

error: patch failed: core/includes/pager.inc:35
error: core/includes/pager.inc: patch does not apply

The pager documentation has already been fixed in #2530296: Fix up docs in core/includes/pager.inc, please remove any change to pager.inc as per #23.1

Re-rolled the patch. Not worked on comment #26.

@sriharsha.uppuluri do not forget to set status to 'Needs review' after you upload a patch, so that it can get processed by the testbots and reviewers get to know that there is something to look at :)

@mondrake Yes, i know that but i didn't do the changes what mentioned in #26 that's why i haven't put it to needs review.

Setting back to Needs Work then, which was correct.

Had a lumbago... and finally applied suggestions from @jhodgdon at #26 !

Thanks for the patch! Sorry for delay in review -- I've been on vacation.

So this looks pretty good! A few minor things to fix:

1. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -511,7 +511,8 @@ function hook_form_system_theme_settings_alter(&$form, \Drupal\Core\Form\FormSta
   + * For more detailed information, see
   + * @link themeable Theme system overview topic @endlink.
   

   Probably could use a "the" at the end of the first line here.

2. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -559,7 +560,8 @@ function hook_preprocess(&$variables, $hook) {
   + * For more detailed information, see
   + * @link themeable Theme system overview topic @endlink.
   

   And same here.

3. +++ b/core/lib/Drupal/Core/Utility/ThemeRegistry.php
   @@ -24,15 +24,6 @@
   -   * Whether the partial registry can be persisted to the cache.
   -   *
   -   * This is only allowed if all modules and the request method is GET. _theme()
   -   * should be very rarely called on POST requests and this avoids polluting
   -   * the runtime cache.
   -   */
   -  protected $persistable;
   

   Why are we removing this? I see in the __construct() for this class, it sets this variable, and it is used in the resolveCacheMiss method. Please instead fix the _theme() mention here.

Done with changes. Please review.

Looks like the feedback was addressed from #37 thank you @snehi

Agreed with RTBC. This is changing one line of non-docs code so moving into base component and leaving for someone else to commit, just in case.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!