Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
It would be useful to have a core Image::getExif()
method to enable fetching EXIF information from image files that contain such metadata, so to avoid custom/contrib to reinvent a way to get it in each implementation. Actually such a method should just hand over execution to the toolkit. A base implementation in ImageToolkitBase
seems feasible.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#11 | 2630242-11.patch | 32.38 KB | mondrake |
#11 | interdiff_9-11.txt | 1.09 KB | mondrake |
#9 | 2630242-9.patch | 32.39 KB | mondrake |
#9 | interdiff_2-9.txt | 15.78 KB | mondrake |
#3 | 2630242-2.patch | 29.12 KB | mondrake |
Comments
Comment #2
mondrakeWorking on a patch.
Comment #3
mondrakeA first patch. Thanks for any review :)
Comment #4
slashrsm CreditAttribution: slashrsm at Examiner.com commentedYup, it totally makes sense to have API for this in core. I have few comments:
It is not entirely clear what this status means. Let's try to make documentation a bit more clear.
Return value is of type int. What do different values mean?
If we reach this point something probably went wrong with the parsing process. We should always return NULL in that case, right?
Why @?
Is this meant to be used by anything outside toolkit class? If not then let's not make it public.
It is a bit confusing to have set method and no way to save that data into the image file. When I initially saw this function I assumed it is there to allow one to change EXIF info, which is not the case.
Comment #5
mondrakeThanks for reviewing @slashrsm!
Let's address point 5 before anything else, I think this is the key point:
Really? In Allowed changes during the Drupal 8 release cycle I find, in the allowed changes for minor releases,
Wouldn't this fit the definition? This is introducing an API improvement in existing API but it does not touch existing code or method signatures, only adds new methods. I could not find an indication that adding methods to interfaces will be a BC break. On the contrary, in the indications about introducing BC layers, the docs say:
i.e. you can have a new method as long as you also keep the old one.
Comment #6
slashrsm CreditAttribution: slashrsm at Examiner.com commentedAdding a function to an interface is a BC-breaking change. If there is a custom/contrib module that implements that interface it will stop working after such change.
There are ways around this. People usually mention creating new interface that extends the original one.
Comment #7
mondrakeYep, you're right... did not think about that. I will rework with new interfaces.
Thanks
Comment #8
mondrakeOk, new patch.
#4.5 - Moved the new methods to a
ImageExifInterface
and aImageToolkitExifInterface
. NowImage
implementsImageExifInterface
besidesImageInterface
, andImageToolkitBase
implementsImageToolkitExifInterface
besidesImageToolkitInterface
.Since
GDToolkit
extends fromImageToolkitBase
, it gets the new methods too.Any toolkit implementing
ImageToolkitInterface
, but not extending from the base class, will have to implement EXIF related methods on its own.#4.1 - Added docs.
#4.2 - OK.
#4.3 - Because
exif_read_data
may throw PHP notices in case of file parsing errors (e.g. zero size files). So we suppress errors from the call, but keep a parsing error condition in the $exifStatus property with valueImageToolkitExifInterface::EXIF_PARSE_FAILURE
. We do something similar forgetimagesize
in the GDToolkit.#4.4 - No that method is not meant to update info in the source file, but just its in-memory copy in the Image object. It must be public because ImageToolkitOperations may need to update that value. Two cases I know about:
a) you get an Image object from the source file, then get its Exif data, then call a CreateNew operation - in that case the operation need to reset EXIF data because it will be irrelevant to the new image
b) you apply an EXIF autorotate operation to set an image in its right visual orientation based on the EXIF 'Orientation' tag. Once done, you need to set the 'Orientation' tag appropriately (i.e. to '1') so to keep the image resource in sync with the EXIF tag (and avoid that re-calling the operation leads to wrong visualization).
There's no standard PHP function to write back EXIF data to an image file. I googled a bit and found a PEL (PHP EXIF Library) that could do that, but that would be a completely separate issue.
Comment #9
mondrake... and the patch
Comment #11
mondrakeFixed test.
Comment #12
claudiu.cristeaI know very little about EXIF and maybe some questions or comments will look naive but this can be also a point for a good review :) Before doing a review some general comments, concerns, questions:
Comment #13
mondrakeInteresting questions @claudiu.cristea :)
I am also not an expert here, the proposal here comes from my realization that different contrib modules all have their own implementation of Exif data parsing, all using the same
exif_read_data
PHP function from the 'exif' PHP extension. I know about:exif_read_data
I am sure there are other examples.
To your points:
I agree, let's discuss further what best architecture should be in place to support this
Comment #14
claudiu.cristeaAs a first look, I think that we need to make first an inventory of all kind of standards that are embedding meta-information into the file itself. Then imagine an interface that fits all those possible cases, even it's very abstract. Yes, this would be more a
file
issue than animage
. At the end, we just ship a plugin for EXIF.Also we need to look if this was not already implemented in contrib. I'm thinking to https://github.com/drupal-media/file_entity and other Media modules. Don't have time right now but I think the Media team should be involved in this discussion.
Comment #15
mondrakeTag typo
Comment #16
slashrsm CreditAttribution: slashrsm at Examiner.com commentedMedia team is involved in this discussion. file_entity doesn't handle that. media_entity_image does some of this, but since it is not the only storage component we have would make a lot of sense to have something that everybody can use.
Comment #18
mondrakeStarted a possible contrib module here https://github.com/mondrake/file_mdm. Any input appreciated.
Comment #19
mondrakeI have published on d.o a contrib module File metadata manager that is introducing a service and API to manage this, still in development. A module implementing an EXIF metadata plugin is in progress and will follow.
Comment #20
mondrakeI suggest to close this as won't fix. At least the approach of this patch, to get EXIF via the Image object, will not happen.
In the contrib space, the File metadata manager module is now in beta, and a first implementation proposed for ImageMagick at #2749283: Enable caching of ::parseFile results to reduce file I/O and shell calls. Reviews there appreciated :)
That will use File metadata manager and a plugin implemented in the ImageMagick module to retrieve and cache image information via ImageMagick's 'identify' executable.
A plugin to manage EXIF information is in wip at EXIF metadata plugin. That one could be used in the ImageMagick and the Image Effects modules that need access to image EXIF information.