CommentFileSizeAuthor
#90 Screenshot_29_07_2013_14_28.png57.71 KBalexpott
#90 Screenshot_29_07_2013_14_27.png49.54 KBalexpott
#84 1788542-image-form-84.patch36.43 KBclaudiu.cristea
#84 interdiff.txt29.88 KBclaudiu.cristea
#80 1788542-image-form-80.patch36.42 KBclaudiu.cristea
#80 interdiff.txt23.6 KBclaudiu.cristea
#78 1788542-image-form-78.patch37.81 KBclaudiu.cristea
#78 interdiff.txt718 bytesclaudiu.cristea
#73 1788542-image-form-73.patch37.81 KBclaudiu.cristea
#73 interdiff.txt3.36 KBclaudiu.cristea
#72 1788542-72.patch36.23 KBfubhy
#72 interdiff.txt3.73 KBfubhy
#71 1788542-image-form-71.patch37.73 KBclaudiu.cristea
#71 interdiff.txt2.93 KBclaudiu.cristea
#70 1788542-image-form-70.patch37.23 KBclaudiu.cristea
#70 interdiff.txt539 bytesclaudiu.cristea
#69 1788542-image-form-69.patch37.08 KBclaudiu.cristea
#69 interdiff.txt1.32 KBclaudiu.cristea
#67 1788542-image-form-67.patch37.02 KBclaudiu.cristea
#67 interdiff.txt18.58 KBclaudiu.cristea
#63 interdiff.txt1.71 KBclaudiu.cristea
#62 1788542-image-form-62.patch33.25 KBclaudiu.cristea
#62 interdiff.txt1.71 KBclaudiu.cristea
#58 action-label.jpg145.79 KBclaudiu.cristea
#58 1788542-image-form-58.patch33.19 KBclaudiu.cristea
#52 interdiff.txt971 bytesandypost
#52 1788542-image-form-52.patch20.98 KBandypost
#51 interdiff.txt470 bytesandypost
#51 1788542-image-form-50.patch20.91 KBandypost
#51 interdiff-todo.txt978 bytesandypost
#47 interdiff.txt14.32 KBandypost
#47 1788542-image-form-47.patch20.91 KBandypost
#45 1788542-image-form-45.patch19.26 KBandypost
#35 interdiff.txt760 bytesandypost
#35 1788542-image-form-35.patch19.26 KBandypost
#34 interdiff.txt5.38 KBandypost
#34 1788542-image-form-34.patch19.06 KBandypost
#33 1788542-image-form-33.patch18.54 KBandypost
#32 interdiff.txt4.82 KBandypost
#32 1788542-image-form-32.patch18.48 KBandypost
#27 1788542-image-style-entity-form-controller.patch18.14 KBtim-e
#25 1788542-image-style-entity-form-controller.patch18.18 KBtim-e
#22 1788542-image-style-entity-form-controller.patch24.17 KBtim-e
#20 1788542-image-style-entity-form-controller.patch18.08 KBtim-e
#14 1788542-image-style-entity-form-controller.patch18.6 KBtim-e
#11 1788542-image-style-entity-form-controller.patch5.08 KBtim-e
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan’s picture

Issue tags: +#pnx-sprint

tagging

gdd’s picture

gdd’s picture

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.

andypost’s picture

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

Right status

gdd’s picture

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.

andypost’s picture

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

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

tim.plunkett’s picture

tim-e’s picture

Assigned: Unassigned » tim-e
tim-e’s picture

Status: Active » Needs review
FileSize
5.08 KB

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

sun’s picture

Hm. What about the edit form though?

tim.plunkett’s picture

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.

tim-e’s picture

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?

tim.plunkett’s picture

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.

andypost’s picture

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

tim.plunkett’s picture

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

andypost’s picture

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

tim-e’s picture

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

tim-e’s picture

Ignore the last patch, will fix and repost

tim-e’s picture

Status: Needs work » Needs review
FileSize
24.17 KB

Trying again

Status: Needs review » Needs work

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

tim.plunkett’s picture

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

tim-e’s picture

Status: Needs work » Needs review
FileSize
18.18 KB

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

larowlan’s picture

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.

tim-e’s picture

Ok fixed up those issues as noted by larowlan

tim-e’s picture

Status: Needs review » Active

Converting to controller/route

andypost’s picture

Status: Active » Needs review
andypost’s picture

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

andypost’s picture

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

andypost’s picture

FileSize
18.48 KB
4.82 KB

Adds more fixes

andypost’s picture

andypost’s picture

Issue tags: +WSCCI-conversion
FileSize
19.06 KB
5.38 KB

Re-roll with routes and new EntityFormController

andypost’s picture

FileSize
19.26 KB
760 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.

andypost’s picture

Status: Needs work » Needs review

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

kim.pepper’s picture

Issue tags: +D8MI, +#pnx-sprint, +WSCCI-conversion

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

podarok’s picture

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

#35 looks good for me
happy pushing

podarok’s picture

Issue tags: +CodeSprintUA

tagging

catch’s picture

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

alexpott’s picture

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

tim.plunkett’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
19.26 KB

just a re-roll

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
20.91 KB
14.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.

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
978 bytes
20.91 KB
470 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

andypost’s picture

FileSize
20.98 KB
971 bytes

So finally should be fine

tim.plunkett’s picture

Issue tags: +MENU_LOCAL_ACTION

Blocks removal of MENU_LOCAL_ACTION

dawehner’s picture

andypost’s picture

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
claudiu.cristea’s picture

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

Title: Use EntityFormController for image styles » Use EntityFormController and EntityListController for image styles
Status: Needs work » Needs review
FileSize
33.19 KB
145.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.

tim.plunkett’s picture

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

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
33.25 KB
claudiu.cristea’s picture

FileSize
1.71 KB
dawehner’s picture

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

fubhy’s picture

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

fubhy’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary changes:

claudiu.cristea’s picture

FileSize
18.58 KB
37.02 KB

Here we go...

Status: Needs review » Needs work

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

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
37.08 KB

This passed Image tests locally.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

FileSize
539 bytes
37.23 KB

More cleanup.

claudiu.cristea’s picture

FileSize
2.93 KB
37.73 KB

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

fubhy’s picture

FileSize
3.73 KB
36.23 KB
+++ 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.

claudiu.cristea’s picture

FileSize
3.36 KB
37.81 KB

Thank you!

OK, then let's change ImageStyleAddFormController too :)

andypost’s picture

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

claudiu.cristea’s picture

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

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

fubhy’s picture

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.

claudiu.cristea’s picture

FileSize
718 bytes
37.81 KB

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

tim.plunkett’s picture

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?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
23.6 KB
36.42 KB

Voilà!

tim.plunkett’s picture

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

claudiu.cristea’s picture

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.

tim.plunkett’s picture

Yes please

claudiu.cristea’s picture

FileSize
29.88 KB
36.43 KB

OK.

Status: Needs review » Needs work

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

claudiu.cristea’s picture

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

claudiu.cristea’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks @claudiu.cristea!

claudiu.cristea’s picture

Issue tags: -#pnx-sprint, -CodeSprintUA

Remove some tags.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
49.54 KB
57.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!

Gábor Hojtsy’s picture

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

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

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

larowlan’s picture

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.