Update stale references to theme functions that have been converted to Twig

My question is : all these functions will be replaced by a template, or some of them will simply disappear ?
My survey results :

Theme_ functions that still exist, no twig template yet
theme_menu_local_tasks()
theme_menu_local_task()
theme_menu_local_action()
theme_username()
theme_menu_tree()
theme_table()
theme_update_report()
theme_language_content_settings_table()
theme_more_link()
theme_system_compact_link()
theme_get_suggestions()
theme_get_setting()
theme_form_element_label()
theme_user_permission_description()
theme_input()
theme_task_list()
theme_mark()
theme_menu_overview_form()

Theme_ functions that still exist, a twig template is available
theme_field_multiple_value_form() REPLACE WITH field-multiple-value-form.html.twig
theme_field() REPLACE WITH field.html.twig

Functions have been removed, but no twig template yet
theme_book_tree()
theme_tablesort_indicator()
theme_mymodule_row()
theme_breadcrumb()
theme_menu_tree__shortcut_default()
theme_update_available_updates_form()
theme_datetime()
theme_simpletest_test_table()

Replacements
theme_custom_block_add_list() REPLACE WITH custom-block-add-list.html.twig
theme_admin_block_content() REPLACE WITH admin-block-content.html.twig
theme_form() REPLACE WITH form.html.twig
theme_image() REPLACE WITH image.html.twig
theme_item_list() REPLACE WITH item-list.html.twig
theme_form_element() REPLACE WITH form-element.html.twig
theme_status_messages() REPLACE WITH status-messages.html.twig
theme_maintenance_page() REPLACE WITH maintenance-page.html.twig
theme_install_page() REPLACE WITH install-page.html.twig

Working on this during Dev Days.

Here is my patch. Hope I didn't forgot lines.

In some cases I didn't make the change : for instance function theme_form_element_label summary lines contains a mention to theme_form_element() but the whole block will be removed one day anyway.
I didn't replaced the occurrences in core themes for that same reason.

Thanks!

I took a quick look at the patch... It may require a little more thought. For instance at the top:

 *   string, or an array of Ajax commands. If returning a renderable array or
  *   a string, the value will replace the original element named in
  *   #ajax['wrapper'], and
- *   theme_status_messages()
+ *   status-messages.html.twig
  *   will be prepended to that
  *   element. (If the status messages are not wanted, return an array
  *   of Ajax commands instead.)

I don't think it's accurate to say that status-messages.html.twig will be prepended to something -- probably it would be more sensible to say that the output of status-messages.html.twig would be prepended? Also, why all the newlines here?

Actually, I think this is the only place in your patch that needs more attention.

I haven't verified that all of the replacements in the patch go to existing Twig files... needs a careful review for that. Can you fix this one spot and then we'll get it reviewed? Thanks!

Did changes to the patch in #6 as suggested in #8.

*   returns a renderable array (most often a form or form fragment), an HTML
  *   string, or an array of Ajax commands. If returning a renderable array or
  *   a string, the value will replace the original element named in
- *   #ajax['wrapper'], and
- *   theme_status_messages()
- *   will be prepended to that
- *   element. (If the status messages are not wanted, return an array
- *   of Ajax commands instead.)
+ *   #ajax['wrapper'], and output of status-messages.html.twig  will be 
+ *   prepended to that element. (If the status messages are not wanted, return
+ *   an array of Ajax commands instead.)
  *   #ajax['wrapper']. If an array of Ajax commands is returned, it will be
  *   executed by the calling code.

the above would be this:

  *   returns a renderable array (most often a form or form fragment), an HTML
  *   string, or an array of Ajax commands. If returning a renderable array or
  *   a string, the value will replace the original element named in
  *   #ajax['wrapper'], and output of status-messages.html.twig  will be 
  *   prepended to that element. (If the status messages are not wanted, return
  *   an array of Ajax commands instead.)
  *   #ajax['wrapper']. If an array of Ajax commands is returned, it will be
  *   executed by the calling code.

This doesn't make any good sence in my ears.

The second sentence is a really long one.. perhaps a end of sentence after #ajax['wrapper']. Begin with 'And the output'. Or maybe even just 'The output..'

After that comes a parantes,first of all I'm not sure why it is in a parantes at all but second, should it really be as an own sentence, that's not usually how a parantes are used.

then it says #ajax['wrapper']. ehm ok what?

ok so these comments wasn't really a part of the issue but if my comments are valid why not clean this up at the same time.

wait for more reviews and see what they think

Ok, I realize now that the comments in #10 are pretty much valid, so need your review on the following:

  *   returns a renderable array (most often a form or form fragment), an HTML
  *   string, or an array of Ajax commands. If returning a renderable array or
  *   a string, the value will replace the original element named in
  *   #ajax['wrapper']. And the output of status-messages.html.twig  will be
  *   prepended to that element. If the status messages are not wanted, it will return
  *   an array of Ajax commands instead. If an array of Ajax commands is
  *   returned, it will be executed by the calling code.

Better! But I think it can still be improved.

In this part: "And the output of status-messages.html.twig will be prepended to that element." is not a sentence. Either replace the . before it with , or change "And" to "Also,".

Then this sentence: "If the status messages are not wanted, it will return an array of Ajax commands instead."... Um....

So let's look at the context of this text. It's inside @defgroup ajax (the "Ajax framework" topic on api.drupal.org), in a section explaining how to add Ajax handling to a form. It says:

... #ajax supports the following parameters...:

 * - #ajax['callback']: The callback to invoke to handle the server side of the
 *   Ajax event, which will receive a $form and $form_state as arguments, and
 *   returns a renderable array (most often a form or form fragment), an HTML
 *   string, or an array of Ajax commands. If returning a renderable array or
 *   a string, the value will replace the original element named in
...

So this whole section of text is describing a callback function's parameters and return values. Let's make that clearer. Maybe something like this:

The callback to invoke to handle the server side of the Ajax event. The function will be passed parameters $form and $form_state, and should return either a renderable array (most often a form or form fragment), an HTML string, or an array of Ajax commands. If you return a renderable array or a string, ... If you return an array of Ajax commands, they will be executed by the calling code.

(I think we can get rid of the statement about "If the status messages are not wanted" entirely.)

One other thing you could fix in this patch:

+++ b/core/includes/theme.inc
@@ -1005,7 +1005,7 @@ function theme_disable($theme_list) {
  */
 
 /**
- * Preprocess variables for theme_datetime().
+ * Preprocess variables for datetime.html.twig.

Change Preprocess to Preprocesses.

Thanks!

Updated the patch in #9 according to suggestions mentioned in #13. Attaching the new patch and interdiff.

Hm. This didn't come out quite right:

  * - #ajax['callback']: The callback to invoke to handle the server side of the
+ *   Ajax event. The function will be passed parameters $form and $form_state,
+ *   and should return either a renderable array (most often a form or form
+ *   fragment), an HTML string, or an array of Ajax commands. If you return a
+ *   renderable array or a string, or an array of Ajax commands. If you return
+ *   an array of Ajax commands, they will be executed by the calling code.

This sentence near the end: " If you return a renderable array or a string, or an array of Ajax commands." was supposed to say something like this from #12:

... the value will replace the original element named in #ajax['wrapper']. And the output of status-messages.html.twig will be prepended to that element....
(but all one sentence).

Other than that, looks good!

At last, this should fix it.

Yes, thanks! This patch looks good to me now. Any other opinions?

Anyway, leaving at "needs review" because:
- I haven't actually verified that all of the referenced theme functions have really been replaced by the newly-referenced twig functions in core.
- I haven't verified that all the nonexistent theme function references have been covered by the patch.

Found some more such instances and have replaced/removed them in this patch. Need your review on this.

Um...

diff -u b/core/themes/seven/seven.theme b/core/themes/seven/seven.theme
--- b/core/themes/seven/seven.theme
+++ b/core/themes/seven/seven.theme
@@ -136,7 +136,7 @@
 }
 
 /**
- * Overrides theme_custom_block_add_list().
+ * Overrides custom-block-add-list.html.twig.

I don't think you can have a function in a theme file that overrides a Twig template. So if theme_custom_block_add_list() (and the others like that in this file) do not exist, then the overrides should have also been converted to Twig. Which would be a separate issue, but I don't think we should include them in this patch.

Also:

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.php
@@ -27,8 +27,7 @@
  *   If a template file should be used, the file has to be placed in the
  *   module's templates folder.
  *   Example: theme = "mymodule_row" of module "mymodule" will implement either
- *   theme_mymodule_row() or mymodule-row.html.twig in the
- *   [..]/modules/mymodule/templates folder.
+ *   mymodule-row.html.twig in the [..]/modules/mymodule/templates folder.

Read the sentence after the patch. The "either" no longer makes sense.

Oh Okay, I was not aware of the overwrite thing.

Attaching another patch where I didn't touched that. Missed to remove the 'either' in previous patch.

In this patch, I've retained the overrides.

There's still one override in the seven.theme file:

--- a/core/themes/seven/seven.theme
+++ b/core/themes/seven/seven.theme
@@ -185,7 +185,7 @@ function seven_admin_block_content($variables) {
 }
 
 /**
- * Overrides theme_tablesort_indicator().
+ * Overrides tablesort-indicator.html.twig.
  *

And by the way, it looks like issue #1987424: seven.theme - Convert theme_ functions to Twig is taking care of these.

Other than that, looks reasonable... someone still needs to do a more careful check to make sure:
a) All the .twig files that are referenced in this patch actually exist
b) No other theme_...() functions are referenced that should have their references removed.

Removed the change mentioned in #22.

Looks good. Thanks!

Someone still needs to do a more careful check to make sure:
a) All the .twig files that are referenced in this patch actually exist
b) No other theme_...() functions are referenced that should have their references removed.

I will try to do that sometime this week unless someone else wants to?

ok i done a first check and got some more findings

install.core.inc have references to theme_task_list #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig
form.inc,common.inc have references to theme_table #1939008: Convert theme_table() to Twig
themes/* for theme_menu_ functions are changed #1898478: menu.inc - Convert theme_ functions to Twig
modules/system/css/system.theme.css line 371 theme_menu_tree #1898478: menu.inc - Convert theme_ functions to Twig

lib/Drupal/Core/Menu/LocalTaskManager.php line 299 theme_menu_local_task (see above for reference)

References to theme_table exists:
lib/Drupal/Core/Entity/EntityListBuilder.php
lib/Drupal/Core/Entity/Query/QueryInterface.php

edit: adding
includes/common.inc have references to theme_menu_overview_form #1938918: Convert menu theme tables to table #type

unassigned pguillard since he/she hasn't worked on the issue for quite awhile.

Working on it at Drupalcon Austin

A little over my expertise, un-assigning and moving on to a new novice task.

need to reroll this first

ok a reroll done and hopefully working

next step is to go through and find those leftovers in comment 25, some of them are probably not valid any longer though

ok gone through the comment in 25 and also did a new grep and some were dealt with and some new had arised.

But this issue #1898430: menu.module - Convert theme_ functions to Twig is postponed on #1938918: Convert menu theme tables to table #type so theme_menu_overview_form can't be dealt with now.

I did the interdiff with the command interdiff so not sure how that worked out.

Hm...

  * place of a form that contains parent relationships, the form must be themed
  * into a table. The table must have an ID attribute set. If using
- * theme_table(), the ID may be set as follows:
+ * template_preprocess_table(), the ID may be set as follows:

It seems like here we'd be better off saying something like "If using a table type in a render array..." -- or is this really for use inside a template preprocess override function? The rest of the references to template_preprocess_table() are probably good though, since that seems to have a bunch of docs that are being referenced.

Not sure about this...

It does look like a few comment lines are over 80 characters, though, so you might check this?

@jhodgdon I was kinda hoping you were going to say it's all wrong.

I feel that it's a bit strange the theme_table -> template_preprocess_table transition, but looking at the code it looks like it. But using like table.html.twig doesn't feel right either..

Yeah, I think we need to look at these more carefully. The "Right Way" (TM) to do these things these days is to put either #theme or #type into a render array. And it was never really right to call things like theme_table() directly anyway.

So we should probably point people both towards the right way to do things, and any documentation that might exist. I think the idea of pointing to template_preprocess_*() was that there might be some useful docs there. But we could do that like "If you are using #type = 'foo' in a render array, ... . See template_preprocess_foo() for more information." or something like that?

Apparently this comment in #36 didn't make sense. Sorry!

What I was trying to say was that in the previous patch, some references to theme_foo() were changed to be references to template_preprocess_foo(). But without rewriting the documentation, these references do not make sense at all. I think filiijonka was agreeing with that in #35.

For instance that table reference cited in #34 says "If using template_preprocess_table(), the ID may be set as follows" but what it's really trying to say is something like "if you're trying to theme a table by setting #theme to 'table' in a render array..." . So the docs need to be rewritten -- they do not make sense as they are.

In other words, it is not OK to just change references from theme_table() to refer to template_preprocess_table() because they do not make sense in the documentation. On the other hand if there is documentation in the header of template_preprocess_table() that is useful for people to see, then we should also say something like "See the documentation for template_preprocess_table() for more information".

Hopefully that is clearer. Anyway the current patch isn't all that great in places due to problems like this.

@jhodgdon can we change them from @see theme_table() to @see table.html.twig as they the equivalent end point for the rendering?

@see references should be changed to refer to wherever there is documentation for how to use that particular theme function (or some of them may now be render elements). And if there isn't any relevant documentation, I think I would just take out the @see.

The previous few comments were particularly aimed at in-text references, not @see lines, where the flow of words also needs to be considered.

just an explanation to some of the changes I've done from the prev. patch

1. +++ b/core/includes/common.inc
   @@ -2094,7 +2094,7 @@ function _drupal_add_library($library_name, $every_page = NULL) {
   + * template_preprocess_table(), the ID may be set as follows:
   

   I was actually beginning to wonder if we need this reference at all. Simply say the table must have an ID Attribute set and it may be set as follows:

2. +++ b/core/lib/Drupal/Core/Database/Query/TableSortExtender.php
   @@ -37,7 +37,7 @@ class TableSortExtender extends SelectExtender {
   +   * @see template_preprocess_table()
   

   Not really sure why there were an @see at all here. But will set it to table.html.twig

3. +++ b/core/modules/views/views.api.php
   @@ -826,7 +826,7 @@ function hook_views_query_alter(ViewExecutable $view, QueryPluginBase $query) {
   + * @see template_preprocess_table()
   

   Changed this to table.html.twig but doesn't really feel correct.

This is mostly great! A few of the comments still don't quite make sense or need a small adjustment, I think, but I've checked and the rest of this patch is very good. I also checked and all of the references to template files and preprocess functions are to existing files/functions. We're close!

To fix:

a)

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -149,7 +149,7 @@
    * Enables sortable tables for this query.
    *
    * @param $headers
-   *   An array of headers of the same structure as described in theme_table().
+   *   An array of headers of the same structure as described in table.html.twig.

I checked and the docs for table.html.twig do not describe the header structure. They're actually described in the docs for template_preprocess_table(). So we should change that reference.

b)

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -614,9 +614,9 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
 
     $build_info = $form_state->getBuildInfo();
     // If no #theme has been set, automatically apply theme suggestions.
-    // theme_form() itself is in #theme_wrappers and not #theme. Therefore, the
-    // #theme function only has to care for rendering the inner form elements,
-    // not the form itself.
+    // form.html.twig itself is in #theme_wrappers and not #theme. Therefore,
+    // the #theme function only has to care for rendering the inner form
+    // elements, not the form itself.

So... Here, form.html.twig is not really added to anything. What's added is:

      '#theme_wrappers' => array('form'),

So I think the sensible thing to say would be more like "The form theme hook itself, which is rendered by form.html.twig, is in #theme_wrappers ..." right?

c)

+++ b/core/lib/Drupal/Core/Render/Element/Table.php
@@ -234,10 +234,11 @@ class Table extends FormElement {
   }
 
   /**
-   * #pre_render callback to transform children of an element into #rows suitable for theme_table().
+   * #pre_render callback to transform children of an element into #rows
+   * suitable for table.html.twig.

Our docs standards call for the first line of the doc block to be a one-line summary. So this should be probably "Transforms table element children into table rows.", or something like that, and then move the rest into a second paragraph. Or actually, maybe just omit the rest because I think the following section fully describes what is going on.

Will try do to this one in a day or two, just a short question on a, in the table.html.twig there is a @see to the template_preprocess_table(), couldn't that be sufficient? it feels a bit odd I agree on that but perhaps by trying to point the user as much as possible to that file and then people can see it?

Hi

ok thought this should be simple but,

b) can't find this code reference you do,
'#theme_wrappers' => array('form'),

c) perhaps it would be better to try to follow the way the rest of the functions summaries are written in the class so perhaps

* #pre_render callback to transform children of an element of #type 'table'.

b) Regarding the 'form' theme wrapper, this is part of the defaults for the 'form' RenderElement.

c) That would be fine.

thanks for the input.

This is very good!

There is one more remaining very small thing to fix:

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -149,7 +149,8 @@
    * Enables sortable tables for this query.
    *
    * @param $headers
-   *   An array of headers of the same structure as described in theme_table().
+   *   An array of headers of the same structure as described in
+   *   template_preprocess_table().
    *   Use a 'specifier' in place of a 'field' to specify what to sort on.
    *   This can be an entity or a field as described in condition().
    * @return \Drupal\Core\Entity\Query\QueryInterface

Now that the first line has been split up into two lines, the rest of the paragraph needs to be re-wrapped. It would be a bonus if we added a blank line at the end of the @param description before the @return. :)

Also I added a beta evaluation line to the summary.

Thank you @filijonka, I touched up that last extra space in the last patch you uploaded. Depending on your editor there should be an option to remove trailing spaces on save. If it's sublime here a nice setup to keep those things from popping up. http://realityloop.com/blog/2014/03/05/drupal-development-using-sublime-...

I added in theme_input(). callls I found to input.html.twig. and a couple of wrapping issues and item-list.html.twig items too.

Used a quick regular expression to find some stragglers:
for theme_[a-z_]+\(

There may be a few more if you open up that and remove for , just trying to be low impact. Hopefully that helps.

Thanks for the new patch and touch-up job! The latest one looks good.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed e55610f and pushed to 8.0.x. Thanks!