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

There are references to theme functions in core docs that are now converted to Twig templates and therefore don't exist any longer (broken links on api.drupal.org is just one negative effect this has).

Examples:

  • theme_field() is now (or will soon be again!) field.html.twig
  • theme_item_list() is now item-list.html.twig
  • theme_image() is now image.html.twig

The theme function to Twig conversion effort (#1757550: [Meta] Convert core theme functions to Twig templates) does a lot of things and each patch goes through quite a rigorous process. But we're certainly not perfect!

Proposed resolution

Search through core and find all references to theme_ functions. Update them if they are referencing theme functions that no longer exist or have been converted to Twig.

Remaining tasks

Beta evaluation: This is just API docs and code comments (documentation), so it is an unfrozen, prioritized change and allowed during beta.

User interface changes

n/a

API changes

n/a

Files: 
CommentFileSizeAuthor
#51 interdiff.txt16.37 KBjoelpittet
#51 update_stale_references-2226863-51.patch29.33 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,051 pass(es). View
#47 interdiff-2226863-45-47.txt846 bytesfilijonka
#47 2226863-47.patch14.53 KBfilijonka
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,962 pass(es). View
#45 interdiff-2226863-39-45.txt2.3 KBfilijonka
#45 2226863-45.patch14.35 KBfilijonka
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,834 pass(es). View
#40 interdiff-2226863-33-39.txt5.51 KBfilijonka
#40 2226863-39.patch14.33 KBfilijonka
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,301 pass(es). View
#33 interdiff-2226863-32-33.txt4.75 KBfilijonka
#33 2226863-33.patch14.88 KBfilijonka
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,586 pass(es). View
#32 2226863-32.patch9.21 KBfilijonka
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,556 pass(es). View
#23 interdiff-21-23.txt294 bytescs_shadow
#23 drupal-update_stale_references_to_theme_functions-2226863-23.patch13.34 KBcs_shadow
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,691 pass(es). View
#21 interdiff-19-21.txt1.33 KBcs_shadow
#21 drupal-update_stale_references_to_theme_functions-2226863-21.patch13.76 KBcs_shadow
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,700 pass(es). View
#19 interdiff-16-19.txt3.89 KBcs_shadow
#19 drupal-update_stale_references_to_theme_functions-2226863-19.patch14.14 KBcs_shadow
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,646 pass(es). View
#16 interdiff-14-16.txt996 bytescs_shadow
#16 drupal-update_stale_references_to_theme_functions-2226863-16.patch9.77 KBcs_shadow
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,591 pass(es). View
#14 interdiff-9-14.txt3.04 KBcs_shadow
#14 drupal-update_stale_references_to_theme_functions-2226863-14.patch9.65 KBcs_shadow
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,647 pass(es). View
#9 drupal-update_stale_references_to_theme_functions-2226863-9.patch9.4 KBcs_shadow
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,654 pass(es). View
#6 update_stale_references_to_theme_functions-2226863.patch8.97 KBpguillard
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,922 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

Adding some examples and a link to the Twig conversion meta to the issue summary.

pguillard’s picture

Assigned: Unassigned » pguillard
pguillard’s picture

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

Cottser’s picture

Thanks @pguillard!

Theme_ functions that still exist, no twig template yet:

These should be left as is.

Theme_ functions that still exist, a twig template is available AND
Replacements:

These should both be updated, the only reason field-html.twig and theme_field() still exist is because we're waiting for a commit on #2226233: Redo changes from field.module to Twig conversion issue that were clobbered.

Functions have been removed, but no twig template yet

Some of these have Twig templates and should be updated if we have documentation referring to them:

  1. book-tree.html.twig
  2. tablesort-indicator.html.twig
  3. breadcrumb.html.twig
  4. datetime.html.twig

Others are a bit special:

  1. theme_mymodule_row() is just an example and can probably be left as is :)
  2. The reference to theme_menu_tree__shortcut_default() is pending conversion: #1898478: menu.inc - Convert theme_ functions to Twig
  3. The reference to theme_simpletest_test_table() can be removed because that has been converted to #type table and no longer uses \Drupal\Component\Utility\SortArray::sortByTitleProperty().
  4. I traced the reference to theme_update_available_updates_form() all the way back to #538660: Move update manager upgrade process into new authorize.php file (and make it actually work) but it didn't exist then (I checked out 6abcc47e25e936aea84c2f1e287bc5e1a045fff4) so I think it can be safely removed.
scaragucc'’s picture

Working on this during Dev Days.

pguillard’s picture

FileSize
8.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,922 pass(es). View

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.

Cottser’s picture

Status: Active » Needs review

Thanks @pguillard, sending to testbot (status = needs review).

jhodgdon’s picture

Status: Needs review » Needs work

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!

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
9.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,654 pass(es). View

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

filijonka’s picture

*   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

jhodgdon’s picture

Status: Needs review » Needs work
cs_shadow’s picture

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.
jhodgdon’s picture

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!

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
9.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,647 pass(es). View
3.04 KB

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

jhodgdon’s picture

Status: Needs review » Needs work

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!

cs_shadow’s picture

FileSize
9.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,591 pass(es). View
996 bytes

At last, this should fix it.

cs_shadow’s picture

Status: Needs work » Needs review
jhodgdon’s picture

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.

cs_shadow’s picture

FileSize
14.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,646 pass(es). View
3.89 KB

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

jhodgdon’s picture

Status: Needs review » Needs work

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.

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
13.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,700 pass(es). View
1.33 KB

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.

jhodgdon’s picture

Status: Needs review » Needs work

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.

cs_shadow’s picture

Status: Needs work » Needs review
FileSize
13.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,691 pass(es). View
294 bytes

Removed the change mentioned in #22.

jhodgdon’s picture

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?

filijonka’s picture

Status: Needs review » Needs work

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

filijonka’s picture

Assigned: pguillard » Unassigned

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

legaudinier’s picture

Assigned: Unassigned » legaudinier

Working on it at Drupalcon Austin

legaudinier’s picture

Assigned: legaudinier » Unassigned

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

joelpittet’s picture

Issue tags: +Amsterdam2014
jsobiecki’s picture

Issue tags: -Amsterdam2014 +dcwroc2014
filijonka’s picture

Assigned: Unassigned » filijonka
Issue tags: -dcwroc2014

need to reroll this first

filijonka’s picture

Status: Needs work » Needs review
FileSize
9.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,556 pass(es). View

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

filijonka’s picture

FileSize
14.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,586 pass(es). View
4.75 KB

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.

jhodgdon’s picture

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?

filijonka’s picture

@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..

jhodgdon’s picture

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?

jhodgdon’s picture

Status: Needs review » Needs work

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.

joelpittet’s picture

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

jhodgdon’s picture

@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.

filijonka’s picture

Status: Needs work » Needs review
FileSize
14.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,301 pass(es). View
5.51 KB

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.

jhodgdon’s picture

Status: Needs review » Needs work

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.

filijonka’s picture

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?

filijonka’s picture

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'.

jhodgdon’s picture

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

c) That would be fine.

filijonka’s picture

Assigned: filijonka » Unassigned
Status: Needs work » Needs review
FileSize
14.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,834 pass(es). View
2.3 KB

thanks for the input.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Needs work

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.

filijonka’s picture

Status: Needs work » Needs review
FileSize
14.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,962 pass(es). View
846 bytes

Status: Needs review » Needs work

The last submitted patch, 47: 2226863-47.patch, failed testing.

filijonka queued 47: 2226863-47.patch for re-testing.

filijonka’s picture

Status: Needs work » Needs review
joelpittet’s picture

FileSize
29.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,051 pass(es). View
16.37 KB

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.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed e55610f on 8.0.x
    Issue #2226863 by cs_shadow, filijonka, joelpittet, pguillard: Update...

Status: Fixed » Closed (fixed)

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