Files: 
CommentFileSizeAuthor
#68 17282244-image-styles-followup-68.patch9.86 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 49,603 pass(es). View
#60 image-styles-1782244-60.patch11.32 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,328 pass(es). View
#48 drupal8.image-config.48.patch12.34 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-config.48.patch. Unable to apply patch. See the log in the details link for more information. View
#41 drupal8.image-config.41.patch15.09 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-config.41.patch. Unable to apply patch. See the log in the details link for more information. View
#36 drupal8.image-config.36.patch12.94 KBsun
FAILED: [[SimpleTest]]: [MySQL] 46,222 pass(es), 1 fail(s), and 0 exception(s). View
#26 1782244-image-styles-config-entity-26.patch50.92 KBheyrocker
PASSED: [[SimpleTest]]: [MySQL] 46,048 pass(es). View
#26 1782244-interdiff-24-26.txt3.64 KBheyrocker
#24 1782244-core-imagestyles-24.patch50.37 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,597 pass(es). View
#24 1782244-interdiff-19-24.txt18.35 KBandypost
#23 1782244-core-imagestyles-23.patch44.54 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,603 pass(es). View
#23 1782244-interdiff-19-23.txt1.73 KBandypost
#19 1782244-image-styles-config-entity_16.patch45.75 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 42,103 pass(es), 4 fail(s), and 24 exception(s). View
#16 1782244-image-styles-config-entity_16.patch45.75 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1782244-image-styles-config-entity_16.patch. Unable to apply patch. See the log in the details link for more information. View
#7 1782244-6-image-style-config-entity.patch5.97 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 41,996 pass(es), 133 fail(s), and 9 exception(s). View

Comments

larowlan’s picture

Issue tags: +#pnx-sprint

tagging

andypost’s picture

Priority: Major » Normal
catch’s picture

Priority: Normal » Major

This seems major (at least) to me? Also #1781372: Add an API for listing (configuration) entities is in so we can use that for the image style configuration page now.

catch’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Priority: Normal » Major

Can we make it after feature freeze?

xjm’s picture

Priority: Major » Normal
tstoeckler’s picture

Assigned: Unassigned » tstoeckler

I'm going to take a stab at this. If there's no patch until Tuesday, feel free to take this or un-assign.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned
Status: Active » Needs work
FileSize
5.97 KB
FAILED: [[SimpleTest]]: [MySQL] 41,996 pass(es), 133 fail(s), and 9 exception(s). View

Posting a WIP patch in order to meet my own deadline. :-)

Works:
* List controller
Doesn't work:
* Form controller

Unassigning. I'll try to bring this home, but I'm not sure if I'll get to it.

heyrocker’s picture

Looking through this, I'm curious why you changed the path to all the admin urls? For instance, 'edit' is now 'manage', '[style]/delete' is now 'manage/[style]/delete', etc.

heyrocker’s picture

OK after discussing with xjm and timplunkett in IRC, I learned that it is more semantically proper and standard to go [thing]/[task] rather than the other way around (which is how it was in D7.) An open question to me though is that 'manage' seems unnecessary, we could just go '[style]/edit' etc.

heyrocker’s picture

Assigned: Unassigned » heyrocker

I'm grabbing this, I know its marked as novice but it will provide me a good opportunity to get more well versed in config entities.

heyrocker’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

andypost’s picture

This issue also should kill one theme function - replace with listController, related issue #1805928: Convert theme_image_style_list

andypost’s picture

Issue summary: View changes

Added link to meta issue

tstoeckler’s picture

Well we need the 'manage' in there, because otherwise we have the following menu routers:
admin/config/media/image-style/%image_style
admin/config/media/image-style/add

So you inherently can't name an image style 'add'. In general, I think we should try to avoid both wildcards and static paths on the same level of a menu tree. We do the same thing for node types and currently for the config test entity.

The reason I changed this in the first place is because the list controller appends '/edit' and '/delete' to the entity URIs, to create the operation links. The menu routers before were
admin/config/media/image-style/delete/%image_style
admin/config/media/image-style/edit/%image_style
so that obviously didn't work.

Edit: Where I said "config entity" before, I actually meant "config test entity".

andypost’s picture

tstoeckler’s picture

Right, that shows that they have the same problem over there. They need to change the menu routers as well.

tstoeckler’s picture

Issue summary: View changes

Added twig related issues

heyrocker’s picture

Status: Needs work » Needs review
FileSize
45.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1782244-image-styles-config-entity_16.patch. Unable to apply patch. See the log in the details link for more information. View

Here we go. As discussed elsewhere, this is just the conversion to ConfigEntity and does not include the ListController or FormController changes. There are only two other moderately significant changes here

- I removed image_style_effects() because it was totally pointless.
- I removed hook_image_style_alter(). We are going to get rid of it anyways, and it reduced the code in that area. A few months ago I did some research and to the best of my knowledge nobody is using that hook in contrib-land. That said, if someone wants to push that change off to a followup then I'm fine with that too.

Not addressed at all is that we should just remove all the image_effect_*() functions entirely. That is total followup work.

This passes all tests locally.

Status: Needs review » Needs work

The last submitted patch, 1782244-image-styles-config-entity_16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

This all looks awesome.

+++ b/core/modules/image/image.admin.incundefined
@@ -195,40 +195,38 @@ function image_style_form_submit($form, &$form_state) {
   image_style_save($style);

@@ -270,9 +268,10 @@ function image_style_add_form_submit($form, &$form_state) {
+  image_style_save($style);

+++ b/core/modules/image/image.moduleundefined
@@ -506,8 +507,9 @@ function image_config_import_create($name, $new_config, $old_config) {
+  image_style_save($style);

@@ -519,7 +521,7 @@ function image_config_import_change($name, $new_config, $old_config) {
   return image_style_save($style);

These should all be $style->save()
(and more in the tests)

+++ b/core/modules/image/image.moduleundefined
@@ -625,22 +597,8 @@ function image_style_load($name) {
 function image_style_save($style) {
-  $config = config('image.style.' . $style['name']);
-  $is_new = $config->isNew();
-
-  $config
-    ->set('name', $style['name'])
-    ->set('label', $style['label']);
-  if (isset($style['effects'])) {
-    $config->set('effects', $style['effects']);
-  }
-  else {
-    $config->set('effects', array());
-  }
-  $config->save();
+  $style->save();
 
-  // Let other modules update as necessary on save.
-  $style['is_new'] = $is_new;
   module_invoke_all('image_style_save', $style);
 
   // Clear all caches and flush.

This should all be removed, and use hook_image_style_presave() that is triggered by ConfigStorageController::save()

+++ b/core/modules/image/image.moduleundefined
@@ -663,34 +621,18 @@ function image_style_save($style) {
 function image_style_delete($style, $replacement_style_name = '') {
   image_style_flush($style);
 
-  $config = config('image.style.' . $style['name']);
-  $config->delete();
+  $style->delete();
 
   // Let other modules update as necessary on save.
-  $style['old_name'] = $style['name'];
-  $style['name'] = $replacement_style_name;
+  if ($replacement_style_name) {
+    $style->set('name', $replacement_style_name);
+  }
   module_invoke_all('image_style_delete', $style);
 
   return TRUE;

Ideally the same for here, but I'm not sure about the replacement style stuff

+++ b/core/modules/image/lib/Drupal/image/ImageStyle.phpundefined
@@ -0,0 +1,45 @@
+  public $name;
+  ¶
+  /**
+   * The image style label.
+   *
+   * @var string
+   */
+  public $label;
+  ¶
+  /**

Trailing whitespace.

+++ b/core/modules/image/lib/Drupal/image/ImageStyle.phpundefined
@@ -0,0 +1,45 @@
+   * Implements Drupal\Core\Entity\EntityInterface::id().
+   */
+  public function id() {
+    return $this->name;

The docblock should be "Overrides Drupal\Core\Entity\Entity::id()."

But in reality, Drupal\Core\Entity\Entity::id() should be less stupid and use the entity keys...

heyrocker’s picture

FileSize
45.75 KB
FAILED: [[SimpleTest]]: [MySQL] 42,103 pass(es), 4 fail(s), and 24 exception(s). View

Not completely sure what happened there

Status: Needs review » Needs work

The last submitted patch, 1782244-image-styles-config-entity_16.patch, failed testing.

heyrocker’s picture

Assigned: heyrocker » Unassigned

Crap I forgot about the upgrade path tests. If someone wants to pick this up at this point, feel free. I may not get back to it within a couple days.

heyrocker’s picture

Also, when the change notice for this is written, we should make sure and note the following: Upgrade path will migrate all styles present in the image_styles table. However, it will not update and styles supplied via hook_image_effect_info(). It will be responsibility of the contributed module to supply this style information in a YAML file in the module's config directory.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
44.54 KB
PASSED: [[SimpleTest]]: [MySQL] 42,603 pass(es). View

reverted changes in upgrade path and fixed small whitespace issues

andypost’s picture

FileSize
18.35 KB
50.37 KB
PASSED: [[SimpleTest]]: [MySQL] 42,597 pass(es). View

Added new hooks and a bit cleaned up doc blocks
Removed image_style() image_style_save() so fixed all of #18 except image_style_delete()

Suppose we need to override delete() method of entity and introduce storage controller with this override

EDIT todo:
1) how to deal with alter hook_image_styles_alter()
2) change path for administration could be done in follow-up for form-controller admin/config/media/image-styles/edit/

heyrocker’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/image.moduleundefined
@@ -537,160 +554,71 @@ function image_config_import_delete($name, $new_config, $old_config) {
+function image_image_style_update($style) {
+  // Flush cached media for the style.
   image_style_flush($style);
-
-  return $style;

Does this get called on creates as well as saves?

I think we should leave the delete stuff for now while we work out the replacement style stuff. We have to open a followup to figure out how to deal with that in deployments, so we can address it there.

I also think we are fine removing hook_image_styles_alter() entirely. I've done a reasonably in-depth search of contrib and I haven't found any instances of it being used, and there are none in core. We have been establishing a standard that there are no alters for config as well. Dump it.

Actually shouldn't image.api.php be essentially emptied out? All usages of the hook_image_crud() hooks should move to responding to the appropriate entity hooks so are we repeating them here? They aren't really 'image api' hooks anymore. It seems like the only ones that should stay are

hook_image_effect_info()
hook_image_effect_info_alter() (both of these pending reworking of the image effect api)
hook_image_style_flush()

So other than the doc questions and the one question above I think this is ready to go. I've been testing it quite a bit in accordance with the import UI patch and things are working well.

heyrocker’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
50.92 KB
PASSED: [[SimpleTest]]: [MySQL] 46,048 pass(es). View

I just talked to jhodgdon about the documentation issue, and she agrees that the hook_ENTITY_NAME_CRUD() hooks should not be documented anywhere outside of entity.api.php. Therefore here is a new patch that removes them. I also verified that an entity create will call hook_entity_name_udpate() so that is no longer an issue. I won't RTBC this patch even though its technically just a docs reroll, but I consider it so.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

The only ugly part of this patch is the list(, , $id) = explode('.', $name); stuff, but that will be addressed in #1760358: Provide a way to extract the ID from a config object name.

Everything else looks great!

andypost’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

+1 to RTBC, We need this get commited to proceed with
#1788542: Use EntityFormController and EntityListController for image styles
#1809376: Use EntityListController for image styles

+++ b/core/modules/image/image.moduleundefined
@@ -537,160 +554,71 @@ function image_style_delete($style, $replacement_style_name = '') {
+  $style->delete();
 
   // Let other modules update as necessary on save.
-  $style['old_name'] = $style['name'];
-  $style['name'] = $replacement_style_name;
+  if ($replacement_style_name) {
+    $style->set('name', $replacement_style_name);
+  }

Filed as follow-up #1818702: Get rid of image_style_delete() to find a proper way to implement this

catch’s picture

catch’s picture

Title: Convert image styles $style array into ImageStyle (extends ConfigEntity) » Change notice: Convert image styles $style array into ImageStyle (extends ConfigEntity)
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Great to see this one ready. Looks like we'll need a change notice since this removes the _alter() hook and other things. Committed/pushed to 8.x.

xjm’s picture

We should additionally document the conversion on http://drupal.org/node/1818734. I've added this issue there but not updated it.

andypost’s picture

I've updated all related issues with links to this issue, suppose we could change title and priority

http://drupal.org/node/1818734
http://drupal.org/node/1734556
http://drupal.org/node/1799642

heyrocker’s picture

Status: Active » Needs review
andypost’s picture

Title: Change notice: Convert image styles $style array into ImageStyle (extends ConfigEntity) » Convert image styles $style array into ImageStyle (extends ConfigEntity)
Priority: Critical » Normal
Status: Needs review » Fixed

Suppose all places are fixed, also we have a special change notice. heyrocker++

webchick’s picture

I came across #1821534: Can't delete image effects from an image style when testing #1697256: Create a UI for importing new configuration today. Most likely from this issue, I guess. Can someone take a look?

sun’s picture

Priority: Normal » Major
Status: Fixed » Needs review
FileSize
12.94 KB
FAILED: [[SimpleTest]]: [MySQL] 46,222 pass(es), 1 fail(s), and 0 exception(s). View

Sorry for coming late to the party. Unfortunately, there are a range of major problems in the committed code:

(bumping to major, even though some of the changes are actually critical regressions which should turn this issue into a critical bug, but I don't want to pollute the critical queue)

+++ b/core/modules/image/image.admin.inc
@@ -195,40 +195,38 @@ function image_style_form_submit($form, &$form_state) {
+  // Update the image style name if it has changed. We can not rename a piece
+  // of config, all we can do is create a new one and delete the old one.
+  if (isset($form_state['values']['name']) && $style->id() != $form_state['values']['name']) {

This is actually not true. Configurables have explicit support for renames, which is even covered by tests.

And after fixing the code and testing this manually, it also works as expected.

+++ b/core/modules/image/image.module
@@ -328,7 +329,7 @@ function image_file_predelete(File $file) {
  * Implements hook_image_style_save().
  */
 function image_image_style_save($style) {
-  if (isset($style['old_name']) && $style['old_name'] != $style['name']) {
+  if ($style->id() != $style->getOriginalID()) {
     $instances = field_read_instances();
     // Loop through all fields searching for image fields.
     foreach ($instances as $instance) {

This should actually run on update only.

Worse, I fail to see how and from where this hook would be invoked. ConfigStorageController invokes the commonly known entity CRUD hooks only; i.e.:

hook_ENTITY_TYPE_insert()
hook_ENTITY_TYPE_update()
hook_entity_insert()
hook_entity_update()

+++ b/core/modules/image/image.module
@@ -360,6 +361,8 @@ function image_image_style_save($style) {
 function image_image_style_delete($style) {
   image_image_style_save($style);

While this hook is invoked, I doubt it is actually doing something.

In order for this to work, $style->id() would have to be set to the replacement style ID before image_image_style_save() is invoked, so it is actually doing something.

And for all of this to work in the config import process, we actually have to figure out a fair amount of things. Updating/replacing references like Image module is doing for image styles will be quite a common task for all Configurables that can be referenced in other configuration. This gets me back to #1609418: Configuration objects are not unique, and as long as that is blocked on consensus, we will have to introduce a new mechanism for the config import process that is able to record and map "replacement config", tracked by UUIDs. The idea of a manifest file, as proposed in #1697256: Create a UI for importing new configuration, might help us to resolve that problem.

+++ b/core/modules/image/image.module
@@ -537,160 +554,71 @@ function image_config_import_delete($name, $new_config, $old_config) {
+  $style = entity_load('image_style', $id);
   return image_style_delete($style);

Should call $style->delete().

+++ b/core/modules/image/image.module
@@ -537,160 +554,71 @@ function image_config_import_delete($name, $new_config, $old_config) {
+ * @param string $name
+ *   The ID of the ImageStyle object to load.
  */
...
+function image_style_load($name) {

I wonder why we didn't rename 'name' to 'id' as part of this change?

We should create a follow-up for that.

+++ b/core/modules/image/image.module
@@ -537,160 +554,71 @@ function image_config_import_delete($name, $new_config, $old_config) {
+function image_image_style_load($styles) {

Normally, we are extending/overriding the entity storage controller for such operations.

+++ b/core/modules/image/image.module
@@ -1124,38 +1047,31 @@ function image_effect_load($ieid, $style_name) {
+function image_effect_save($style, &$effect) {
...
+  $style->effects[$effect['ieid']] = $effect;
+  $style->save();

This is the reason for why we see the full effect definition in configuration files after updating an image effect.

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.php
@@ -34,14 +34,14 @@ function createSampleImage($style) {
+    return image_style_url($style->id(), $file_path) ? $file_path : FALSE;

This patch introduced image_style_uri(), but there is a image_style_url() already, which is very confusing. We should rename the entity URI callback to image_style_entity_uri().

+++ b/core/modules/user/user.module
@@ -2920,8 +2920,8 @@ function user_image_style_delete($style) {
 function user_image_style_save($style) {

Same problem as Image module's implementation.

---

Attached patch fixes most of the problems. I did not move the code to a new ImageStyleStorageController yet though.

Status: Needs review » Needs work

The last submitted patch, drupal8.image-config.36.patch, failed testing.

yched’s picture

Do we still have a use case for the unique ieid ? AFAIK those were only introduced because of limitations in our former *xml* storage (couldn't handle arrays properly).
They are also very hacky and brittle: a string being a hash of effect name plus its settings - no way we can reliably generate that for any arbitrary collection of settings. Besides, it doesnt ensure uniqueness anyway you could perfectly well have the same effect with the same settings twice in the same style, if the effect is not idempotent ("darken by N%")

andypost’s picture

Priority: Major » Normal
Status: Needs work » Fixed

@sun this issue is about convertion, all other staff should be done in follow-ups as it was written in comments above.

Configurables have explicit support for renames, which is even covered by tests.

ConfigStorageController invokes the commonly known entity CRUD

hook_ENTITY_TYPE_insert() => image_image_style_save() && user_image_style_save() is not it?
hook_ENTITY_TYPE_delete() => image_image_style_delete() is not it?

This patch introduced image_style_uri(), but there is a image_style_url() already

They are very different things

$style->effects[$effect['ieid']] = $effect;

Addressed in #1821534: Can't delete image effects from an image style

PS: I update summary with all this follow-ups

andypost’s picture

Issue summary: View changes

Added follow-ups

andypost’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Title: Convert image styles $style array into ImageStyle (extends ConfigEntity) » Regression: Convert image styles $style array into ImageStyle (extends ConfigEntity)
Category: task » bug
Priority: Normal » Critical
Status: Fixed » Needs review
Issue tags: +Needs tests, +regression
FileSize
15.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-config.41.patch. Unable to apply patch. See the log in the details link for more information. View

Ok, sorry, you asked for it. #36 tried to clarify that the committed patch entirely broke Image module's maintenance of image styles in field settings.

The regressions are completely obvious in the follow-up patch: hook_image_style_save() no longer exists.

Attached patch fixes the test failure.

Filed #1822150: EntityStorageControllerInterface::delete() should accept an array of entities, not IDs as a follow-up for the introduced workaround to fix the failures.

Since this is a critical bug now, we need tests that prevent us from re-introducing the regression again. Essentially:

1) Add an image field, using 'medium' style for image widget form upload preview, and 'large' style for the formatter.
2) Create an entity with an image.
3) Delete the 'medium' style using 'thumbnail' as a replacement.
4) Assert that the image field instance's form widget preview style was updated to 'thumbnail'.
5) Delete the 'large' style using 'thumbnail' as a replacement.
6) Assert that the image field formatter was updated to 'thumbnail'.

tim.plunkett’s picture

+++ b/core/modules/image/image.moduleundefined
@@ -72,6 +72,35 @@ function image_help($path, $arg) {
+ * Implements hook_entity_info().
+ */
+function image_entity_info() {
+  return array(
+    'image_style' => array(
+      'label' => t('Image style'),
+      'entity class' => 'Drupal\image\ImageStyle',
+      'controller class' => 'Drupal\Core\Config\Entity\ConfigStorageController',
+      'uri callback' => 'image_style_entity_uri',
+      'config prefix' => 'image.style',
+      'entity keys' => array(
+        'id' => 'name',
+        'label' => 'label',
+        'uuid' => 'uuid',
+      ),
+    ),
+  );

@@ -1163,31 +1129,3 @@ function _image_effect_definitions_sort($a, $b) {
-/**
- * Implements hook_entity_info().
- */
-function image_entity_info() {
-  return array(
-    'image_style' => array(
-      'label' => t('Image style'),
-      'entity class' => 'Drupal\image\ImageStyle',
-      'controller class' => 'Drupal\Core\Config\Entity\ConfigStorageController',
-      'uri callback' => 'image_style_uri',
-      'config prefix' => 'image.style',
-      'entity keys' => array(
-        'id' => 'name',
-        'label' => 'label',
-        'uuid' => 'uuid',
-      ),
-    ),
-  );

That certainly makes it hard to follow this patch. Can you at least try to make it reviewable?

sun’s picture

Also filed #1822176: Why do we have $entity_info['entity keys']['id'] *and* MyEntity::id() method overrides? as another follow-up, since I was very tempted to remove the ::id() method override from ImageStyle, only to figure out that we have some weird entity abstraction duplication going on there.

re #42:
The only change to image_entity_info() is the renamed entity URI callback function name.

The reason for moving it up is that we generally put the registry-alike info hook implementations at the top of a .module file for easier/quick discovery. When I initially reviewed the new code, I completely missed the hook_entity_info() implementation and almost thought it was forgotten, because it was added to the bottom.

xjm’s picture

So we had no test coverage for that hook whatsoever? Yikes.

xjm’s picture

Issue tags: +Novice

sun's clear "test recipe" in #41 should make it straightforward to add test coverage for this.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Novice, +regression, +Configuration system, +Config novice, +Configurables, +#pnx-sprint

The last submitted patch, drupal8.image-config.41.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
12.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-config.48.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolled against HEAD.

patrickd’s picture

Status: Needs review » Reviewed & tested by the community

Tested as per #42
Works good, looks good

webchick’s picture

Status: Reviewed & tested by the community » Needs work

If we're fixing a regression, where are the tests?

heyrocker’s picture

Issue tags: -Novice, -Config novice

Removing novice tags

znerol’s picture

It occurs to me that ImageAdminStylesTest::testStyleReplacement should cover the case outlined in #41. What is missing in that test?

znerol’s picture

Successfully performed the test sequence outlined in #41 on current 8.x (without this patch). That means either we need another test sequence or the problem was fixed in #1821534: Can't delete image effects from an image style (covered by tests). It also means that objections in #45, #50 are probably invalid.

znerol’s picture

Successfully performed the test sequence outlined in #41 on current 8.x with commit 0d073ce #1821534: Can't delete image effects from an image style reverted. That means that I'm sort of lost now. @sun or @xjm could you please point out what exactly needs to be tested for?

andypost’s picture

@znerol sequence from #41 needs to be implemented as test* to proof that #48 fixes the regression

znerol’s picture

@andypost: I understand. But in order to verify that the patch actually fixes the regression, we need a test that fails before applying the patch (i.e. current 8.x) and succeeds after applying the patch. However performing the steps outlined in #41 by hand on a fresh 8.x install (and even after rolling back 0d073ce) does not expose any problems. That means that those steps are not suitable to cover the regression.

Before implementing the test I have to know which sequence of steps will lead to the problem the patch tries to fix.

catch’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -regression, -Configuration system, -Configurables, -#pnx-sprint

#48: drupal8.image-config.48.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +regression, +Configuration system, +Configurables, +#pnx-sprint

The last submitted patch, drupal8.image-config.48.patch, failed testing.

andypost’s picture

patch needs re-roll

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.32 KB
PASSED: [[SimpleTest]]: [MySQL] 50,328 pass(es). View

#1292470: Convert user pictures to Image Field made some of it obsolete.

Still needs tests.

Status: Needs review » Needs work
Issue tags: -Needs tests, -regression, -Configuration system, -Configurables, -#pnx-sprint

The last submitted patch, image-styles-1782244-60.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +regression, +Configuration system, +Configurables, +#pnx-sprint

#60: image-styles-1782244-60.patch queued for re-testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Confirm #53 Styles are replaced successfully without patch and we have a test for that replacement

Anyway patch looks a good clean-up

webchick’s picture

Status: Reviewed & tested by the community » Needs work

No tests?

heyrocker’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests, -regression

testStyleReplacement() has always existed to test style replacement, which was never actually broken despite claims to the contrary (verified in #53 and #63.) Setting back to RTBC.

webchick’s picture

Title: Regression: Convert image styles $style array into ImageStyle (extends ConfigEntity) » Convert image styles $style array into ImageStyle (extends ConfigEntity)
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +regression

I just spent like 20 minutes trying to parse what the actual critical problem is here in order to determine what regression we're fixing, and it seems like there is none.

- There are steps to reproduce a bug in #41. However, those steps are refuted by later comments (#53 and #56) which are able to do those tasks successfully.
- The patch itself doesn't really contain much in the way of functional changes aside from removing image_style_delete() in favour of $style->delete() and changing hook_image_style_save() to hook_image_style_update(). One would think that if that were fixing some kind of critical bug, the existing tests in ImageAdminStylesTest.php would've been raising a stink about it, because they seem pretty thorough.

So as far as I can tell, this is just a straight-forward follow-up to improve some things, by better leveraging more of the Config Entity API and making the scope of the hook more direct. OK, great. But let's done down the drama-meter, OK? :)

Needs a re-roll.

webchick’s picture

Issue tags: -Needs tests, -regression

project_issue--

heyrocker’s picture

FileSize
9.86 KB
PASSED: [[SimpleTest]]: [MySQL] 49,603 pass(es). View

Here is a reroll to HEAD. One slightly notable change: hook_config_import_delete() was removed by #1806178: Remove code duplication for hook_config_import_*() implementations of config entities, and the @todo related to it in image.api.php is also resolved from that issue, so the two related hunks are just removed.

Other than that its a straight reroll. Should go right back to RTBC if the bot passes.

heyrocker’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

andypost’s picture

Major follow-up #1821848-17: Move image style load/update/delete operations into a new ImageStyleStorageController

ImageStyleStorageController::importDelete() uses removed function image_style_delete()

Status: Fixed » Closed (fixed)

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

ngf’s picture

Issue summary: View changes

Updated issue summary.