In #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity), several of the procedural image functions were moved to methods, but not uri(). Instead, image_style_entity_uri() was added, had a typo, and has no coverage.

Since this function has existed for less than 24 hours, no API change.

#13 1885644-interdiff-13.txt7.82 KBandypost
#13 1885644-image-uri-13.patch13.98 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,987 pass(es). View
#11 1885644-image-uri-11.patch13.78 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,718 pass(es). View
#10 1885644-interdiff-10.patch13.52 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,685 pass(es). View
#10 1885644-image-uri-10.patch14.05 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,718 pass(es). View
#4 image-1885644-4-FAIL.patch867 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,678 pass(es), 2 fail(s), and 0 exception(s). View
#4 image-1885644-4-PASS.patch2.33 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,672 pass(es). View
image-uri-PASS.patch2.23 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,433 pass(es), 51 fail(s), and 6 exception(s). View
image-uri-FAIL.patch764 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,454 pass(es), 38 fail(s), and 6 exception(s). View


Status: Needs review » Needs work

The last submitted patch, image-uri-PASS.patch, failed testing.

swentel’s picture

Tests pass locally for me as well.

aspilicious’s picture

Fatal error: Call to a member function id() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.php on line 37
FATAL Drupal\image\Tests\ImageAdminStylesTest: test runner returned a non-zero error code (255).

tim.plunkett’s picture

Title: Image::uri() is broken and has no coverage » ImageStyle::uri() is broken and has no coverage
Status: Needs work » Needs review
2.33 KB
PASSED: [[SimpleTest]]: [MySQL] 49,672 pass(es). View
867 bytes
FAILED: [[SimpleTest]]: [MySQL] 49,678 pass(es), 2 fail(s), and 0 exception(s). View

The testbots run in a subdir (or with a prefix, I forget) and that throws url() off.

xjm’s picture

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageAdminStylesTest.phpundefined
@@ -120,6 +120,10 @@ function testStyle() {
+    $style_uri_path = url($style_uri['path'], $style_uri['options']);
+    $this->assertTrue(strpos($style_uri_path, $style_path) !== FALSE, 'The image style URI is correct.');

Can we add an inline comment here explaining the subdirectory thing?

andypost’s picture

Status: Needs review » Postponed

I'm changing a path to image style in #1809376-4: Use EntityListController for image styles
So let's commit a proper patch

tim.plunkett’s picture

Status: Postponed » Needs review

No, this doesn't make it any harder to do that conversion, it just means you have to change one line here.
Also, this is a current major bug in HEAD, that's a conversion.

sun’s picture

Hm. I don't see any other entity type that overrides the ::uri() method in this way. The existing are specifying a uri_callback.

Could we move that approach/idea of overriding ::uri() into a separate issue? I'd find it inconsistent if some entities in core are doing it this way and some are doing it in another way.

Essentially, just s/manage/edit/ + plus test assertion here?

That said, @andypost is right in that the patch *should* be /manage instead of /edit. So I think it would make sense to perform that change first.

tim.plunkett’s picture

View::uri() does :)
And the rest should.

andypost’s picture

14.05 KB
PASSED: [[SimpleTest]]: [MySQL] 49,718 pass(es). View
13.52 KB
PASSED: [[SimpleTest]]: [MySQL] 49,685 pass(es). View
andypost’s picture

13.78 KB
PASSED: [[SimpleTest]]: [MySQL] 50,718 pass(es). View
+++ b/core/modules/image/image.module
@@ -201,9 +201,6 @@ function image_theme() {
     // Theme functions in
-    'image_style_list' => array(
-      'variables' => array('styles' => NULL),
-    ),

This is not a part of patch :( It seems that we have no tests for registry

sun’s picture

Thanks, that makes a lot more sense.

+++ b/core/modules/image/
@@ -412,7 +412,7 @@ function image_effect_delete_form_submit($form, &$form_state) {
-  $form_state['redirect'] = 'admin/config/media/image-styles/edit/' . $style->id();
+  $form_state['redirect'] = 'admin/config/media/image-styles/manage/' . $style->id() . '/edit';

+++ b/core/modules/image/image.module
@@ -134,7 +134,7 @@ function image_menu() {
-  $items['admin/config/media/image-styles/edit/%image_style'] = array(
+  $items['admin/config/media/image-styles/manage/%image_style/edit'] = array(
     'title' => 'Edit style',

The /edit path/suffix should actually be a MENU_DEFAULT_LOCAL_TASK; i.e., the path/to/%image_style itself should be the primary router path.

See contact_menu() for a reference implementation.

andypost’s picture

13.98 KB
PASSED: [[SimpleTest]]: [MySQL] 50,987 pass(es). View
7.82 KB

This a bit of out-scope but really makes sense

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

andypost’s picture

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