Use EntityListController for image styles

#1588422: Convert contact categories to configuration system commited

This was fixed as follow-up #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity)

Remaining todo #1821848: Move image style load/update/delete operations into a new ImageStyleStorageController

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

We need to re-manage a style uri anyway

Updated issue summary.

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

See also: #1885644: ImageStyle::uri() is broken and has no coverage

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

Re-roll after #1885644: ImageStyle::uri() is broken and has no coverage

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

Something wrong with testbot

Now patch is green, needs review

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

Actually yes! This was helpful

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

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

Before & patch #14

After #7

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

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

We have to fix #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) first

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

Fixed doc blocks to use full namespace

Updated issue summary.

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

Reroll for {@inheritdoc}

previous patch was wrong

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

Re-roll after #1807042: Reorganizie entity storage/list/form/render controller annotation keys

Convert to route

Marked as duplicate #1987724: Convert image_style_list() to a new style controller

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

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

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

Filed #2019071: EntityListController::buildRow() should return secure label cos it out of scope

This patch holds #1788542-51: Use EntityFormController and EntityListController for image styles to introduce action links

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

new merge

Making major because of dependent #1788542-51: Use EntityFormController and EntityListController for image styles

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

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

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

Fix doc-block

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

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

How is this different than the parent?

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

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

Yep

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

That's perfect

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

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.

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.

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.

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!

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

No longer applies.

Re-roll

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

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

#1821854: Convert image effects into plugins broke this.

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.

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.

Removing tag.

Updated issue summary.