Convert theme_field_ui_table into a template

tagging

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 :)

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

That issue is in, let's go :)

Tagging.

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.

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 ?

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

Removing tags for now.

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

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

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.

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.

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

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.

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

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.

@lokapujya what page are you using to see this?

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.

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

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)

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.

Ok no interdiff cus this is a different direction.

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

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

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.

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

Pull Request. Remove mention of field_ui_element_info.

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.

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

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.

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.

Missing interdiff.

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.

Now for something completely different...

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

Fixed the nits I found on my own review

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

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.

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 :)

Similarly to #1938912: Convert language content setting table theme to a twig template and as a child of #2348381: [META-0 theme functions left] Convert/refactor core theme functions, this is a good rc target!

Tried to address #54

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.

DIE LAST THEME FUNCTION DIE!!

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

Follow up on #2605518: Table drag indentation is not rendered