Convert image theme tables to table #type

Will tackle this once #1788542: Use EntityFormController and EntityListController for image styles is in

I think theme_image_anchor and theme_image_style_list might need to be removed from here since they're not form tables.

theme_image_style_list() will gone with #1809376: Use EntityListController for image styles other issues are nothing about form controller

Updated issue summary.

Thanks @andypost, updated summary to cross out all but theme_image_style_effects and updated #1898420: image.module - Convert theme_ functions to Twig

Tagging.

Needs work, but wanted to get something up before lunch.

Issues with the current patch.

• Tabledrag isn't hooked up properly. The #tabledrag property is added to the table array, but the js isn't firing.
• Table #empty property isn't going to work here, b/c we're manually adding an add form on the bottom of the table, so we always have at least one item
• Not sure how to handle the colspan with the new render item. I tried adding an '#attributes' => array('colspan' => '3') to the 'image_style_new' render element without success.

Re-rolled the patch in #6 so it'll apply cleanly. No other changes were made.

Actually, I started thinking about this and I think this patch is going in the wrong direction. It's replacing the code in theme_image_style_effects(), but what it probably should be doing is REMOVING that theme function all together and put the '#type' => 'table' element directly into the form!

1. +++ b/core/modules/image/image.admin.inc
   @@ -16,50 +16,46 @@
   -      'data' => t('There are currently no effects in this style. Add one by selecting an option below.'),
   

   maybe #empty needed here

2. +++ b/core/modules/image/image.admin.inc
   @@ -16,50 +16,46 @@
   + ¶
   

   trailing whitespace

Made changes to dsnopek's patch as suggested by andypost.

Patch attached.

Awesome! Just a small nitpick

+++ b/core/modules/image/image.admin.inc
@@ -16,50 +16,47 @@
+    } else {

'Else' should be on new line, see https://drupal.org/coding-standards#controlstruct

Hi Andy,

I changed the else.

Patch attached.

And the last one, @JeroenT sorry that I missed that in previous review
Code now much cleaner!
It would be good to make follow-up to remove this ugly "add effect" drop down and use inline action a-la we have for menu links and proposed for fields #1963340: Change field UI so that adding a field is a separate task

+++ b/core/modules/image/image.admin.inc
@@ -16,50 +16,48 @@
-      // Add the row for adding a new image effect.

please leave the comment here, it useful to point wtf going here

I made 2 patches.

drupal8.image-module.1938910-17.patch is the one where I added the comment again.

drupal8.image-module-inline-action.1938910-17.patch is the patch where i started on the inline action. It works locally but there are still some tests failing (I think they need to be changed).

@JeroenT Let's leave the scope of that issue to straight conversion
Filed #2116663: Move Add new effect out of effects table

Added instructions for testing manually.

Readded the comment + made some changes to fix the tabledrag.

Small change for coding standards.

+++ b/core/modules/image/image.admin.inc
@@ -16,50 +16,55 @@
+      'id' => 'image-style-effects'

Mind adding a comma to the end of the array item?

This patch is looking pretty close to ready.

Added the comma as suggested by joelpittet.

1. +++ b/core/modules/image/image.admin.inc
   @@ -16,56 +16,55 @@
   -      array(
   -        'action' => 'order',
   -        'relationship' => 'sibling',
   -        'group' => 'image-effect-order-weight',
   -      ),
   ...
   +      array('order', 'sibling', 'image-effect-order-weight'),
   

   #type=>table got some improvements recently it seems, the array going in is key'd, that's the issue here.

2. +++ b/core/modules/image/image.admin.inc
   @@ -16,56 +16,55 @@
   +          '#markup' => drupal_render($form[$key]['label']) . (empty($summary) ? '' : ' ' . $summary),
   

   This can very likely be done without drupal_render() being called which would really help. Want to give it a crack or more details on how that would be done?

3. +++ b/core/modules/image/image.admin.inc
   @@ -16,56 +16,55 @@
   -      $row[] = '<div class="image-style-new">' . drupal_render($form['new']['new']) . drupal_render($form['new']['add']) . '</div>';
   

   I think we are missing the class from this. It's on the div so I think #type=>container or something like that could do the trick.

This patch should fix all the exceptions + the suggestions made by joelpittet.

Nice work on getting it green. Dreditor seem to be buggered for me so I'll wait to do a code review.
Can you drop the theme function out of this equation, all it does is create a table.

All the code here get's simplifed a bit by moving it to ImageStyleEditForm::form.
Also, if you can prevent the early rendering inside the table it's even better(no drupal_render calls, I'm quite sure it's possible:)

There is a 4 space indent in there too that should be 2.

Have a look at https://drupal.org/node/1876710 for reference on keeping the 'draggable' class attribute consistent.

I think we are almost there, but I can't find a way to add the "image-style-order-weight" class to the td elements.

It was a small fix, the associative array for tablegroup was wrong.

Fixed some of the remaining issues.

Patch attached.

Fixed the last remaining failed tests.

Patch attached.

Great work @JeroenT, just a couple things and questions for you. I'll do a manual test after that to see how things are working and matching up.

1. +++ b/core/modules/image/lib/Drupal/image/Form/ImageStyleEditForm.php
   @@ -68,22 +68,54 @@ public function form(array $form, array &$form_state) {
   +        t('Effect'),
   +        t('Weight'),
   +        t('Operations'),
   

   You can now use $this->t inside the form, to remove the global dependency on the t function, which will likely turn into a trait method once PHP 5.4 is in.

2. +++ b/core/modules/image/lib/Drupal/image/Form/ImageStyleEditForm.php
   @@ -127,25 +148,39 @@ public function form(array $form, array &$form_state) {
   +    $form['effects']['new']['operations'] = array(
   +      'data' => array(),
        );
   

   Is this to be an empty cell?

1. I changed the t() function with $this->t().

2. Yes. Currently it's done like this in theme_image_style_effects() :

$row[] = '';

Maybe it's better to add a colspan to the td element, but I can't find a way to add it.

Thanks @JeroenT you rock. I don't think there is a colspan ability in type table. I spotted #1065828: Make type #table support colspan for cells so that conversion should be good enough.

I'll do a manual testing on this and if I see a solution I'll let you know.

Hmm, haven't had a time to do manual testing, I'll have to just open this up and unassign for now.

After reviewing the HTML-code I still found a difference. After applying this patch the HTML-code has no differences.

#47 not apply anymore, here a reroll.

47: drupal8.image-module.1938910-47.patch queued for re-testing.

#48 not applied
Re-rolled patch, added missing table header.

Updated tests to match with name and proportions splitted in separate columns.

@Temoor why did the tests need to change?

And can you provide an interdiff for the changes so we don't have to review each like for each new patch and concentrate on the changes between them. @see https://drupal.org/documentation/git/interdiff

I've missed two lines while making reroll.
Attached patch doesn't contain any additional changes and is clean reroll of #48.
Btw, how interdiff can be made with a patch that doesn't apply anymore?

@Temoor good question, usually just post the re-roll first then do another patch for the changes. It gives a chance for the testbot to run against the re-roll to make sure no mistakes were made. Then the interdiff can tell the people reviewing the patch what has changed since the actual last change, which is much tougher to do mixed in with the re-roll changes as well.

Tested manually, works fine and BTW also solves part of #2318297: Rotate effect: do not use html entity for 'degree' but its unicode character i.e. double-escaping of HTML in the image style effects table. RTBC for me. Adding Twig tag as it would reduce count of theme_* functions by one.

Tested manually and nice one less SafeMarkup::set() and one less theme function!

Committed 83fa87f and pushed to 8.0.x. Thanks!