Task

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

Module Theme function name Where in Code What is it really?
image theme_image_anchor function (unusable) table (custom widget)
image theme_image_style_effects function (unusable) form table draggable
image theme_image_style_list function (unusable) table w/ operations

Instructions for manual testing

  1. Install Drupal 8 with the "Standard" profile
  2. Navigate to this path: /admin/config/media/image-styles/manage/large
  3. Make sure the table looks and works the same, before and after applying the patch

Related issues

Files: 
CommentFileSizeAuthor
#55 drupal8.image_module-1938910-55.patch7.87 KBTemoor
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,262 pass(es). View
#53 drupal8.image_module-1938910-53.patch9.54 KBTemoor
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,148 pass(es). View
#51 drupal8.image_module-1938910-51.patch7.76 KBTemoor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,310 pass(es), 4 fail(s), and 0 exception(s). View
#48 drupal8.image-module.1938910-48.patch7.82 KBgnuget
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,374 pass(es). View
#47 interdiff.txt1.97 KBJeroenT
#47 drupal8.image-module.1938910-47.patch7.88 KBJeroenT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8.image-module.1938910-47.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

duellj’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Assigned: Unassigned » larowlan
Status: Active » Postponed
Cottser’s picture

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

andypost’s picture

Status: Postponed » Active

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

andypost’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

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

jibran’s picture

Issue tags: +Novice

Tagging.

Brandonian’s picture

Status: Active » Needs work
FileSize
2.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938910-6-theme-image-effects.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1938910-6-theme-image-effects.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Cross out functions that need not be converted

dsnopek’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,457 pass(es). View

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

dsnopek’s picture

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!

larowlan’s picture

Assigned: larowlan » Unassigned
andypost’s picture

Status: Needs review » Needs work
  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

JeroenT’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,072 pass(es). View

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

Patch attached.

andypost’s picture

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

JeroenT’s picture

FileSize
2.76 KB
PASSED: [[SimpleTest]]: [MySQL] 59,667 pass(es). View

Hi Andy,

I changed the else.

Patch attached.

andypost’s picture

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

JeroenT’s picture

FileSize
11.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,418 pass(es), 104 fail(s), and 12 exception(s). View
2.76 KB
FAILED: [[SimpleTest]]: [MySQL] 59,111 pass(es), 4 fail(s), and 1 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal8.image-module-inline-action.1938910-17.patch, failed testing.

andypost’s picture

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

andypost’s picture

Issue summary: View changes

Added instructions for testing manually.

JeroenT’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 57,966 pass(es). View

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

JeroenT’s picture

FileSize
2.89 KB
PASSED: [[SimpleTest]]: [MySQL] 57,919 pass(es). View

Small change for coding standards.

joelpittet’s picture

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

JeroenT’s picture

FileSize
2.97 KB
FAILED: [[SimpleTest]]: [MySQL] 63,686 pass(es), 0 fail(s), and 108 exception(s). View

Added the comma as suggested by joelpittet.

JeroenT’s picture

Status: Needs review » Needs work

The last submitted patch, 23: drupal8.image-module.1938910-23.patch, failed testing.

joelpittet’s picture

  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.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
3.3 KB
PASSED: [[SimpleTest]]: [MySQL] 63,739 pass(es). View

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

joelpittet’s picture

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.

JeroenT’s picture

FileSize
7.67 KB
FAILED: [[SimpleTest]]: [MySQL] 63,709 pass(es), 6 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 29: drupal8.image-module.1938910-29.patch, failed testing.

pratik60’s picture

Status: Needs work » Needs review
FileSize
7.66 KB
FAILED: [[SimpleTest]]: [MySQL] 64,222 pass(es), 6 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 31: drupal8.image-module.1938910-30.patch, failed testing.

JeroenT’s picture

FileSize
7.68 KB
FAILED: [[SimpleTest]]: [MySQL] 64,220 pass(es), 2 fail(s), and 0 exception(s). View

Fixed some of the remaining issues.

Patch attached.

JeroenT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 33: drupal8.image-module.1938910-31.patch, failed testing.

JeroenT’s picture

FileSize
7.85 KB
PASSED: [[SimpleTest]]: [MySQL] 64,585 pass(es). View

Fixed the last remaining failed tests.

Patch attached.

JeroenT’s picture

Status: Needs work » Needs review
JeroenT’s picture

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: -Novice

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?

JeroenT’s picture

Status: Needs work » Needs review
FileSize
7.87 KB
PASSED: [[SimpleTest]]: [MySQL] 64,655 pass(es). View
600 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1938910-36-40.patch. Unable to apply patch. See the log in the details link for more information. View

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.

JeroenT’s picture

FileSize
600 bytes
joelpittet’s picture

Assigned: Unassigned » joelpittet

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.

Status: Needs review » Needs work

The last submitted patch, 40: interdiff-1938910-36-40.patch, failed testing.

joelpittet’s picture

JeroenT’s picture

Status: Needs work » Needs review
joelpittet’s picture

Assigned: joelpittet » Unassigned

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

JeroenT’s picture

FileSize
7.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal8.image-module.1938910-47.patch. Unable to apply patch. See the log in the details link for more information. View
1.97 KB

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

gnuget’s picture

FileSize
7.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,374 pass(es). View

#47 not apply anymore, here a reroll.

gnuget’s picture

The last submitted patch, 47: drupal8.image-module.1938910-47.patch, failed testing.

Temoor’s picture

FileSize
7.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,310 pass(es), 4 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 51: drupal8.image_module-1938910-51.patch, failed testing.

Temoor’s picture

Status: Needs work » Needs review
FileSize
9.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,148 pass(es). View

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

joelpittet’s picture

@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

Temoor’s picture

FileSize
7.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,262 pass(es). View

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?

joelpittet’s picture

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

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Twig

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

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

  • alexpott committed 83fa87f on 8.0.x
    Issue #1938910 by JeroenT, Temoor, gnuget, pratik60, dsnopek, Brandonian...
m1r1k’s picture

Issue tags: +#ams2014contest

Status: Fixed » Closed (fixed)

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