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.

CommentFileSizeAuthor
#13 1885644-interdiff-13.txt7.82 KBandypost
#13 1885644-image-uri-13.patch13.98 KBandypost
#11 1885644-image-uri-11.patch13.78 KBandypost
#10 1885644-interdiff-10.patch13.52 KBandypost
#10 1885644-image-uri-10.patch14.05 KBandypost
#4 image-1885644-4-FAIL.patch867 bytestim.plunkett
#4 image-1885644-4-PASS.patch2.33 KBtim.plunkett
image-uri-PASS.patch2.23 KBtim.plunkett
image-uri-FAIL.patch764 bytestim.plunkett

Comments

Status: Needs review » Needs work

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

swentel’s picture

swentel commented

Tests pass locally for me as well.

aspilicious’s picture

aspilicious commented

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

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Image::uri() is broken and has no coverage » ImageStyle::uri() is broken and has no coverage
Status: Needs work » Needs review
StatusFileSize
new image-1885644-4-PASS.patch2.33 KB
new image-1885644-4-FAIL.patch867 bytes

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

xjm’s picture

xjm
she/her
Primary language English
commented 
+++ 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

andypost
he/him
Primary language Russian
commented
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

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
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

sun
Primary language German
Location Karlsruhe
commented

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

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1885644-image-uri-10.patch14.05 KB
new 1885644-interdiff-10.patch13.52 KB

Added changes to fix urls from #1809376-4: Use EntityListController for image styles

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1885644-image-uri-11.patch13.78 KB 
+++ b/core/modules/image/image.module
@@ -201,9 +201,6 @@ function image_theme() {
     // Theme functions in image.admin.inc.
-    '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

sun
Primary language German
Location Karlsruhe
commented

Thanks, that makes a lot more sense.

+++ b/core/modules/image/image.admin.inc
@@ -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

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1885644-image-uri-13.patch13.98 KB
new 1885644-interdiff-13.txt7.82 KB

This a bit of out-scope but really makes sense

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented

Next clean-up #1809376-7: Use EntityListController for image styles

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