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.
Comment | File | Size | Author |
---|---|---|---|
#24 | 2477381-24.patch | 2.95 KB | mondrake |
Comments
Comment #1
mondraketest only patch shows the failure, do-not-test patch is built on top of #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser
Comment #3
mondrakePostponed on #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser
Comment #4
mondrakeBoth #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).
Comment #5
mondrakeComment #6
andypostmaybe better to re-title that with pointing about jpeg only?
makes sense
Comment #8
mondrakeComment #9
mondrakeReroll
Comment #10
andypostThis is bug
Comment #11
mondrake@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.
Comment #12
phenaproximaThis should probably be assertSame(). Other than that, I see no problems with this patch. Once that's fixed, RTBC from me.
Comment #13
phenaproximaThis is now blocking #2652138: Image styles do not play nicely with SVGs.
Comment #14
mondrake#12Why? AFAICS assertSame is for objects - here we're comparing IMAGETYPE_* integer constants (http://php.net/manual/it/image.constants.php)Comment #15
mondrakePlease disregard #14, I misunderstood. This is doing #12.
Comment #16
mondrakeComment #17
phenaproximaYowZA!
Comment #18
alexpottCommitted 2d292cd and pushed to 8.2.x. Thanks!
Re-open if we think this is worth backporting to 8.1.x
Comment #20
mondrakeRe-opened for backport to 8.1.x
Comment #21
mondrakeActually status = Patch (to be ported)
Comment #22
phenaproximaComment #23
mondrakeComment #24
mondrakeSame as #16, only difference usage of assertEqual instead of assertSame.
Comment #25
mondrakeCan I RTBC this given it's basically the same patch that was committed in 8.2.x?
Comment #26
Gábor HojtsyRemove duplicate tag.
Comment #27
alexpottCommitted 613a15e and pushed to 8.1.x. Thanks!