Problem/Motivation

Noticed in #2377747: Incorrect node create validation error when an invalid image is attached to a field already.

GDToolkit only returns 'png gif jpeg' as supported extensions, since in order to build the list it uses the image_type_to_extension on the list of IMAGETYPE_* constants returned by ::supportedTypes, and that function only maps one extension to each type.

The complete list would be 'png gif jpe jpg jpeg'.

Not a big deal in itself as ::parseFile processes files with those extensions too, but this impacts list exposed to users like e.g. in the 'convert' image effect and in image validator messages.

Proposed resolution

IDEALLY: Leverage the file.mime_type.mapper service being introduced in #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser, by getting the mime types of the internal image types and retrieving all the extensions associated to those mime types.

INTERIM: Take the GDToolkit changes in #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only).

Remaining tasks

Review

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#24 2477381-24.patch2.95 KBmondrake
#16 2477381-16.patch2.97 KBmondrake
#16 interdiff_9-16.txt751 bytesmondrake
#9 2477381-9.patch2.97 KBmondrake
#4 2477381-4.patch2.95 KBmondrake
#1 2477381-1-do-not-test.patch2.2 KBmondrake
#1 2477381-1-test-only.patch1.16 KBmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Active » Needs review
FileSize
2477381-1-test-only.patch1.16 KB
2477381-1-do-not-test.patch2.2 KB

test only patch shows the failure, do-not-test patch is built on top of #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser

Status: Needs review » Needs work

The last submitted patch, 1: 2477381-1-test-only.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Postponed

Postponed on #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Issue summary: View changes
Priority: Normal » Major
Status: Postponed » Needs review
Issue tags: +Contributed project soft blocker, +Media Initiative, +D8Media
Related issues: +#1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only)
FileSize
2477381-4.patch2.95 KB

Both #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser and #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only) are delaying, but this issue is blocking an Image Effects issue, #2651768-4: ImageSelector\Dropdown::getList should return all files supported by current toolkit.

So I am unpostponing this one, with a patch that takes the GDToolkit changes from #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only).

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.0.x-dev » 8.1.x-dev

maybe better to re-title that with pointing about jpeg only?

+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php
@@ -398,7 +398,15 @@ public static function isAvailable() {
+      if ($extension === 'jpeg') {
+        $extensions[] = 'jpg';
+        $extensions[] = 'jpe';

makes sense

Status: Needs review » Needs work

The last submitted patch, 4: 2477381-4.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Version: 8.1.x-dev » 8.2.x-dev
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
FileSize
2477381-9.patch2.97 KB

Reroll

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.2.x-dev » 8.1.x-dev

This is bug

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +needs backport to 8.1.x

@andypost the location of the kernel tests has moved in 8.2.x (see #2687897: Convert system module's kernel tests to NG), so the previous patch won't apply. I think it's better fix in 8.2.x and once done backport to 8.1.x which will be a different patch. Added backport tag.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php
@@ -107,6 +107,26 @@ function testManipulations() {
+      $this->assertEqual($expected_image_type, $image_type);

This should probably be assertSame(). Other than that, I see no problems with this patch. Once that's fixed, RTBC from me.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +blocker

This is now blocking #2652138: Image styles do not play nicely with SVGs.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review

#12

This should probably be assertSame().

Why? AFAICS assertSame is for objects - here we're comparing IMAGETYPE_* integer constants (http://php.net/manual/it/image.constants.php)

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

Please disregard #14, I misunderstood. This is doing #12.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
FileSize
interdiff_9-16.txt751 bytes
2477381-16.patch2.97 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

YowZA!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 2d292cd and pushed to 8.2.x. Thanks!

Re-open if we think this is worth backporting to 8.1.x

  • alexpott committed 2d292cd on 8.2.x 
    Issue #2477381 by mondrake: GDToolkit::getSupportedExtensions returns...
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Fixed » Needs work

Re-opened for backport to 8.1.x

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Patch (to be ported)

Actually status = Patch (to be ported)

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: -blocker
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Version: 8.2.x-dev » 8.1.x-dev
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Patch (to be ported) » Needs review
FileSize
2477381-24.patch2.95 KB

Same as #16, only difference usage of assertEqual instead of assertSame.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -needs backport to 8.1.x

Can I RTBC this given it's basically the same patch that was committed in 8.2.x?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -Media Initiative

Remove duplicate tag.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 613a15e and pushed to 8.1.x. Thanks!

  • alexpott committed 613a15e on 8.1.x 
    Issue #2477381 by mondrake: GDToolkit::getSupportedExtensions returns...

Status: Fixed » Closed (fixed)

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