Once #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) is in, we should convert image styles to use ConfigEntityListController.

Proposed

1) introduce ImageStyleListController class
2) Remove theme_image_styles() function - not needed with new #table
3) Change path for core's default
4) Fix tests
5) Style labels should be bold links according suggestion #1814916-65: Convert menus into entities

1809376-image-list.png

Follow ups

Once admin-path fixed proceed with forms #1788542: Use EntityFormController and EntityListController for image styles

Files: 
CommentFileSizeAuthor
#61 1809376-61.patch11.28 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,421 pass(es). View
#61 interdiff.txt5.31 KBfubhy
#59 1809376-59.patch10.63 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,514 pass(es). View
#55 interdiff.txt3.18 KBspeely
#55 1809376-imageList-55.patch10.47 KBspeely
PASSED: [[SimpleTest]]: [MySQL] 56,824 pass(es). View
#49 interdiff.txt1.89 KBandypost
#49 1809376-imageList-49.patch7.97 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es). View
#46 interdiff.txt417 bytesandypost
#46 1809376-imageList-46.patch7.92 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,209 pass(es), 1 fail(s), and 0 exception(s). View
#42 interdiff.txt1.39 KBandypost
#42 1809376-imageList-42.patch7.91 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,247 pass(es). View
#41 interdiff.txt3.46 KBandypost
#41 1809376-imageList-41.patch7.6 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,282 pass(es), 1 fail(s), and 0 exception(s). View
#36 1809376-imageList-36.patch6.29 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,890 pass(es). View
#32 1809376-imageList-32.patch6.29 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#32 interdiff.txt729 bytesandypost
#26 interdiff.txt2.75 KBandypost
#26 1809376-imageList-26.patch6.19 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es). View
#25 1809376-imageList-25.patch4.8 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,243 pass(es). View
#24 interdiff.txt1.53 KBandypost
#24 1809376-imageList-24.patch4.75 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,857 pass(es). View
#23 1809376-image-list-22.patch5.34 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,725 pass(es). View
#22 interdiff.txt1.05 KBandypost
#22 1809376-image-list-20.patch5.49 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#20 interdiff.txt1.14 KBandypost
#20 1809376-image-list-20.patch5.49 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,173 pass(es). View
#19 1809376-image-list.png19.16 KBandypost
#19 1809376-interdiff-19.txt1.98 KBandypost
#19 1809376-image-list-19.patch5.43 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 53,620 pass(es). View
#14 1809376-interdiff-14.txt611 bytesandypost
#14 1809376-image-list-14.patch4.81 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,646 pass(es). View
#14 before and #14 2013-01-18 00:44:55.png12.39 KBandypost
#14 after #7 18.01.2013 - 00:47:01.png13.67 KBandypost
#15 after-7.png13.67 KBandypost
#15 before-and-14.png12.39 KBandypost
#7 1809376-image-list-7.patch4.71 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,628 pass(es). View
#4 1809376-interdiff-4.txt1.94 KBandypost
#4 1809376-image-list-4.patch17.25 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,826 pass(es). View
#3 1809376-image-list-3.patch18.38 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,882 pass(es). View

Comments

andypost’s picture

andypost’s picture

andypost’s picture

Status: Closed (duplicate) » Needs review
FileSize
18.38 KB
PASSED: [[SimpleTest]]: [MySQL] 49,882 pass(es). View

Suppose this should be fixed before #1788542: Use EntityFormController and EntityListController for image styles

We need to re-manage a style uri anyway

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

FileSize
17.25 KB
PASSED: [[SimpleTest]]: [MySQL] 49,826 pass(es). View
1.94 KB

Clean-up copy-paste errors

Diffstats

 image.admin.inc                                    |   86 +++------------------
 image.module                                       |   17 +---
 lib/Drupal/image/ImageStyleListController.php      |   45 ++++++++++
 lib/Drupal/image/Plugin/Core/Entity/ImageStyle.php |    3 
 lib/Drupal/image/Tests/ImageAdminStylesTest.php    |   18 ++--
 5 files changed, 76 insertions(+), 93 deletions(-)
tim.plunkett’s picture

sun’s picture

Title: Convert image styles to use ConfigEntityListController » Use EntityListController for image styles

I got heavily confused by the issue titles, so let me harmonize this one with #1788542: Use EntityFormController and EntityListController for image styles

andypost’s picture

FileSize
4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 50,628 pass(es). View

Status: Needs review » Needs work
Issue tags: -Configuration system, -Configurables

The last submitted patch, 1809376-image-list-7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

#7: 1809376-image-list-7.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +Configurables

The last submitted patch, 1809376-image-list-7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

Something wrong with testbot

andypost’s picture

Now patch is green, needs review

tim.plunkett’s picture

Issue tags: +Needs screenshots

Code looks great, can we see some before and after screenshots?

andypost’s picture

FileSize
13.67 KB
12.39 KB
4.81 KB
PASSED: [[SimpleTest]]: [MySQL] 50,646 pass(es). View
611 bytes

Actually yes! This was helpful

The only difference now that edit link points to admin/config/media/image-styles/manage/%image_style/edit

andypost’s picture

Issue tags: -Needs screenshots
FileSize
12.39 KB
13.67 KB

d.o won't show this files...

Before & patch #14
before-and-14.png

After #7
after-7.png

sun’s picture

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.php

+    $row['label'] = l($entity->label(), $uri['path'], $uri['options']);

Can we use #type link here?

Aside from that, this looks ready to go.

andypost’s picture

@sun we have two different pre-render arrays for "sortable" and "renderable" tables - is there any chance|idea to unify them to have ability quickly switch between them?

andypost’s picture

andypost’s picture

Status: Postponed » Needs review
FileSize
5.43 KB
PASSED: [[SimpleTest]]: [MySQL] 53,620 pass(es). View
1.98 KB
19.16 KB

New patch, label changed to #type link as @sun pointed in #16

Also I added bold style as suggested in #1814916-65: Convert menus into entities
1809376-image-list.png

andypost’s picture

FileSize
5.49 KB
PASSED: [[SimpleTest]]: [MySQL] 54,173 pass(es). View
1.14 KB

Fixed doc blocks to use full namespace

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

#20: 1809376-image-list-20.patch queued for re-testing.

andypost’s picture

FileSize
5.49 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
1.05 KB

Reroll for {@inheritdoc}

andypost’s picture

FileSize
5.34 KB
PASSED: [[SimpleTest]]: [MySQL] 54,725 pass(es). View

previous patch was wrong

andypost’s picture

FileSize
4.75 KB
PASSED: [[SimpleTest]]: [MySQL] 54,857 pass(es). View
1.53 KB

as per #1872870-29: Implement a RoleListController and RoleFormController there should not be links because links "better use" for Content Entities

diffstat

 image.admin.inc                                    |   63 ---------------------
 image.module                                       |    3 -
 lib/Drupal/image/ImageStyleListController.php      |   48 ++++++++++++++++
 lib/Drupal/image/Plugin/Core/Entity/ImageStyle.php |    1 
 4 files changed, 51 insertions(+), 64 deletions(-)
andypost’s picture

FileSize
4.8 KB
PASSED: [[SimpleTest]]: [MySQL] 55,243 pass(es). View
andypost’s picture

Issue tags: +WSCCI-conversion
FileSize
6.19 KB
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es). View
2.75 KB

Convert to route

andypost’s picture

Status: Needs review » Needs work
Issue tags: -Configuration system, -Configurables, -WSCCI-conversion

The last submitted patch, 1809376-imageList-26.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

#26: 1809376-imageList-26.patch queued for re-testing.

kim.pepper’s picture

tstoeckler’s picture

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.php
@@ -0,0 +1,46 @@
+    $row['label'] = check_plain($entity->label());

ConfigEntityListController currently does:

$row['label'] = $entity->label();

It seems that is a XSS vulnerability. So can we fix that there instead of overriding it here?

Otherwise looks good. I didn't try this out, otherwise I would RTBC.

andypost’s picture

FileSize
729 bytes
6.29 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
andypost’s picture

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system, +Configurables, +WSCCI-conversion

The last submitted patch, 1809376-imageList-32.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
6.29 KB
PASSED: [[SimpleTest]]: [MySQL] 57,890 pass(es). View

new merge

Status: Needs review » Needs work

The last submitted patch, 1809376-imageList-36.patch, failed testing.

andypost’s picture

Priority: Normal » Major
Status: Needs work » Needs review
dawehner’s picture

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.phpundefined
@@ -0,0 +1,47 @@
+    $build['#attached']['css'][] = drupal_get_path('module', 'image') . '/css/image.admin.css';

Is there a follow up issue already to convert this to a library?

andypost’s picture

Probably there should be some meta issue to convert all #attached to library but this out of scope the issue
Also this css is not used on the list page so I reverted it back in #33 to make this conversion straight

andypost’s picture

FileSize
7.6 KB
FAILED: [[SimpleTest]]: [MySQL] 58,282 pass(es), 1 fail(s), and 0 exception(s). View
3.46 KB

Re-roll with ur_generator injected, also admin.css is not needed at all here, it used only for preview?

andypost’s picture

FileSize
7.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,247 pass(es). View
1.39 KB

Fix doc-block

Status: Needs review » Needs work
Issue tags: -Configuration system, -Configurables, -WSCCI-conversion

The last submitted patch, 1809376-imageList-42.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Configurables, +WSCCI-conversion

#42: 1809376-imageList-42.patch queued for re-testing.

tim.plunkett’s picture

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.phpundefined
@@ -0,0 +1,93 @@
+/**
+ * Contains \Drupal\image\ImageStyleListController.
+ */

Missing @file

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.phpundefined
@@ -0,0 +1,93 @@
+    $row['label'] = String::checkPlain($entity->label());

How is this different than the parent?

andypost’s picture

FileSize
7.92 KB
FAILED: [[SimpleTest]]: [MySQL] 58,209 pass(es), 1 fail(s), and 0 exception(s). View
417 bytes

How is this different than the parent?

Parent should do this check after #2019071: EntityListController::buildRow() should return secure label

Status: Needs review » Needs work

The last submitted patch, 1809376-imageList-46.patch, failed testing.

dawehner’s picture

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.phpundefined
@@ -0,0 +1,94 @@
+   *
+   * @var \Drupal\Core\Routing\UrlGenerator
...
+   * @param \Drupal\Core\Routing\UrlGenerator $url_generator
+   *   The URL generator.

Instead of type hinting the core url generator directly we should go with PathBasedGeneratorInterface

andypost’s picture

Status: Needs work » Needs review
FileSize
7.97 KB
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es). View
1.89 KB

Yep

Berdir’s picture

#49: 1809376-imageList-49.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That's perfect

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Dries’s picture

Priority: Major » Normal

This shouldn't be major. As a rule, if the blockee is normal, the blocker is not automatically major -- it should be prioritized on its own merits.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Install this, and go to /admin/config/media/image-styles.
You won't see any operations links, because there is no access controller.

We need to expand the tests to use clickLink() instead of navigating by URL, and we need an access controller.

When the tests are written, please upload the patch with those tests without the access controller to prove it fails.

speely’s picture

Status: Needs work » Needs review
FileSize
10.47 KB
PASSED: [[SimpleTest]]: [MySQL] 56,824 pass(es). View
3.18 KB

I've extended the patch in #49 by adding an appropriate ImageStyleAccessController.

In addition, I've added a test which checks whether the user with 'administer image styles' permission can access the "Edit" link controlled by the ImageStyleAccessController using the clickLink() function.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +D8MI, +sprint, +language-config, +blocker

This looks good! I know this does not use the latest in menu tab plugin technology and all that, but this also blocks adding image style support to config translation (given no alterable list operations), so this blocks #2044387: Add remaining configuration entity or page into configuration translation module which blocks #1952394: Add configuration translation user interface module in core, so it would be good to get this land.

Did I say I love we have these shared interests to have clean API implementations? :) Yay!

Gábor Hojtsy’s picture

#55: 1809376-imageList-55.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies.

fubhy’s picture

Status: Needs work » Needs review
FileSize
10.63 KB
PASSED: [[SimpleTest]]: [MySQL] 57,514 pass(es). View

Re-roll

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/lib/Drupal/image/ImageStyleAccessController.phpundefined
@@ -0,0 +1,33 @@
+        return user_access('administer image styles', $account);

This should be $account->hasPermission now.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.phpundefined
@@ -0,0 +1,94 @@
+    $row['label'] = t('Style name');

Can't we also use a translation manager?

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStyleListTest.phpundefined
@@ -0,0 +1,53 @@
+  function testImageStyleAccess() {

At least this function should have a short docblock.

fubhy’s picture

Status: Needs work » Needs review
FileSize
5.31 KB
11.28 KB
PASSED: [[SimpleTest]]: [MySQL] 57,421 pass(es). View
dawehner’s picture

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAccessController.phpundefined
@@ -0,0 +1,33 @@
+  }
+}

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStyleListTest.phpundefined
@@ -0,0 +1,56 @@
+  }
+}

/me hides ... missing empty line.

tim.plunkett’s picture

Status: Needs review » Needs work
claudiu.cristea’s picture

This interfere with #1788542: Use EntityFormController and EntityListController for image styles on local actions. Let me merge this into #1788542: Use EntityFormController and EntityListController for image styles as it easier to handle both in the same patch.

claudiu.cristea’s picture

Status: Needs work » Closed (duplicate)

I merged and reworked this patch in #1788542-58: Use EntityFormController and EntityListController for image styles because they interfere in a way that makes difficult separate patching.

Marking this as duplicate of #1788542: Use EntityFormController and EntityListController for image styles.

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing tag.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.