Updated: Comment #33.
Problem/Motivation
- Right now
ImageInterface::getExtension()
is used to check if an image is a supported image type or if the file is an image file. This is a bad pattern because:- The extension is a filesystem component that can lie about the file's nature,
- A file having
.PNG
extension on disk may have a JPEG or even a non-image content, - Image type is not the same thing as the image extension.
- Being a filesystem component, the extension is not something that is linked to the toolkit (
ImageToolkitInterface
) but to the image (ImageInterface
). Toolkits should have image types, images should have image type (talking about content nature) and extension (talking about external, filesystem file extension). - Image types are determined (badly, by extension) in
GDToolkit::getInfo()
based on a hardcoded list of extensions. - If a custom module wants to add a new image type (extension), he normally has to extend
GDToolkit
and forkGDToolkit::getInfo()
. Ugly! - The list of possible image types is something that should be provided by the toolkit. Some toolkits may provide more types than the other.
- Extension should be decoupled from image type. Images created from image resources (images not on disk) or temporary images have no extension while they still have an image type.
Proposed resolution
- Add
supportedTypes
method toImageToolkitInterface
. supportedTypes
should return a list ofIMAGETYPE_*
constants. Note thatIMAGETYPE_*
constants are PHP constants and are not bundled to GD library.- Declare a simple
ImageInterface::isSupported()
method inImageInterface
with implementation inImage
that will be used to test if a image type is supported by the current toolkit or if a file is a valid image. ImageInterface::getExtension()
should ALWAYS return the filesystem extension and will not be used for other purposes anymore. What would be the logic of returning an extension different from the real filesystem extension as long we will not use the extension as a way to check the image type or if a file is image?.ImageInterface::getExtension()
will fallback toimage_type_to_extension()
for images created from resources or images without any extension on filesystem (like temporary files).- Declare
ImageInterface::getType()
that returns the correspondingIMAGETYPE_*
constant.
Remaining tasks
Rework #2063373: Cannot save image created from scratch
User interface changes
No changes in UI.
API changes
- New
ImageTookitInterface::supportedTypes()
static method. Implementations must declare the supported image types. - New
ImageInterface::isSupported()
method. This is used if a image type is supported OR if a file is a valid image. - New
ImageInterface::getType()
method. Used to test if an image is of a specified type.
Related Issues
Original report by claudiu.cristea
It turned out in #2063373-20: Cannot save image created from scratch that Image::getExtension()
needs to check if the image extension is a valid one (.png, .gif, .jpg). It's not the only place. Extension are already checked in GDToolkit::getInfo()
. This is a code duplication and, worst, a hardcoded list of extensions.
If a custom module wants to add a new image extension, he normally has to extend GDToolkit
and fork GDToolkit::getInfo()
. This is really ugly! But, if #2063373: Cannot save image created from scratch will get in, then will have to extend Image
too and fork the ::getExtension()
. In fact #2063373-20: Cannot save image created from scratch is wrong because the list of possible extensions is something that should be provided by the toolkit. Some toolkits may provide more extensions than the other.
Comment | File | Size | Author |
---|---|---|---|
#38 | image-types-2066219-38.patch | 18.64 KB | claudiu.cristea |
#38 | interdiff.txt | 542 bytes | claudiu.cristea |
#36 | image-types-2066219-36.patch | 18.66 KB | claudiu.cristea |
#36 | interdiff.txt | 3.34 KB | claudiu.cristea |
#34 | image-types-2066219-34.patch | 17.84 KB | claudiu.cristea |
Comments
Comment #0.0
claudiu.cristeaUpdated issue summary.
Comment #0.1
claudiu.cristeaUpdated issue summary.
Comment #1
claudiu.cristeaPatch.
Comment #2
larowlanEmpty patch :)
Comment #3
claudiu.cristeaSorry :)
Comment #4
larowlanMissing 'public' ?
Comment #6
claudiu.cristea@larowlan, it shouldn't be declared as public in the interface. All interface methods are public. The other methods of the same interface are without "public" too.
Comment #7
larowlanof course you're right :)
Comment #8
claudiu.cristeaThe problem are the JPEG images (IMAGETYPE_JPEG). There's one type and 2 extensions.
image_type_to_extension(IMAGETYPE_JPEG)
always returns.jpeg
extension. That's why we need an additional check to get the extension from the path. Even then, an empty extension may be returned (e.g. temporary uploaded files have no extension). In such situation fallback to.jpg
.Note that before this patch the returned extensions for JPEG images was always
.jpg
regardless of how the file was saved on disk,Comment #9
tim.plunkettI don't fully understand the need to add in that image_type_to_extension logic, can we have a test that justifies it?
s/event/even
s/self/static
grep -nr "public function" core | grep Interface.php | wc -l
1501
grep -nr "public static function" core | grep Interface.php | wc -l
22
grep -nr " function" core | grep Interface.php | wc -l
34
grep -nr " static function" core | grep Interface.php | wc -l
1
I'm going to have to disagree with you here. Which it is true you can't put protected/private methods on an interface, its in our coding standard to do so.
Comment #10
claudiu.cristeaCreated followup for interface method declarations: #2066879: Apply coding standards for interface method declarations.
Comment #11
tstoecklerThis should be static::getAvailableExtensions() in case someone wants to subclass.
Comment #12
claudiu.cristeaThere's nothing special with
image_type_to_extension()
. It's the "PHP way" to map from image type (IMAGETYPE_*
constants) to image extensions. What would be the alternative? Hardcoding another mapping array? I see this being the straight way for mapping (excepting the fact that is not completely handle the JPEG case).I cleaned also the other interface methods by adding the "public" prefix.
I added also a test and a patch only with the test.
Comment #13
claudiu.cristeaOh, forgot to "needs review"
Comment #14
fietserwinDo we need to cater for extensions in uppercase? If so, this should be changed in the testtoolkit as well and it should be documented that the new method returns an array of lowercase extensions.
I'm not a native English speaker but I think it is "An array ..."
Comment #15
claudiu.cristeaGood point, @fietserwin. The question is: How do we handle case inconsistencies?
Assuming that we'll change this line...
next
Image::getExtension()
may occur:image.png
=>png
image.PNG
=>png
image.jpg
=>jpg
image.jpeg
=>jpeg
image
(no extension but JPEG content) =>jpg
image.JPG
=>JPG
You can see that .png and .jpg inconsistencies are handled not in the same way. PNGs (uppercase) are converted to lowercase while JPGs (uppercase) are kept as they are.
Needs decision! How will
ImageToolkitInterface::getInfo
return the extension?Comment #16
tim.plunkettJust do
$extension = Unicode::strtolower($extension);
on the line before.Comment #17
claudiu.cristeaI would bet on that :)
Comment #18
fietserwinIMO, option 2 is better.
That said, and looking at the problems that might give, shouldn't the method being added be named getAvailableImageTypes (or getSupportedImageTypes) and return a list of IMAGETYPE_XXX constants. The getInfo() method would be changed to return the extension as is, or, if absent (e.g. new file), use image_type_to_extension() to define one and that one should be (will be) lowercase.
Though conceptually better, this might give new problems if e.g. the imagemagick toolkit wants to define more supported file types than GD supports. This because the IMAGETYPE_ constants and the image_type_to_extension() function are quite GD specific: Imagemagick toolkit would need to define its own constants and image_type_to_extension() may no longer be called in a non-strict GD environment (i.e. outside the GD toolkit class).
Comment #19
larowlanThis looks ready to me
Comment #20
fietserwinI had a better look at the documentation for getInfo():
This changed my mind. It outdates #14 (case is not to be considered) and #18 (option 1 is actually better).
So IMO getInfo() should just return jpg (for jpg images, as that is the "commonly" used extension) and ignore the actual extension (jpeg vs jpg) and thus also the case (JPG vs jpg).
This will make the code easier as it only has to call image_type_to_extension() and thus does not have to do an if on type (jpg versus other) and does not have to call the pathinfo and strltolower functions.
Furthermore, I would still favor to decouple extensions and image formats a bit more, thus changing the name of the method to the one as suggested in #18 (getSupportedImageTypes) or to getSupportedImageFormats() (which I now prefer as imagetype is GD specific speak). It should return a list of strings that denote the supported image formats, probably being a list of the commonly used extensions. Thus 1 per image format, thus it should not return jpg and jpeg but only jpg.
Comment #21
claudiu.cristea@fietserwin, remember that
image_type_to_extension(IMAGETYPE_JPEG)
always returns .jpegComment #22
fietserwinNot my preference or what I had expected, but I am OK with that. But: it will be an API change for getInfo() compared to the D7 image_gd_get_info() function, so perhaps handle that one differently?
Comment #23
claudiu.cristeaThank you. Long story, indeed :)
For now I think that:
.jpg
,.jpeg
... should be enough.
I'm not against abstracting more the toolkit and images objects but I think that should be a new issue. This issue was only to provide a unified way to check the extension of an image. Let's keep the issue inside the initial scope.
Comment #24
fietserwinFair enough, but then always return 'jpg' as D7 and current D8 do, otherwise we introduce an API change:
Comment #25
claudiu.cristeaThat is a bug. The "official" extension for JPEG images is .jpeg. The short .jpg bacame popular because was used first on filesystems where the extension wS limited to 3 characters.
Comment #26
fietserwinBug or not, it should return the "Commonly used file extension for the image" (singular). Which one that is going to be is not that important to me, but it should be one, not 2 possibilities.
To prevent nasty bugs in code using this information, I prefer to not change the currently returned value thus going for jpg. But I will RTBC as well if it is consistently returning jpeg.
Comment #27
fietserwinThis patch is BC with existing behavior (as it should IMO). We can do the suggested renaming (getSupportedImageFormats) in a later stage. but let's get this done so that the other issues blocked on this can progress.
Comment #28
claudiu.cristeaI deliberately didn't came back with a patch because I needed more time to think on this. The question is: For what purpose is used, in fact, ImageInterface::getExtension()? (edited)
Let's see:
This returns 16 occurrences:
$fileinfo->getExtension()
and the other related to Twig.if ($image->getExtension()) {...}
So, yes, you're right, I agree that we should make now this step.
Brief plan:
getSupportedTypes
method toImageToolkitInterface
.getSupportedTypes
should returnone of thea list ofIMAGETYPE_*
constantsor. We need to test and make sure first thatNULL
IMAGETYPE_*
are NOT provided by GD extension but by PHP itself.isImage()
method inImageInterface
with implementation inImage
that will be used for file testing.::getExtension()
should ALWAYS return the filesystem extension and will not be used for other purposes anymore. What would be the logic of returning an extension different from the real filesystem extension as long we will not use the extension as a way to check the image type or if a file is image?::getExtension()
will fallback toimage_type_to_extension()
for images created from resources or images without any extension on filesystem (like temporary files)Comment #29
claudiu.cristea@fietserwin, I will start working on the fix based on #28. Can you check if
IMAGETYPE_*
are not GD dependant?Comment #30
fietserwinIn the PHP documentation they are introduced in http://www.php.net/manual/en/image.constants.php. Though the path does not contain GD in the url, it is found under: PHP Manual > Function Reference > Image Processing and Generation > GD > Predefined constants.
OTOH: The functions image_type_to_extension() and image_type_to_mime_type() have a note that This function does not require the GD image library.... I am not sure how to check this. I work on Windows and cannot (AFAIK) disable GD, but will check that tomorrow, to late now for me.
Comment #31
claudiu.cristea@fietserwin, Yes the
image_type_to_*
functions are GD agnostic and it seems logical to me thatIMAGETYPE_*
must be too. But this is not documented anywhere.Comment #32
fietserwin#29:
It turns out that I could disable GD, some simple test after disabling revealed that:
- IMAGETYPE_xxx constants are available.
- Calling gd_info() resulted in a Fatal error: Call to undefined function gd_info().
#28:
- Not sure that we should go for IMAGETYPE_xxx constants, see my remark in last paragraph of #18. Why not going for mime types (for getSupportedTypes)? They are registered, so no confusion possible and they may be assumed to be complete (covering all image types that a toolkit may consider supporting). We then could use finfo_file (see http://www.php.net/manual/en/function.finfo-file.php) in isImage and check if it is supported with getSupportedTypes.
- getInfo should better remain BC (as in #27)
Comment #32.0
fietserwinUpdated issue summary.
Comment #32.1
claudiu.cristeaUpdating summary.
Comment #33
claudiu.cristeaUpdated title and summary according to #28.
Comment #33.0
claudiu.cristeasummary update
Comment #33.1
claudiu.cristeaSummary update.
Comment #34
claudiu.cristeaHere's the patch based on #28 and the new summary.
EDIT (ADDED): There's no interdiff as is not relevant.
Comment #35
fietserwinI did a review purely on going through the diff (i.e. I did not even apply it and have a look at the resulting classes). So far, I did not find any shocking points. Below are some minor points I would like to see changed.
Personally, I still prefer to switch to mime types. We now have some kind of duplication with properties type versus mimeType and methods getType() versus getMimeType(). So why drop the concept of extension as base to decide image format specific code on and instead introduce a new concept 'type' while we already have mimeType. What, e.g., if a toolkit supports Google's WebP image format for which PHP has no IMAGETYPE_XXX constant?
But if you can convince me, I'm willing to accept the way as the current patch does.
Can we make it clearer that these 3 are just examples, something like "One of the IMAGETYPE_XXX constants, e.g. IMAGETYPE_JPEG."
Same here. We are doing this also to allow other toolkits to support more then just these 3 image formats.
return in_array($this->getType(), $this->toolkit->supportedTypes()); ???
Checks if the image format is supported.
See above.
TODO: this error message becomes toolkit dependent...
I Prefer === for clarity.
see above.
Comment #36
claudiu.cristea@fietserwin, thank you for reviewing.
IMAGETYPE_*
constants seems to be the most abstracted way in PHP to represent image types. Also, I'm afraid that mime types are not consistent enough. For example I see that there are 2 mime types for JPEG images: image/jpeg and image/pjpeg (http://en.wikipedia.org/wiki/Internet_media_type#Type_image).Below some inputs on some of your points. The others are fixed in the interdiff.txt.
$this->type
is a protected property and is accessible from the class, why calling the getter?Comment #37
fietserwinBut this is RTBC for me, even if you keep disagreeing on those points.
Comment #38
claudiu.cristeaThat really makes sense to me (dropping one processInfo()). Let's change it :)
Comment #39
fietserwinAs said, RTBC, even while I prefer to switch to mime types., but let's move on (back) to the issues that are on the origin of this refactoring.
Comment #40
tim.plunkettI've largely stayed out of this issue, mostly because @claudiu.cristea and @fietserwin both know the actual implications of the change better than I. But now that they've come to a consensus for this issue, I can vouch for the patch, and +1 RTBC.
Comment #41
alexpottCommitted 156bdc4 and pushed to 8.x. Thanks!
Comment #42
claudiu.cristeaChange notice https://drupal.org/node/2104301
Comment #43
mondrakeChange notice looks good.
Comment #44
fietserwinMade some language corrections (singular -> plural), no conceptual changes, except changed "default toolkit" to "active toolkit".
Comment #45
claudiu.cristeaI think that it is safe to set this as fixed.
Comment #46.0
(not verified) CreditAttribution: commentedUpdated issue summary.