Task

Convert the following theme functions to use the new table #type:

Module Theme function name Where in Code What is it really?
field_ui theme_field_ui_table function (unusable) table

Steps to Reproduce

  • Visit admin/structure/types/manage/article/form-display
  • Test form saves.
  • Compare markup.

Related issues

Files: 
CommentFileSizeAuthor
#56 convert-1938900-55.patch19.8 KBjoelpittet
#56 interdiff.txt5.05 KBjoelpittet
#50 interdiff.txt7.52 KBlauriii
#50 convert_field_ui_theme-1938900-50.patch19.19 KBlauriii
#48 convert_field_ui_theme-1938900-48.patch19.01 KBjoelpittet
#44 interdiff.txt579 bytesCottser
#43 convert_field_ui_theme-1938900-43.patch7.68 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 116,647 pass(es), 438 fail(s), and 32 exception(s). View
#40 interdiff.txt1.82 KBjoelpittet
#40 convert_field_ui_theme-1938900-40.patch7.74 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,476 pass(es), 1 fail(s), and 0 exception(s). View
#35 1938900-34.txt658 byteslokapujya
#28 convert_field_ui_theme-1938900-28.patch7.66 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,610 pass(es), 27 fail(s), and 14 exception(s). View

Comments

swentel’s picture

Issue tags: +Field API

tagging

Cottser’s picture

Status: Active » Postponed

Since this isn't a 'form' table, I think it will just need a Twig conversion for now. Tentatively postponing this one and activating #1898068: field_ui.module - Convert theme_ functions to Twig.

If I'm wrong, please correct me :)

Cottser’s picture

Status: Postponed » Active

Should have confirmed with @duellj first, but he said this would actually work better as #type table. Sorry for the mix-up.

duellj’s picture

Status: Active » Postponed

Unfortunately, this table conversions requires colspans, which aren't yet supported by #type table. This issue needs to make it in first: #1948374: #type 'table' allow attributes on table cells

swentel’s picture

Status: Postponed » Active

That issue is in, let's go :)

neochief’s picture

Assigned: Unassigned » neochief
jibran’s picture

Issue tags: +Novice

Tagging.

neochief’s picture

Now, after reviewing the field_ui code I'm not even sure that this task should be done at all.

theme_field_ui_table() is used for field_ui_table element rendering. I don't see a reason to translate one element into another, since in the end of the day, they both use theme_table().

I also was thinking about removing field_ui_table element at all in favor of table element, but it's used in several places, so it's not a really good idea.

There might be some other approach, but I'm not sure I understand what you guys had in mind during your conversations before. So, brief recap would be welcomed.

swentel’s picture

Yeah, field ui is also kind of special doing funky stuff. Also there's an issue to rethink Field UI a little over at #1963340: Change field UI so that adding a field is a separate task, so maybe we should defer to that issue and close this one ?

neochief’s picture

Status: Active » Closed (fixed)

Yes, I think so. Please, reopen the issue if you think otherwise.

neochief’s picture

Status: Closed (fixed) » Closed (won't fix)
swentel’s picture

Issue tags: -Novice, -Field API

Removing tags for now.

tstoeckler’s picture

Issue summary: View changes
Status: Closed (won't fix) » Active

That one was fixed but the horror that is theme_field_ui_table() still exists.

joelpittet’s picture

Status: Active » Needs review
FileSize
6.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,945 pass(es), 147 fail(s), and 54 exception(s). View

This probably hella wrong but on the right track... I think... Let's see how much tests this has:)

joelpittet’s picture

  1. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,89 @@
    +      '#pre_render' => [
    +        [$parent_class, 'preRenderTable'],
    +        [$class, 'preRenderTable'],
    +      ],
    

    I expect this will preRender in the top to bottom order.

  2. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,89 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function preRenderTable($element) {
    

    This prerender method probably needs some thourough vetting, I mostly dumped it in and changed the array syntax.

Status: Needs review » Needs work

The last submitted patch, 14: convert_field_ui_theme-1938900-14.patch, failed testing.

joelpittet’s picture

Assigned: neochief » Unassigned
Issue tags: +Needs steps to reproduce, +Twig, +Needs manual testing

Ok, well doesn't look too bad, probably need to just find an example someplace. Then just step through this with the debugger and modify till it works.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
8.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: tests were executed, but no results were found. View
2.73 KB
+++ b/core/modules/field_ui/src/Element/FieldUiTable.php
@@ -7,23 +7,89 @@
  * @RenderElement("field_ui_table")

1. Should this be a FormElement? since Table is a FormElement.

2. Had to do some scary things to get the table to render. See interdiff.

tstoeckler’s picture

Hmm... IMO it doesn't make much sense to add FieldUiTable as a dedicated element. Now that we have '#type' => 'table' we should be able to set things like the colspan etc. that the pre-render function is currently doing directly in the form array when putting it together.

joelpittet’s picture

@lokapujya it is an extended Table element, since Table is a FormElement, it should be fine that we also extend Table.

  1. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -25,11 +26,9 @@ public function getInfo() {
    -      '#regions' => ['' => []],
    

    This is the difference between the definition of a table and this type of table.

  2. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -25,11 +26,9 @@ public function getInfo() {
    -        [$parent_class, 'preRenderTable'],
             [$class, 'preRenderTable'],
    

    Unfortunately the union will clobber the existing '#pre_render' key and the Table::preRenderTable static method will not be called.

  3. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -807,7 +807,7 @@ public function tablePreRender($elements) {
    -      $elements['#regions'][$region_name]['rows_order'] = array_reduce($trees[$region_name], array($this, 'reduceOrder'));
    +      $elements['#regions'][$region_name]['rows_order'] = array_reduce($trees[$region_name], array('Drupal\field_ui\Form\EntityDisplayFormBase', 'reduceOrder'));
    
    @@ -838,7 +838,7 @@ public function reduceOrder($array, $a) {
    -      $array = array_merge($array, array_reduce($a['children'], array($this, 'reduceOrder')));
    +      $array = array_merge($array, array_reduce($a['children'], array('Drupal\field_ui\Form\EntityDisplayFormBase', 'reduceOrder')));
    

    I'd hope these changes don't need to be made?

joelpittet’s picture

Sorry that should be more clear... we are making a #type => table in the theme function. Type=>table is \Drupal\Core\Render\Element\Table

That is why it should be golden.

joelpittet’s picture

@lokapujya what page are you using to see this?

Status: Needs review » Needs work

The last submitted patch, 18: 1938900-17.patch, failed testing.

lokapujya’s picture

admin/structure/types/manage/article/form-display

@tstoeckler -

we should be able to set things like the colspan etc. that the pre-render function is currently doing directly in the form array when putting it together.

Great, that will make this help clean this up.

[$parent_class, 'preRenderTable'],
Removed that because the rows were getting duplicated.

I'd hope these changes don't need to be made?

It is because $this isn't available in a static function.

Well, I need a break from coding this issue. Spent too much time messing with preRender.

lokapujya’s picture

+++ b/core/modules/field_ui/src/Element/FieldUiTable.php
@@ -25,11 +26,9 @@ public function getInfo() {
-      '#regions' => ['' => []],

Reverted that change; It was irrelevant to getting the table to render.

joelpittet’s picture

Oh they are being duplicated because I dumped the theme function logic for building the rows almost verbatim into the new prerender function. Sorry, my fault on that. I just tried to kick start this issue(didn't test).

There is only one instance of this type/theme function being used in core. I agree with @tstoeckler, we should just kill it off and move the necessary logic if possible to that.

Kind which I knew if contrib was using this at all... (Thinking about Greggles' Gotta Download Them All module to save the day, if I knew it was up to date to some degree)

lokapujya’s picture

Ran one of the tests locally. The scary change pointed out in #20.3 causes a bunch of exceptions, and might be why the tests didn't run.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs steps to reproduce
FileSize
7.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,610 pass(es), 27 fail(s), and 14 exception(s). View

Ok no interdiff cus this is a different direction.

@tstoeckler this direction along the lines of what you suggested?

Status: Needs review » Needs work

The last submitted patch, 28: convert_field_ui_theme-1938900-28.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 28: convert_field_ui_theme-1938900-28.patch, failed testing.

andypost’s picture

Issue tags: +Entity Field API

Intentional removal of drupal_render($child) looks breaks forms embedded in this table

  1. +++ b/core/modules/field_ui/field_ui.module
    @@ -189,87 +177,6 @@ function field_ui_view_mode_delete(EntityViewModeInterface $view_mode) {
    -    // Add form rows, in the order determined at pre-render time.
    -    foreach ($region['rows_order'] as $name) {
    -      $element = $elements[$name];
    -
    -      $row = array('data' => array());
    
    +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -811,6 +811,73 @@ public function tablePreRender($elements) {
    +      // Add form rows, in the order determined at pre-render time.
    +      foreach ($region['rows_order'] as $name) {
    +        $element = $elements[$name];
    +        if (isset($element['data'])) {
    +          // Already built?
    +          continue;
    +        }
    +        $row = ['data' => []];
    

    This looks strange

  2. +++ b/core/modules/field_ui/field_ui.module
    @@ -189,87 +177,6 @@ function field_ui_view_mode_delete(EntityViewModeInterface $view_mode) {
    -      // Render children as table cells.
    -      foreach (Element::children($element) as $cell_key) {
    -        $child = &$element[$cell_key];
    -        // Do not render a cell for children of #type 'value'.
    -        if (!(isset($child['#type']) && $child['#type'] == 'value')) {
    -          $cell = array('data' => drupal_render($child));
    
    +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -811,6 +811,73 @@ public function tablePreRender($elements) {
    +        // Render children as table cells.
    +        foreach (Element::children($element) as $cell_key) {
    +          $child = $element[$cell_key];
    +          // Do not render a cell for children of #type 'value'.
    +          if (!(isset($child['#type']) && $child['#type'] == 'value')) {
    +            $cell = ['data' => $child];
    

    somehow render of cell lost

joelpittet’s picture

Status: Needs work » Needs review

Intentional removal of drupal_render($child) looks breaks forms embedded in this table

In what way does it break them? I tested this and the form "seemed" to be working fine.

Re: #32.1 Yes, yes it does... need to find out how or why the rows are already converted for #theme => table on the $element.

Re: #32.1 I have a theory here... maybe those value keys need to be printed and that whole don't render #type => value, is because they were probably already rendered before they hit the theme function before or maybe they just need to be rendered outside the table.

joelpittet’s picture

Status: Needs review » Needs work

Whoops still needs work/new patch. Thanks for the review @andypost

lokapujya’s picture

FileSize
658 bytes

Pull Request. Remove mention of field_ui_element_info.

lokapujya’s picture

What's different about the field created in the failing test, in that it doesn't show up in the /form-display table? edit: If you get the /display page, field_test does show up.

edit: in other words, why is field_test created via fieldUIAddNewField() one of the "already built" elements? but, a new field added via the UI does show up in the table.

andypost’s picture

@lokapujya probably because some properties are not configured for field.
PS: patch still applies with ofset

The last submitted patch, 28: convert_field_ui_theme-1938900-28.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,476 pass(es), 1 fail(s), and 0 exception(s). View
1.82 KB

I may have got the prerenders in the wrong order and there is duplicates now but the tests may pass...

It was missing some of the table preprocessing which made the extra fields missing for example 'links' and the responsive classes were missing.

Status: Needs review » Needs work

The last submitted patch, 40: convert_field_ui_theme-1938900-40.patch, failed testing.

The last submitted patch, 40: convert_field_ui_theme-1938900-40.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
7.68 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 116,647 pass(es), 438 fail(s), and 32 exception(s). View

Removing the original table render seems to get things closer to me (the responsive stuff is probably not relevant in this case either) but field_tags is missing (probably similar to what you were mentioning with links @joelpittet), weird things are happening. This is a tricky one. Just seeing what the bot thinks about this one.

What I was seeing was the $elements array seems to get messed up in the loop and field_tags gets replaced with the data values from the body field or something equally odd.

Cottser’s picture

FileSize
579 bytes

Missing interdiff.

Status: Needs review » Needs work

The last submitted patch, 43: convert_field_ui_theme-1938900-43.patch, failed testing.

The last submitted patch, 43: convert_field_ui_theme-1938900-43.patch, failed testing.

Cottser’s picture

Maybe we need to keep the table pre_render but switch up what we do in the specific pre render to be even moreso like moving things around. Haven't yet studied in detail what is happening in each step.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
19.01 KB

Now for something completely different...

lauriii’s picture

Status: Needs review » Needs work

Great to see the overall work. Amazing work @joelpittet!!

  1. +++ b/core/modules/field_ui/field_ui.module
    @@ -218,75 +227,8 @@ function field_ui_entity_form_mode_delete(EntityFormModeInterface $form_mode) {
    -function theme_field_ui_table($variables) {
    

    \o/

  2. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,207 @@
    +    // $info['#pre_render'][] = [$class, 'tablePreRender'];
    

    This probably shouldn't be there anymore

  3. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,207 @@
    +   * @return array
    +   *
    

    Missing return comment

  4. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,207 @@
    +            $indentation = array(
    +              '#theme' => 'indentation',
    +              '#size' => $depth,
    +            );
    +            $row[$cell]['#prefix'] = drupal_render($indentation) . (isset($row[$cell]['#prefix']) ? $row[$cell]['#prefix'] : '');
    

    Do we actually need the drupal_render there?

  5. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,207 @@
    +      $elements['#regions'][$region_name]['rows_order'] = array_reduce($trees[$region_name], array('Drupal\field_ui\Element\FieldUiTable', 'reduceOrder'));
    

    We can also say array($this, 'reduceOrder')

  6. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,207 @@
    +   * Determines the rendering order of an array representing a tree.
    +   *
    +   * Callback for array_reduce() within
    +   * \Drupal\field_ui\Form\EntityDisplayFormBase::tablePreRender().
    +   */
    +  public static function reduceOrder($array, $a) {
    

    Is missing param docs

  7. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,207 @@
    +      $array = array_merge($array, array_reduce($a['children'], array('Drupal\field_ui\Element\FieldUiTable', 'reduceOrder')));
    

    use $this instead

  8. +++ b/core/modules/field_ui/templates/field-ui-table.html.twig
    @@ -0,0 +1,47 @@
    + * Default theme implementation to display a field UI table.
    

    s/field/Field

lauriii’s picture

Status: Needs work » Needs review
FileSize
19.19 KB
7.52 KB

Fixed the nits I found on my own review

Cottser’s picture

Just for the record I did a markup diff, looks great! Will try to find some time to review the patch soon.

Cottser’s picture

Just wondering if we should re-title this one along the lines of the language table one. Much more complex but overall feels like we're doing something similar.

lauriii’s picture

Title: Convert field_ui theme tables to table #type » Convert theme_field_ui_table into a template
Cottser’s picture

Status: Needs review » Needs work

Overall looks very good, some docs things and some thoughts on preserving BC below:

  1. +++ b/core/modules/field_ui/field_ui.module
    @@ -218,75 +227,8 @@ function field_ui_entity_form_mode_delete(EntityFormModeInterface $form_mode) {
      *
      * @ingroup themeable
      */
    -function theme_field_ui_table($variables) {
    ...
    +function template_preprocess_field_ui_table(&$variables) {
    

    We need to update the docblock here now that it's a preprocess function.

    https://www.drupal.org/node/1354#themepreprocess

  2. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,221 @@
    +  public static function tablePreRender($elements) {
    ...
    +  public static function reduceOrder($array, $a) {
    
    +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -678,119 +673,6 @@ public function multistepAjax($form, FormStateInterface $form_state) {
    -  public function tablePreRender($elements) {
    ...
    -  public function reduceOrder($array, $a) {
    

    For the least disruption I think we should consider that these methods on EntityDisplayFormBase (reduceOrder and tablePreRender) should be deprecated and wrap the FieldUiTable methods. We may not necessarily consider them API but they are public methods and not marked @internal so we should be mindful of that.

    Continuing along that same line of thought, we would have two pre-renders in FieldUiTable, one to do the same as the previous pre-render, and one to do the similar things the theme function was doing before.

    I think that or something along those lines is probably the right thing to do to preserve BC at this point.

  3. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,221 @@
    +   * Callback for array_reduce() within
    +   * \Drupal\field_ui\Form\EntityDisplayFormBase::tablePreRender().
    

    The comment here should be updated because it doesn't live inside that class any more.

  4. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,221 @@
    +   * @param mixed $array
    +   *   Holds the return value of the previous iteration;  in the case of the
    +   *   first iteration it instead holds the value of initial.
    

    Minor: two spaces after the semicolon, should just be one.

    The wording towards the end of this sentence gets a bit vague, what does "value of initial" mean? "holds the initial value"?

  5. +++ b/core/modules/field_ui/templates/field-ui-table.html.twig
    @@ -0,0 +1,47 @@
    + * @see template_preprocess_table()
    

    Should we consider pointing to template_preprocess_field_ui_table() instead here, in case that ever gets changed? Just a thought, feel free to say no.

  6. +++ b/core/modules/field_ui/templates/field-ui-table.html.twig
    @@ -0,0 +1,47 @@
    +  {% include 'table.html.twig' %}
    

    Observation: Cool :)

xjm’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
19.8 KB

Tried to address #54

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

I looked at this thoroughly and manually tested again, only two minor docs points. Otherwise RTBC, thanks all!

  1. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,232 @@
    +    // Prepend FieldUiTable's prerender.
    

    Super duper duper minor, can be fixed on commit: s/prerender/prerenders/.

  2. +++ b/core/modules/field_ui/src/Element/FieldUiTable.php
    @@ -7,23 +7,232 @@
    +   * @param array $elements
    +   *   A structured array containing two sub-levels of elements. Properties
    +   *   used:
    +   *   - #tabledrag: The value is a list of $options arrays that are passed to
    +   *     drupal_attach_tabledrag(). The HTML ID of the table is added to each
    +   *     $options array.
    ...
    +   * @param array $elements
    +   *
    

    Minor: Maybe we could copy over the param docs? preRenderRegionRows is missing docs here.

  • webchick committed 5c5cecc on 8.0.x
    Issue #1938900 by joelpittet, lokapujya, Cottser, lauriii, swentel,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

DIE LAST THEME FUNCTION DIE!!

Committed and pushed to 8.0.x. YEAH! :D

Status: Fixed » Closed (fixed)

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

swentel’s picture