Problem/Motivation
Up to #2244359: Lazy-load GD resource in GDToolkit, it was not possible to build and save an image from scratch.
It was only possible to load an image from a file, manipulate it and save it at a destination.
Getting an image object from the image.factory without specifying a file to read from led to an invalid image object.
Invalid image objects would not save and there was no way to change the validity status through image operations.
Proposed resolution
#2244359: Lazy-load GD resource in GDToolkit moved image validity status checking to the toolkit, so it is now possible to get image objects from scratch and modify their validity status through image operation.
However we are missing a core operation to do that.
The proposal is to introduce a new 'create_new' image operation that would initialise an image if it is not got from file, and let each toolkit manage what it means to it.
To match the OP code, this would mean that a new image could be created like
$destination = 'public://image.png';
$image = \Drupal::service('image.factory')->get();
$image->createNew(20, 20);
// ... any other image manipulation operation
$image->save($destination);
which is toolkit agnostic.
For the GD toolkit, this means splitting the current 'createTmp' method into a 'getTransparentColor' method (to fetch transparent color channel from current resource) and a 'create_new' image toolkit operation plugin.
Remaining tasks
Review patch.
User interface changes
None.
API changes
One more method added to ImageInterface: createNew()
Original report by @claudiu.cristea
Updated: Comment #25
Problem/Motivation
I'm expecting that next piece of code will create an image on disk from an image resource:
$destination = 'public://image.png';
$res = imagecreatetruecolor(20, 20);
$image = new Image($destination, \Drupal::service('image.toolkit'));
$image
->setResource($res)
->setWidth(20)
->setHeight(20)
->save();
But it doesn't! :(
(unless I didn't understood well how to use the new Image
class or I missed something)
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#54 | 2063373-54.patch | 23.48 KB | mondrake |
#54 | interdiff_52-54.txt | 3.7 KB | mondrake |
#52 | 2063373-52.patch | 21.85 KB | mondrake |
Comments
Comment #1
claudiu.cristeaI wrote 2 tests: a PHPUnit test and also a simpletest (for qa.drupal.org), both illustrating this bug.
Comment #3
claudiu.cristeaDropped the phpunit test.
Comment #4
claudiu.cristeaComment #6
larowlanYou shouldn't need setResource just new Image and save()
Comment #7
claudiu.cristea@larowlan, and how to pass the resource? In this case it's an empty one but imagine that you need to generate an image from a processed resource. It should work.
Comment #8
larowlan$this->container->get('image.toolkit') would be better in this case
So what we're missing here is ::setExtension() which prevents the call into ::processInfo() and makes the test pass.
Comment #9
claudiu.cristeaGreat! That was my guess too. Didn't have time to test :)
Why we don't try first to extract the extension from
$image->getSource()
? Maybe in::processInfo()
? Course, only ifImage::$extension
is empty. In that case the simpletest should work without::setExtension()
(but keep::setExtension()
method too).Comment #10
larowlanthere is a line something like this in processInfo() (typing from memory mind you)
So that doesn't work with generated images. Ie there are three uses for the API, existing files, uploaded (tmp) files and dynamically generated files.
Without an extension set ->save() looks for a function named image, instead of imagepng/imagejpeg, and just dies.
With it set, it just works.
So I think getExtension is needed.
Comment #11
claudiu.cristeaHere's my idea from #9.
Comment #12
claudiu.cristeaJust a comment to #11:
If the file is on disk or
::setExtension()
is explicitly used, those will take precedence -- and that's fine.Comment #13
larowlanYep, happy with that, can you update the UnitTest too, we had 100% coverage on that class and I'm keen to keep it that way.
Should be able to do create an image with a nonexistent path but containing a .png and then call getExtension() and still get the right thing.
Comment #14
larowlanupdates unit test coverage
Comment #15
larowlanCode coverage report before #14
and after
Comment #16
andypostpatch needs re-roll against current core
Comment #18
larowlanyeah I stuffed the commands royally
Comment #20
claudiu.cristeaWe need to limit the extensions to image extensions.
And this creates a new discussion: Why only .png, .jpg, .gif? First of all .jpeg is a valid JPEG extension too but
::getExtension()
will return .jpg (in GDToolkit::getInfo()). And this leads to a next question: Why not all image types? Next type are already defined by PHP (I think in GD): IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_JPEG2000, IMAGETYPE_PNG, IMAGETYPE_SWF, IMAGETYPE_PSD, IMAGETYPE_BMP, IMAGETYPE_WBMP, IMAGETYPE_XBM, IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM, IMAGETYPE_IFF, IMAGETYPE_JB2, IMAGETYPE_JPC, IMAGETYPE_JP2, IMAGETYPE_JPX, IMAGETYPE_SWC, IMAGETYPE_ICO.Maybe not .SWF, but I see no reason why popular .BMP cannot be added. Anyway, should this be a followup? Especially for .JPEG extension?
Comment #21
claudiu.cristeaTurned green, any reviews?
Comment #22
claudiu.cristea#204497: Make the background fill color configurable where image styles result in blank space is blocked by this.
Comment #23
arunvs CreditAttribution: arunvs commentedNew patch with more extension
Comment #24
claudiu.cristeaThank you but the patch is not qualifying for rtbc.
Invalid indentation.
Invalid docblock. It shouldn't start with "+".
Generally, the extension issue needs more discussion and work. We need to handle the extension issue also in Toolkit (GDTookit). I will get back with a patch.
Comment #25
claudiu.cristeaRelated #2066219: Decouple image type from image extension.
Comment #26
claudiu.cristeaAssigning to me.
Comment #27
claudiu.cristeaPostponing till #2066219: Decouple image type from image extension.
Comment #27.0
claudiu.cristeaUpdated issue summary.
Comment #28
fietserwinStatus is still postponed while #2066219: Decouple image type from image extension is already a while in. But this is not bad, I even think this is good, let's now keep it postponed on: #2196067: Remove setWidth and setHeight from ImageInterface and #2168511: Allow to pass save options to ImageInterface::save.
BTW: if i look at the piece of code you expect to work, I get a good feeling about all the work we are doing to clean up the interface. ;) TRy to explain what this should do and why it should work to a newbie that you just explained about Dupal's image system and the idea that it supports multiple toolkits :(
There are a number of problems with that code:
- setResource(): this method is GD specific, while Image should be toolkit agnostic, but that is solved in [##2103621] if you know that you are working with GD, you can now do $image->getToolkit()->setResource(), otherwise you will have to create a "create canvas" toolkit operation.
- setWidth(), setHeight(): and automagically the width and height of the image are set? But that will be handled by #2196067: Remove setWidth and setHeight from ImageInterface.
The real problem is indeed in the image format you want it to save in and there we have #2168511: Allow to pass save options to ImageInterface::save for, but even then, as last fallback, using the extension of the target file is a good idea.
Comment #29
fietserwinAccording to [#32196067-10], not being able to start with a canvas, is a regression from D7. Tagging as such, priority is already major
Comment #30
mondrakeAfter #2244359: Lazy-load GD resource in GDToolkit it should now be possible to address this. Working on a patch.
Comment #31
mondrakeLet's see what bot thinks about this. Explanations later
Comment #32
mondrakeExplanations in the updated issue summary.
Comment #33
mondrakeComment #34
fietserwinI'm not very happy with the name but am also finding it difficult to come up with a good name. At the operation level, 'canvas' seems more appropriate. Here, at the image level, createNew or createCanvas seem better.
Another idea is to integrate this into the constructor: we only allow to pass in a source at constructor time, so we could (should!) also say that we only allow to start with a blank canvas at constructor time.
Mime type is not really an argument at creation time, it is a save option.
Transparent color idem and then only for gif. But to create a 'blank' canvas we should be able to specify the fill color, not only as rgb but as rgba! having a fully transparent value as default.
Even if we do not change all of the above: NULL can be passed as transparent color when called from load or one of the (GD) operations.
Except for logging, I have not found any reasons, so far, for operations to have knowledge of the image. Operations should call each other via the toolkit, not via the image. (Compare how ScaleAndCrop and Scale call other operations.) So to create a convenience method for logging only seems not correct and to advocate bad practice.
Why not $this->apply('set_new')? Why use image to call a method on yourself. Especially as I think, see above, that this should not be a method on Image, or should only be called once, directly after creating an Image object.
As said, I would prefer canvas (or create_canvas) and without mime type and preferably with a fill color not a transparent color argument.
So, no real errors, but am also finding it a bit difficult to be happy with these changes. Regarding defining mime type as a save option, would mean that this issue would become dependent on #2168511: Allow to pass save options to ImageInterface::save or that we are going to refactor that part in that issue (or a follow-up). I think I prefer to do the other issue first.
What do others think? (thus leaving to NR to get more reactions)
Comment #35
mondrakeComment #36
mondrakeThanks for review in #34, @fietserwin.
#1: changed (provisionally) to 'create_new'. Here I am mainly a) converting the current
createTmp()
into an image operation of its own, and b) enabling starting an image object from scratch through a core operation. A 'create_canvas' operation would mean to me getting into solid color background, gradients, patterns etc. I'd leave that to contrib. Same for the mime_type and transparent_color parameters - these are needed if we want to convertcreateTmp()
to an operation.#2: fine, reverted. IMHO, operations should throw exceptions rather than using the logger service, and let logging be done at toolkit level. But that can be a separate issue.
#3: I was 50/50 on that but fine, that's how ScaleAndCrop calls 'scale' and 'crop' operations, so let's be consistent.
Comment #37
Jelle_SComing from #2341251: Allow image effects to change the extension, and with #2340699: Let GDToolkit support WEBP image format in mind, we need to be careful using the PHP constants, as you mentioned for the .JPEG extension, but also because currently, PHP has no constant for WebP images, which is becoming very popular.
Comment #38
mondrake#37
I would leave the discussion on supporting additional image formats to a separate issue like e.g. #2340699: Let GDToolkit support WEBP image format. Here the latest patches are about allowing creating an image (of any currently supported image type) without having to read it from a file to begin with.
Comment #39
fietserwin#37: If I look at the problems we have with correctly supporting gif, I am more inclined to dropping support for gif instead of adding others. Internally, GD does not handle all formats the same, making it a nightmare to test and support all formats. E.g. internally GD works either with a truecolor image canvas or a palette color canvas and using transparency in either is not straightforwad either. In contrastt ImageMagick e.g. internally always works with 4 (16 or even 32bit) channels, handling all formats the same. Only upon saving, the format comes back into view.
BTW: We have added a change to D8 that allows the toolkits to define the supported formats, where formerly it was hardcoded into core. So it is very well possible to add support for formats in contrib, either by extending the core GD toolkit or by using another toolkit.
Comment #40
Jelle_S#39: I tried that with the Picture module (adding WebP support).
It means adding a new toolkit (extending the current GD toolkit), AND for each effect (convert, scale, crop, ...) creating that again as well, because those effects define a toolkit for which they work. Maybe Toolkits should be able to define a base toolkit and inherit the effects? (but that's probably for an other issue, I don't mean to hijack this one :-) )
Comment #41
fietserwinCopied comments 37 - 40 to #2340699: Let GDToolkit support WEBP image format. So, let's continue any discussion about supported image formats over there.
Comment #42
mondrakeRerolled
Comment #44
mondrakeFixed the test, the new test image 'image-test-no-transparency.gif' introduced in #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in is in any case reset to transparent via 'create_new'.
Comment #45
mondrakego bot
Comment #46
mondrakeChanged 'set_new' to 'create_new' in the issue summary.
Comment #48
fietserwinThe upper return NULL can be removed.
This class misses a validateArguments() that checks for:
- positive width and height
- valid and supported mime type
- valid color
When the validation has been added, this can be removed.
return false - true - false. The 1st if checks for failure, the 2nd for success. For me, this may be done in 1 if that checks for success.
same as above.
Does it makes (much) sense to test create_new as a real operation and have all test images converted to the same canvas?
Especially as further down we have separate tests for create_new.
IMO: remove the operation with the test images and extract the latter foreach (that specifically and only tests create_new) into its own test method.
Even when all my remarks are processed, this patch still buggers me, but I am not sure why.
- It may be due to another running issue that prefers extension(/format) over mime type,so we are getting a non-consistent use of image type, mime type and extension in the gd toolkit. (#2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect)
- Can we create a non transparant gif from scratch?
- Why is (GD) image processing so storage format based?
But this should not hold this issue. This (and the other mentioned issue) reveal deeper lying problems with the GD toolkit that we only discover here, now we are going beyond very simple processing like a crop or resize. So, I think I will open another issue for that.
Comment #49
fietserwinComment #50
mondrake#48 all done, thanks for the review.
Added also tests for failures of CreateNew, and changed the 'mime_type' argument to an 'extension' argument in line with what's being done in #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect.
Comment #51
mondrakego bot
Comment #52
mondrakeCouple of nits, sorry
Comment #53
fietserwin(Sorry for the late reaction, the issue got out of sight)
In principle, this is good to go, but to keep in line with the mentioned issue #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect, I propose to introduce a public method extensionToImageType() on GdToolkit. That method will already have 2 usages and the method supportedTypes() can remain protected. (We can wait for the other issue, or do it here as well and see which one gets in first.)
Comment #54
mondrakeReroll, after #2330899: Allow image effects to change the MIME type + extension, add a "convert" image effect, and changes as per #53.
Comment #55
fietserwinExactly what I was looking for. Thanks.
Comment #56
alexpottThis is looking good but we need a change record for this - or update the record(s) for the removal of image_gd_create_tmp
Comment #57
fietserwinCreated CR https://www.drupal.org/node/2356113.
Comment #58
alexpottThanks for creating the CR. The patch looks good - committed 7189de5 and pushed to 8.0.x. Thanks!
Comment #60
alexpott