Files: 
CommentFileSizeAuthor
#84 1788542-image-form-84.patch36.43 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,188 pass(es).
[ View ]
#84 interdiff.txt29.88 KBclaudiu.cristea
#80 1788542-image-form-80.patch36.42 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,556 pass(es).
[ View ]
#80 interdiff.txt23.6 KBclaudiu.cristea
#78 1788542-image-form-78.patch37.81 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,490 pass(es).
[ View ]
#78 interdiff.txt718 bytesclaudiu.cristea
#73 1788542-image-form-73.patch37.81 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,184 pass(es).
[ View ]
#73 interdiff.txt3.36 KBclaudiu.cristea
#72 1788542-72.patch36.23 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,488 pass(es).
[ View ]
#72 interdiff.txt3.73 KBfubhy
#71 1788542-image-form-71.patch37.73 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,171 pass(es).
[ View ]
#71 interdiff.txt2.93 KBclaudiu.cristea
#70 1788542-image-form-70.patch37.23 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,492 pass(es).
[ View ]
#70 interdiff.txt539 bytesclaudiu.cristea
#69 1788542-image-form-69.patch37.08 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#69 interdiff.txt1.32 KBclaudiu.cristea
#67 1788542-image-form-67.patch37.02 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#67 interdiff.txt18.58 KBclaudiu.cristea
#63 interdiff.txt1.71 KBclaudiu.cristea
#62 1788542-image-form-62.patch33.25 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 57,172 pass(es).
[ View ]
#62 interdiff.txt1.71 KBclaudiu.cristea
#58 action-label.jpg145.79 KBclaudiu.cristea
#58 1788542-image-form-58.patch33.19 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#52 interdiff.txt971 bytesandypost
#52 1788542-image-form-52.patch20.98 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,998 pass(es).
[ View ]
#51 interdiff.txt470 bytesandypost
#51 1788542-image-form-50.patch20.91 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,598 pass(es).
[ View ]
#51 interdiff-todo.txt978 bytesandypost
#47 interdiff.txt14.32 KBandypost
#47 1788542-image-form-47.patch20.91 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 57,646 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#45 1788542-image-form-45.patch19.26 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,630 pass(es).
[ View ]
#35 interdiff.txt760 bytesandypost
#35 1788542-image-form-35.patch19.26 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1788542-image-form-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 interdiff.txt5.38 KBandypost
#34 1788542-image-form-34.patch19.06 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,706 pass(es).
[ View ]
#33 1788542-image-form-33.patch18.54 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,347 pass(es).
[ View ]
#32 interdiff.txt4.82 KBandypost
#32 1788542-image-form-32.patch18.48 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,697 pass(es).
[ View ]
#27 1788542-image-style-entity-form-controller.patch18.14 KBtim-e
PASSED: [[SimpleTest]]: [MySQL] 53,593 pass(es).
[ View ]
#25 1788542-image-style-entity-form-controller.patch18.18 KBtim-e
PASSED: [[SimpleTest]]: [MySQL] 52,201 pass(es).
[ View ]
#22 1788542-image-style-entity-form-controller.patch24.17 KBtim-e
FAILED: [[SimpleTest]]: [MySQL] 49,867 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#20 1788542-image-style-entity-form-controller.patch18.08 KBtim-e
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1788542-image-style-entity-form-controller_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 1788542-image-style-entity-form-controller.patch18.6 KBtim-e
FAILED: [[SimpleTest]]: [MySQL] 49,158 pass(es), 37 fail(s), and 15 exception(s).
[ View ]
#11 1788542-image-style-entity-form-controller.patch5.08 KBtim-e
PASSED: [[SimpleTest]]: [MySQL] 50,802 pass(es).
[ View ]

Comments

Issue tags:+#pnx-sprint

tagging

Status:Postponed» Closed (won't fix)

I'm just going to roll this into #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity). It doesn't make sense to have two separate issues.

Status:Closed (won't fix)» Closed (duplicate)

Right status

Status:Closed (duplicate)» Postponed

According to the discussion at #1588422-42: Convert contact categories to configuration system we are actually tackling these in separate issues, so reopening this.

Issue summary:View changes

Updated issue summary.

there's a big changes in hook_menu for image module in related #1809376: Use EntityListController for image styles

So I'd recommend to follow current core's pattern entity-type/manage/%id/action for menu

Assigned:Unassigned» tim-e

Status:Active» Needs review
StatusFileSize
new5.08 KB
PASSED: [[SimpleTest]]: [MySQL] 50,802 pass(es).
[ View ]

Ive had a run at moving the create new style form to entity form controller.

In discussion with larowlan and heyrocker, agreed just to do that one form as the effects will be converted to plugins as per http://drupal.org/node/1821854

Hm. What about the edit form though?

Awesome start!

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,75 @@
+      '#default_value' => '',
...
+      '#default_value' => '',

You can use $entity = $this->getEntity(); and then call $entity->id()/label() here

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,75 @@
+      '#required' => TRUE,

This should probably have '#disabled' => !$entity->isNew(),

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,75 @@
+    $style = entity_create('image_style', array(
+      'name' => $form_state['values']['name'],
+      'label' => $form_state['values']['label'],

You should be able to use $this->getEntity() here

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,75 @@
+    return array(
+      'submit' => array(
+        '#value' => t('Create new style'),
+        '#validate' => array(
+          array($this, 'validate'),
+        ),
+        '#submit' => array(
+          array($this, 'save'),
+        ),
+      ),

You should call parent::actions() and just override the #value of the button name, since you definitely want the EntityFormController::submit() function to run.

StatusFileSize
new18.6 KB
FAILED: [[SimpleTest]]: [MySQL] 49,158 pass(es), 37 fail(s), and 15 exception(s).
[ View ]

Attached is a new patch taking into consideration the above feedback and implementing the edit form as well.

I would like to get some feedback on this as it may need some work.

Also, I'm not quite sure what to do about the Delete form. Should I be trying to make the form() method handle all three operations add/edit/delete? In this patch it only handles add/edit and the delete form is still using drupal_get_form. I was thinking pass along the url parameter add/edit/delete as an $operation variable to the form() method and could base it on that. But maybe there is a better way?

None of the conversions have dealt with delete, mostly because of the confirm form. So I wouldn't worry about converting that here either.

I didn't have a chance to review this yet, but thanks tim-e for keeping this moving!

Status:Needs review» Needs work

The last submitted patch, 1788542-image-style-entity-form-controller.patch, failed testing.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.php
@@ -0,0 +1,247 @@
+    $form['label'] = array(
...
+      '#weight' => 2
...
+    $form['name'] = array(
...
+      $form['preview'] = array(
...
+        '#weight' => 1
...
+      $form['effects'] = array(
+        '#theme' => 'image_style_effects',
+        '#weight' => 3

weights needs bigger step and should apply to all elements

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.php
@@ -0,0 +1,247 @@
+      if (!empty($style->effects)) {
+        foreach ($style->effects as $key => $effect) {
+          $form['effects'][$key]['#weight'] = isset($form_state['input']['effects']) ? $form_state['input']['effects'][$key]['weight'] : NULL;
+          $form['effects'][$key]['label'] = array(
+            '#markup' => check_plain($effect['label']),

Suppose this require follow-up issue to convert this theme function to new #table element

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.php
@@ -0,0 +1,247 @@
+  public function save(array $form, array &$form_state) {
+
+    $style = $this->getEntity($form_state);
...
+    $style->set('name', $form_state['values']['name']);
+    $style->set('label', $form_state['values']['label']);
+    $status = $style->save();

set() is not needed here

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.php
@@ -0,0 +1,247 @@
+      if (isset($effect['form callback'])) {
+        $path = 'admin/config/media/image-styles/manage/' . $style->id() . '/add/' . $form_state['values']['new'];
+        $form_state['redirect'] = array($path, array('query' => array('weight' => $form_state['values']['weight'])));
...
+    if ($status == SAVED_UPDATED) {
+      drupal_set_message(t('Changes to the style have been saved.'));
+    }
+    $form_state['redirect'] = 'admin/config/media/image-styles/manage/' . $style->id();

last redirect will override one for effects

We should always be using Entity::set() for that, even public properties. So keep that part. The other feedback looks valid.

@tim I mean that values are already set in parent::submit() so $this->getEntity($form_state); will return a form values and all you need just to call $entity->save()

StatusFileSize
new18.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1788542-image-style-entity-form-controller_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Have had another run at this. May still need some work but should resolve the above feedback.

Ignore the last patch, will fix and repost

Status:Needs work» Needs review
StatusFileSize
new24.17 KB
FAILED: [[SimpleTest]]: [MySQL] 49,867 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Trying again

Status:Needs review» Needs work

The last submitted patch, 1788542-image-style-entity-form-controller.patch, failed testing.

+++ b/core/modules/image/image.admin.incundefined
@@ -24,239 +24,21 @@ function image_style_list() {
+  ¶
+  return entity_get_form($image_style);
+++ b/core/modules/image/image.moduleundefined
@@ -348,13 +346,17 @@ function image_file_predelete(File $file) {
function image_image_style_update(ImageStyle $style) {
+ ¶
   if ($style->id() != $style->getOriginalID()) {
+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,240 @@
+  public function form(array $form, array &$form_state, EntityInterface $style) {
+    ¶
+    $form = parent::form($form, $form_state, $style);
...
+          $form['effects'][$key]['name'] = array(
+            '#type' => 'value', ¶
+            '#value' => $effect['name']
+          );
+          $form['effects'][$key]['data'] = array(
+            '#type' => 'value', ¶
+            '#value' => $effect['data']
...
+  public function effectSave($form, &$form_state) {
+    ¶
+    $style = $this->getEntity($form_state);
...
+    }
+    ¶
+    // Check if this field has any configuration options.
...
+    }
+    ¶
...
+    $style->set('id', $form_state['values']['name']);
+    ¶
+    $status = $style->save();

Trailing whitespace.

+++ b/core/modules/image/image.moduleundefined
@@ -580,7 +582,7 @@ function image_style_options($include_empty = TRUE) {
- * Page callback: Generates a derivative, given a style and image path.
+ * Menu callback; Given a style and image path, generate a derivative.
@@ -746,7 +748,7 @@ function image_style_transform_dimensions($style_name, array &$dimensions) {
- * Flushes cached media for a style.
+ * Flush cached media for a style.
@@ -772,7 +774,7 @@ function image_style_flush($style) {
- * Returns the URL for an image derivative given a style and image path.
+ * Return the URL for an image derivative given a style and image path.
@@ -799,7 +801,7 @@ function image_style_url($style_name, $path) {
- * Returns the URI of an image when using a style.
+ * Return the URI of an image when using a style.
@@ -825,10 +827,7 @@ function image_style_path($style_name, $uri) {
- * Returns a set of image effects.
- *
- * These image effects are exposed by modules implementing
- * hook_image_effect_info().
+ * Pull in image effects exposed by modules implementing hook_image_effect_info().
@@ -870,7 +869,7 @@ function image_effect_definitions() {
- * Loads the definition for an image effect.
+ * Load the definition for an image effect.
@@ -897,7 +896,7 @@ function image_effect_definition_load($effect) {
- * Loads a single image effect.
+ * Load a single image effect.
@@ -934,7 +933,7 @@ function image_effect_load($ieid, $style_name) {
- * Saves an image effect.
+ * Save an image effect.
@@ -963,7 +962,7 @@ function image_effect_save($style, &$effect) {
- * Deletes an image effect.
+ * Delete an image effect.
@@ -977,7 +976,7 @@ function image_effect_delete($style, $effect) {
- * Applies an image effect to the image object.
+ * Given an image object and effect, perform the effect on the file.
@@ -1033,7 +1032,7 @@ function theme_image_style($variables) {
- * Accepts a keyword (center, top, left, etc) and returns it as a pixel offset.
+ * Accept a keyword (center, top, left, etc) and return it as a pixel offset.

These are all opposite changes. Should be reverted.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,240 @@
+ * Definition of Drupal\image\ImageStyleFormController.

Contains \Drupal\...

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,240 @@
+  public function form(array $form, array &$form_state, EntityInterface $style) {
+    ¶
+    $form = parent::form($form, $form_state, $style);
...
+  public function effectSave($form, &$form_state) {
+    ¶
+    $style = $this->getEntity($form_state);
...
+  public function save(array $form, array &$form_state) {
+
+    $style = $this->getEntity($form_state);
...
+    $form_state['redirect'] = 'admin/config/media/image-styles/manage/' . $style->id();
+
...
+  public function actions(array $form, array &$form_state) {
+
+    $actions = parent::actions($form, $form_state);
...
+    return $actions;
+

Extra blank line

-----

I couldn't spot any reason for the failures, but if the rest of this is cleaned up first, I'll make another pass at it.

Status:Needs work» Needs review
StatusFileSize
new18.18 KB
PASSED: [[SimpleTest]]: [MySQL] 52,201 pass(es).
[ View ]

Another run at this. All tests green for me locally.

This is looking awesome.

Some minor nitpicks.

+++ b/core/modules/image/image.moduleundefined
@@ -126,8 +126,8 @@ function image_menu() {
+    'page arguments' => array(5),

There is no 5th argument here, the callback has default to NULL so you don't need this line.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,241 @@
+ * Definition of Drupal\image\ImageStyleFormController.

New standard is \Drupal

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,241 @@
+   * Overrides Drupal\Core\Entity\EntityFormController::form().

Same

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,241 @@
+

The extra blank lines from comment #24 are still present.

Leaving at needs review for others.

StatusFileSize
new18.14 KB
PASSED: [[SimpleTest]]: [MySQL] 53,593 pass(es).
[ View ]

Ok fixed up those issues as noted by larowlan

Status:Needs review» Active

Converting to controller/route

Status:Active» Needs review

@tim-e I think route conversion should be done in follow-up

+++ b/core/modules/image/image.admin.incundefined
@@ -24,239 +24,18 @@ function image_style_list() {
+ * @param optional $image_style

optional is not a data type

+++ b/core/modules/image/image.admin.incundefined
@@ -24,239 +24,18 @@ function image_style_list() {
+function image_style_form($image_style = NULL) {

_form is a bad suffix for page callback

+++ b/core/modules/image/image.moduleundefined
@@ -138,8 +137,8 @@ function image_menu() {
     'title callback' => 'entity_page_label',
...
+    'page callback' => 'image_style_form',
+    'page arguments' => array(5),

Please make different callback for edit page to make proper page title. see contact module as example

StatusFileSize
new18.48 KB
PASSED: [[SimpleTest]]: [MySQL] 54,697 pass(es).
[ View ]
new4.82 KB

Adds more fixes

StatusFileSize
new18.54 KB
PASSED: [[SimpleTest]]: [MySQL] 55,347 pass(es).
[ View ]

Issue tags:+WSCCI-conversion
StatusFileSize
new19.06 KB
PASSED: [[SimpleTest]]: [MySQL] 55,706 pass(es).
[ View ]
new5.38 KB

Re-roll with routes and new EntityFormController

StatusFileSize
new19.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1788542-image-form-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new760 bytes

revert drupal_set_title()

EDIT or we need to use $this->operation like #1938386-23: Convert contact_category_* pages to a new-style Controller does

Status:Needs review» Needs work
Issue tags:-D8MI, -#pnx-sprint, -WSCCI-conversion

The last submitted patch, 1788542-image-form-35.patch, failed testing.

Status:Needs work» Needs review

#35: 1788542-image-form-35.patch queued for re-testing.

#35: 1788542-image-form-35.patch queued for re-testing.

Assigned:tim-e» Unassigned
Status:Needs review» Reviewed & tested by the community

#35 looks good for me
happy pushing

Issue tags:+CodeSprintUA

tagging

#35: 1788542-image-form-35.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+D8MI, +#pnx-sprint, +WSCCI-conversion, +CodeSprintUA

The last submitted patch, 1788542-image-form-35.patch, failed testing.

I'm also not sure about using the same form controller for create and edit as the forms are so different.

Please don't reuse the same operation.
See #2006348: Remove default/fallback entity form operation
At the least it should be

*     "form" = {
*       "add" = "Drupal\image\ImageStyleFormController",
*       "edit" = "Drupal\image\ImageStyleFormController"
*     }

But ideally they'd be different classes, maybe with a base class.

Status:Needs work» Needs review
StatusFileSize
new19.26 KB
PASSED: [[SimpleTest]]: [MySQL] 57,630 pass(es).
[ View ]

just a re-roll

Status:Needs review» Needs work

The last submitted patch, 1788542-image-form-45.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.91 KB
FAILED: [[SimpleTest]]: [MySQL] 57,646 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new14.32 KB

New patch with separated forms (as it was)

Status:Needs review» Needs work
Issue tags:-D8MI, -#pnx-sprint, -WSCCI-conversion, -CodeSprintUA

The last submitted patch, 1788542-image-form-47.patch, failed testing.

Status:Needs work» Needs review

#47: 1788542-image-form-47.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+D8MI, +#pnx-sprint, +WSCCI-conversion, +CodeSprintUA

The last submitted patch, 1788542-image-form-47.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new978 bytes
new20.91 KB
PASSED: [[SimpleTest]]: [MySQL] 57,598 pass(es).
[ View ]
new470 bytes

description is not used for action links

List controller should be commited first #1809376: Use EntityListController for image styles
then interdiff-todo.txtshould be applied for #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()

EDIT test passes locally

StatusFileSize
new20.98 KB
PASSED: [[SimpleTest]]: [MySQL] 56,998 pass(es).
[ View ]
new971 bytes

So finally should be fine

Issue tags:+MENU_LOCAL_ACTION

Blocks removal of MENU_LOCAL_ACTION

Status:Needs review» Needs work

Title:Use EntityFormController for image stylesUse EntityFormController and EntityListController for image styles
Status:Needs work» Needs review
StatusFileSize
new33.19 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new145.79 KB

#1809376: Use EntityListController for image styles was merged here because they interfere in a way that make difficult separate patching.

Here's a reworked patch, including #1809376: Use EntityListController for image styles.

KNOWN BUG: The label/title of the local action Add image style is missed from markup. I followed exactly https://drupal.org/node/2007444 for conversion. Here I need some help.

action-label.jpg

An interdiff.txt was impossible to provide. Image tests ran successfully local.

+++ b/core/modules/image/lib/Drupal/image/Plugin/Menu/ImageStyleAddLocalAction.phpundefined
@@ -0,0 +1,24 @@
+ * Contains \Drupal\image\Plugin\Menu\ImageStyleAddLocalAction.
...
+namespace Drupal\image\Plugin\Menu;

This is one directory too low. Should be namespace Drupal\image\Plugin\Menu\LocalAction;

Status:Needs review» Needs work

The last submitted patch, 1788542-image-form-58.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.71 KB
new33.25 KB
PASSED: [[SimpleTest]]: [MySQL] 57,172 pass(es).
[ View ]

StatusFileSize
new1.71 KB

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAccessController.phpundefined
@@ -0,0 +1,33 @@
+use \Drupal\Core\Entity\EntityAccessController;
+use \Drupal\Core\Entity\EntityInterface;
+use \Drupal\Core\Session\AccountInterface;

No need for the initial backslash.

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

There should be an empty line before the end.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,66 @@
+        'exists' => 'image_style_load',

Theoretical you can use array($storage_controller, 'load')

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

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

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,237 @@
+    form_load_include($form_state, 'inc', 'image', 'image.admin');

Are you sure we need this? theme itself should load the file on the fly.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,237 @@
+    $effects = \Drupal::service('plugin.manager.image.effect')->getDefinitions();
...
+    $effect = \Drupal::service('plugin.manager.image.effect')->getDefinition($form_state['values']['new']);

It would be nice to inject this service ... instead

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,237 @@
+        drupal_set_message(t('The image effect was successfully applied.'));

t() should be also injected

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,237 @@
+       ¶
...
+  protected function updateEffectWeights(ImageStyleInterface $style, array $effects) {    ¶

Unnecessary whitespace left.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.phpundefined
@@ -0,0 +1,106 @@
+use Drupal\Core\Config\Entity\ConfigEntityListController;
+use Drupal\Core\Entity\EntityControllerInterface;
+use Drupal\Core\Routing\PathBasedGeneratorInterface;
+use Drupal\Core\StringTranslation\TranslationManager;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Entity\EntityStorageControllerInterface;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\Entity\EntityInterface;

Let's put symfony to the bottom, as it looks nicer.

Due to the merge with #1809376: Use EntityListController for image styles this patch should also give commit credit for "andypost, fubhy, speely | heyrocker".

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary changes:

StatusFileSize
new18.58 KB
new37.02 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here we go...

Status:Needs review» Needs work

The last submitted patch, 1788542-image-form-67.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
new37.08 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

This passed Image tests locally.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new539 bytes
new37.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,492 pass(es).
[ View ]

More cleanup.

StatusFileSize
new2.93 KB
new37.73 KB
PASSED: [[SimpleTest]]: [MySQL] 57,171 pass(es).
[ View ]

Added call to controller's method instead procedural call to image_style_load in $form['name']['#machine_name']['exists'] in ImageStyleAddFormController too.

StatusFileSize
new3.73 KB
new36.23 KB
PASSED: [[SimpleTest]]: [MySQL] 57,488 pass(es).
[ View ]

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,114 @@
+      $container->get('plugin.manager.entity'),
...
+    $storage_controller = $this->entityManager->getStorageController('image_style');

You are only ever using the storage controller. So let's just inject the storage controller, not the entire entity manager.

Looks good otherwise.

StatusFileSize
new3.36 KB
new37.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,184 pass(es).
[ View ]

Thank you!

OK, then let's change ImageStyleAddFormController too :)

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -20,11 +20,11 @@
+  protected $storageController;
@@ -38,14 +38,14 @@ class ImageStyleAddFormController extends EntityFormController implements Entity
+    $this->storageController = $storage_controller;

Suppose imageStyleStorageController is better. We have this all over core

From IRC:

[2:43pm] claudiu_cristea: andypost: You're talking about variable name or type hinting in https://drupal.org/node/1788542#comment-7684193 ?
[2:45pm] claudiu_cristea: andypost: or both
...
[2:46pm] andypost: claudiu_cristea, yes, variable name + probably imageStyleStorageControllerInterface for type
[2:51pm] claudiu_cristea: andypost: type hint is OK
[2:52pm] claudiu_cristea: ImageStyle storage controller is \Drupal\Core\Config\Entity\ConfigStorageController
[2:52pm] claudiu_cristea: andypost: that extends EntityStorageControllerBase which implements EntityStorageControllerInterface
[2:53pm] claudiu_cristea: andypost: so, I won't change the name as it reflects the interface
[2:53pm] andypost: claudiu_cristea, sure, just checked that we have no interface definition for storage controller, that needs follow-up

Yeah! It's green. Any other review or rtbc?

Status:Needs review» Reviewed & tested by the community

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStyleListTest.phpundefined
@@ -0,0 +1,57 @@
+ * Contains Drupal\image\Tests\ImageStyleListTest.

Misses leading backslash.

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStyleListTest.phpundefined
@@ -0,0 +1,57 @@
+ * Tests access to overview page and operation links

Misses period.

But that's really it. Just read the code twice.

StatusFileSize
new718 bytes
new37.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,490 pass(es).
[ View ]

OK, thank you. It should pass -- only docs.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,113 @@
+class ImageStyleAddFormController extends EntityFormController implements EntityControllerInterface {
+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,296 @@
+ * Base form controller for image style edit forms.
...
+class ImageStyleFormController extends EntityFormController implements EntityControllerInterface {

There SHOULD be an abstract class ImageStyleFormBase that both the add and edit extend. There is a lot of code duplication here.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,113 @@
+  /**
+   * The storage controller.
+   *
+   * @var \Drupal\Core\Entity\EntityStorageControllerInterface
+   */
+  protected $storageController;
...
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $storage_controller
+   *   The storage controller.
...
+      $container->get('plugin.manager.entity')->getStorageController($entity_type),

Because of this trickiness using $entity_type in createInstance(), you have absolutely NO idea what storage controller this is.

Please rename $storageController to $imageStyleStorage, and $storage_controller $image_style_storage

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,113 @@
+    $this->moduleHandler = $module_handler;
...
+    parent::__construct($this->moduleHandler);

Just pass $module_handler to parent::__construct() first, no need to do the assignment ourselves.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,113 @@
+    $form = parent::form($form, $form_state);
...
+    return $form;

This needs to be return parent::form($form, $form_state); at the end, stuff in the parent expects to run later

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,113 @@
+      '#weight' => 20,
...
+      '#weight' => 30,

This should be removed, it wasn't in the original code.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,113 @@
+        'exists' => array($this->storageController, 'load'),

I guess this works, I haven't seen it because we didn't have singular load() available before. But most other forms use entityQuery for this since we don't need it loaded

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -0,0 +1,113 @@
+    $uri = $this->entity->uri();
+    watchdog('image', 'Style %name was created.', array('%name' => $this->entity->label()), WATCHDOG_NOTICE, l($this->translator->translate('Edit'), $uri['path']));
+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,296 @@
+    $uri = $this->entity->uri();
+    watchdog('image', 'Style %label has been updated.', array('%label' => $this->entity->label()), WATCHDOG_NOTICE, l($this->translator->translate('Edit'), $uri['path']));

This should be removed, there is no need to add watchdog calls.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,296 @@
+  protected $storageController;

A lot of my comments about the previous class apply to this as well.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,296 @@
+    $style = $this->entity;

Please don't introduce this pattern. Use $this->entity directly.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,296 @@
+    $style = $this->entity;

Don't bother, just use $this->entity please

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -0,0 +1,296 @@
+   * Update image effect weights.

Updates

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.phpundefined
@@ -0,0 +1,106 @@
+   *   The entity storage controller class.

Say which entity

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

What's this for? The parent does this.

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStyleListTest.phpundefined
@@ -0,0 +1,57 @@
+class ImageStyleListTest extends WebTestBase {

Isn't there an existing web test this could go in?

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStyleListTest.phpundefined
@@ -0,0 +1,57 @@
+   * A user with the permission to administer image styles.
+   *
+   * @var \Drupal\user\UserInterface
+   */
+  protected $adminUser;
...
+    $this->adminUser = $this->drupalCreateUser(array('administer image styles'));
+    $this->drupalLogin($this->adminUser);

This can be a oneliner $this->drupalLogin($this->drupalCreateUser(, no need for a property

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

public

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageStyleListTest.phpundefined
@@ -0,0 +1,57 @@
+    $this->drupalGet('admin/config/media/image-styles');
+    $this->clickLink(t('Edit'));

Can you assert that the correct page loads?

Status:Needs work» Needs review
StatusFileSize
new23.6 KB
new36.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,556 pass(es).
[ View ]

Voilà!

Wow @claudiu.cristea, that was fast.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleAddFormController.phpundefined
@@ -7,95 +7,19 @@
+class ImageStyleAddFormController extends ImageStyleFormBase {

You can drop the Controller suffix if you'd like, to match the base class.

+++ b/core/modules/image/lib/Drupal/image/ImageStyleFormController.phpundefined
@@ -7,27 +7,18 @@
+class ImageStyleFormController extends ImageStyleFormBase {

This should be ImageStyleEditForm

+++ b/core/modules/image/lib/Drupal/image/ImageStyleListController.phpundefined
@@ -88,7 +87,7 @@ public function buildHeader() {
-    $row['label'] = String::checkPlain($entity->label());
+    $row['label'] = $entity->label();

I meant, why are you overriding $row['label'] at all? Just delete this line.

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.phpundefined
@@ -390,4 +390,16 @@ function testConfigImport() {
+  function testImageStyleAccess() {

public function test

Yes, I was thinking to rename them. Woudn't be a good idea to rename an also move them in the Form subdirectory?

There is an existing Form subdir holding delete form.

Yes please

StatusFileSize
new29.88 KB
new36.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,188 pass(es).
[ View ]

OK.

Status:Needs review» Needs work

The last submitted patch, 1788542-image-form-84.patch, failed testing.

Hmm... Drupal\system\Tests\Module\ModuleApiTest is passing locally with latest code pulled.

Status:Needs work» Needs review

#84: 1788542-image-form-84.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Looks good to me, thanks @claudiu.cristea!

Issue tags:-#pnx-sprint, -CodeSprintUA

Remove some tags.

Status:Reviewed & tested by the community» Fixed
StatusFileSize
new49.54 KB
new57.71 KB

Patch works fine but image effect ordering is displaying some interesting behaviour in HEAD that this patch neither fixes or changes, see #2052787: Image style effect ordering exhibits some odd behaviour.

Committed 5f317ad and pushed to 8.x. Thanks!

Yay, this will let image effect translatability be exposed on the UI! Woot!

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

Issue summary:View changes

Updated issue summary.

In #79, @tim.plunkett stated:

+    watchdog('image', 'Style %label has been updated.', array('%label' => $this->entity->label()), WATCHDOG_NOTICE, l($this->translator->translate('Edit'), $uri['path']));

This should be removed, there is no need to add watchdog calls.

I'd like to learn and understand why that advice was given — are user operations logged differently in D8 now?

Drupal did log a message for each (form/UI driven) entity operation in the past, at minimum for create/update/delete operations.

Not limited to administrative configuration changes, but in particular for those, the logging is related to security (security audits).

In fact, Drupal did not always log all user operations in the past — typically, modules simply forgot to implement it, causing an incomplete picture in security [breach] audits.

Unless the logging is (centrally?) handled elsewhere now, this removal appears to be a step backwards and a regression in terms of security to me.

A clarification would be highly appreciated. :)

Unless the logging is (centrally?) handled elsewhere now, this removal appears to be a step backwards and a regression in terms of security to me.

Yeah, I can't see any central handling of this either, but perhaps we should open an issue to add it?
Having it handled centrally would harden contrib modules too, one less thing for them to think about.