Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Follow-up to #2262863: Add srcset to template_preprocess_image
Problem/Motivation
- The new picture html tag allows to define alternatives based on mime type (image format) using the type attribute. To render this attribute, we need to be able to determine the mime type of a derivative without generating it.
- To be able to generate derivatives in multiple output formats, we need an image effect that can change the format an image is saved in.
Proposed resolution
- Add
ImageEffectInterface::getDerivativeMimeType(&$mime_type)
- Add
ImageEffectInterface::getDerivativeExtension(&$extension)
- Now that ImageStyle supports changing the MIME type & extension, add a "convert" ImageEffect
User interface changes
None
API changes
- Adds
ImageEffectInterface::getDerivativeMimeType(&$mime_type)
- Adds
ImageStyleInterface::getDerivativeMimeType(&$mime_type)
- Adds
ImageEffectInterface::getDerivativeExtension(&$extension)
- Adds
ImageStyleInterface::getDerivativeExtension(&$extension)
- Adds
ImageInterface::convert($format)
- (Adds
\Drupal\image\Plugin\ImageEffect\ConvertImageEffect
) - (Adds
\Drupal\system\Plugin\ImageToolkit\Operation\gd\Convert
)
Comment | File | Size | Author |
---|---|---|---|
#83 | interdiff.txt | 6.21 KB | fietserwin |
#83 | allow_image_effects_to-2330899-83.patch | 32.88 KB | fietserwin |
#74 | interdiff.txt | 16.88 KB | fietserwin |
#74 | allow_image_effects_to-2330899-74.patch | 30.98 KB | fietserwin |
#70 | i2330899-70.patch | 33.89 KB | attiks |
Comments
Comment #1
attiks CreditAttribution: attiks commentedComment #2
Jelle_SPatch with tests for the new method.
Comment #3
attiks CreditAttribution: attiks commentedLet's get this committed so we can resume work on picture
Comment #4
webchickHm. Why does this not have a function body? It does not in fact seem to be transforming any mime types? :)
Empty methods on the interfaces make sense, but this is a base class.
Comment #5
attiks CreditAttribution: attiks commentedBecause all image effect provided by core don't do anything with the MIME type, the same is with TransformDimensions and DesaturateImageEffect.
If contrib wants to add an Image Effect that transforms jpg to webP, it can do so.
Comment #6
RainbowArrayYes, TransformDimensions is empty in DesaturateImageEffect, but DesaturateImageEffect extends ImageEffectBase, where TransformDimensions is defined. Although I admittedly may be confused because TransformDimensions in ImageEffectBase is defined as:
That doesn't make a ton of sense to me, so what do I know.
I just don't understand why TransformMimeType isn't defined as changing one MIME type into another one in ImageEffectBase.
Comment #7
Jelle_SIMHO that doesn't make sense either. Currenty every single image effect provided by Core overwrites the
transformDimensions
method, meaning that image effects that don't affect the dimensions of an image overwrite it with an empty method so that$dimensions['width']
and$dimensions['height']
aren't set toNULL
. To me it would seem cleaner ifImageEffectBase
would just definetransformDimensions
as an empty method so that only image effects that affect the image dimensions have to implement it. (This is the way transformMimeType is implemented in this patch)Because the image effects provided by core don't actually transform the MIME type. If you pass them a JPEG, you will get a JPEG back.
Image effects that do transform it, should overwrite the
TransformMimeType
method ofImageEffectBase
.Comment #8
attiks CreditAttribution: attiks commentedAssuming all is explained well
Comment #9
alexpottWhy? Yes I can read the parent issue - but let's save everyone else the work of doing this.
Also I feel these transform* methods are poorly documented and named - they are about telling you what the applying the image effect will do WITHOUT applying it and interrogating the resulting image. The method names make it sound like they are doing the transformation which is not true. Let's do better.
Comment #10
attiks CreditAttribution: attiks commentedComment #11
attiks CreditAttribution: attiks commented#9 Good point about the naming of those functions, but they are based on the existing transformDiminsions.
What about getDerivativeMimeType?
Comment #12
Jelle_SMarked #2341251: Allow image effects to change the extension as a duplicate, since changing the MIME type of an image will also change its extension. Merged the two patches together and changed the method names to getDerivative*. I also adjusted the comments a bit.
Comment #13
RainbowArrayI took a close look through how the patch in #12 merged the patch in #2 with patch #6 in #2341251: Allow image effects to change the extension. It looks like a sensible merging of the two. Hopefully the improved comments and function names help to clear things up. I'm a little more clear at least.
On #2063373: Cannot save image created from scratch in comment #8, attiks asked if it made sense to get in #2063373: Cannot save image created from scratch first, but that was never resolved. That discussion then spiraled off to #2340699: Let GDToolkit support WEBP image format.
Since this is now the last blocker for getting Picturefill updated, could somebody clarify if getting #2063373: Cannot save image created from scratch in first is a nice to have or a must have? If it's a nice to have, but not necessary, let's look at moving this issue to RTBC so we can get it in and then get Picturefill updated. If there is a better way to create images from scratch, great, but I'm not sure that is a necessity to hold up the Picturefill update. It seems like that could maybe be handled in a follow-up.
Comment #14
attiks CreditAttribution: attiks commented#13 I think it is a nice to have, but it should not block the picture patches
Comment #15
mondrake#14 Agree, let's keep the two independent.
Comment #16
attiks CreditAttribution: attiks commentedNew patch fixing getPathToken
Comment #17
alexpott#16 no interdiff?
Comment #18
attiks CreditAttribution: attiks commented#17 Here you go
Comment #19
Jelle_SWe forgot to rename some test methods. New patch fixes that.
Comment #20
attiks CreditAttribution: attiks commentedSince mark tested this and it was RTBC before
Comment #21
attiks CreditAttribution: attiks commentedWe need to have a closer look at the tests
Comment #22
Jelle_SNew patch with added tests for ImageStyle.
Comment #23
Jelle_SIn the previous patch one assertion was still commented out (forgot to remove the comment slashes after testing). New (correct) patch.
Comment #24
mondrakeDo we really need these methods public and on the interface? If you need them for PHPUnit test, AFAIK, you can still mock them if they are protected.
Comment #25
Jelle_SOh, I thought they needed to be public to be able to mock them. I'll change it :-)
Comment #26
fietserwinI don't see the need for this and no I did not read the parent issue, so please explain the why in this issue summary. Explicitly explain the use case where you need it without generating it.
- For any use case where we need this info during or after generating it, issue #2168511: Allow to pass save options to ImageInterface::save is a far more generic solution.
- If there 's a use case for need of this info before generating it, like with the dimensions, I would like to propose to make the current transformDimensions into something more generic like transformImageInfo(). This so because I am afraid that in the next version of Chrome we can get 10% performance improvement if we add an attribute to the img tag that declares if an image contains transparency or not and so we add a method transFormTransparency(). 2 weeks later, Firefox announces support for feature Y that needs to know how many bits the color channels have and we add a method transformColorChannelDepthMeasuredInBits().
My proposal stabilizes the API methods, but still allows to pass information around not yet foreseen.
If needed, transformImageInfo() should be well documented with the info it currently expects and the base method should, indeed, alter nothing. Note that this is in contrast with the current implementation of transformDimensions().
Comment #27
Wim Leers#26.a: it's right there, ever since alexpott asked it in #9, it was added in #10: https://www.drupal.org/node/2330899/revisions/view/7648927/7648969
#26.b: No, because then you've made things *less* explicit. The structure of the array you'd be passing around then would be a D7-style "structured array" that we cannot possibly validate. In other words: terrible DX.
Is it 'jpeg' or 'jpg'? Does it matter? Where is this used? The docs don't tell.
What? Not clear at all.
Also: this is a *nested*
file_exists()
call. This effectively doubles the IO.1) the comment says "check", but it should be "append"
2) why append an extension, why not *change* it?
3) why no strict comparisoin?
Duplicate of the code above. Let's put that in a protected helper method then.
That will also allow for clearer documentation.
… to be set to the derivative's MIME type
s/Extension/The file extension/
Plus analogous to the above.
Again.
Completely different documentation compared to the above. Let's make it consistent.
Why do these need to be on the interface?
First time I see "resource"?
? Please improve this line.
We did this for a while, we no longer do this. Can be removed.
Nit: Should be only one newline.
Nit: s/id/ID/
This will cause problems for other tests, please don't include anything.
Extraneous newline.
Nit: s/uri/URI/.
This feature addition is not captured at all in the issue title!
Comment #28
Jelle_S1. Is it 'jpeg' or 'jpg'?
jpeg, jpg or jpe for Drupal Core (see next answers).
Does it matter?
Depends on the implementation of the Convert operation of the Toolkit. In the case of Drupal Core (GDToolkit) it can be jpeg, jpg or jpe (see \Drupal\system\Plugin\ImageToolkit\Operation\gd\Convert, which is also added by this patch)
Where is it used?
In the implementation of the Convert operation of the current Toolkit (again, GDToolkit for Drupal Core).
The docs don't tell.
Not sure how to document that in a comment?
2. buildUri adds the extension to the end (test.jpeg -> test.jpeg.png), this means that when the derivative is created, it will look for a source file with the name test.jpeg.png, which won't exist, so we strip the last extension and then it will look for test.jpeg, which will exist. That's also why there are two file_exists calls, but the second one will only get called if the image style converts the mime type.
3. See above explanation
15. file.inc is included because of this code in ImageStyle::buildUri():
Can we Unit Test this method without including it? Any suggestions?
18. Since ImageStyle now supports changing the MIME type & extension, we might as well add a Convert operation to the GDToolkit so that we can create an ImageEffect to convert between image types in Drupal Core. It also makes (manual) testing easier.
Comment #29
Jelle_SComment #30
Jelle_SComment #31
Wim LeersfileUriScheme()
,fileUriTarget()
andfileDefaultScheme()
wrapper methods on the class that call the procedural code, they can then be removed later on. Plenty of classes do this: e.g.drupalRender()
which callsdrupal_render()
.#29, #30: thanks, that helps.
Comment #32
Jelle_SYes:
Comment #33
mondrake#28.15: you can take cues on wrapping procedural code in a protected method here from the Image class. FYI at least
file_uri_scheme()
is being replaced by a OO equivalent in #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class, but that's WIPComment #34
fietserwinWell, I didn't know the details of the new picture element, so I rephrased it a bit more explicitly.
Comment #35
attiks CreditAttribution: attiks commented#34Th extensions are coming from the toolkit, it uses
\Drupal::service('image.toolkit.manager')->getDefaultToolkit()->getSupportedExtensions();
Comment #36
Jelle_SI think I got all remarks from #27, except #27.10: That docblock is a copy-paste from the other ImageEffects (CropImageEffect, DesaturateImageEffect, ResizeImageEffect, RotateImageEffect, ScaleAndCropImageEffect, ScaleImageEffect)
Comment #37
Jelle_SFYI: patch still applies. Anyone up for a review?
Comment #38
Dave ReidComment #39
fietserwinThis is only a review of the image/imagetoolkit part. The image style/effect part follows.
Missing @return.
Moreover, the format is not changed by the this convert operation but on saving the image. So, strictly speaking, something like "Instructs the toolkit to save the image in the specified format" is better.
See remark above.
$format is a supported extension (see validateArguments()), thus it is easier to just foreach through all supported types and use image_type_to_extension() (as in GdToolkit::geSupportedExtensions) to get the type.
validateArguments() assures that this cannot occur. (Unless you have a broken PHP installation, in that case it is better to fail fast and hard). So it is better to remove the checks (if you don't want to do it as suggested above).
Only the line setting the type is correct. The other lines do nothing but duplicating the existing resource:
- not gif -> gif: no transparent color was set and no transparent color will be set. If the original contained transparency and we want to retain that we must allocate a color to represent transparent and set all pixels that are currently (almost) transparent to that color.
- gif -> not gif: a transparent color could be set but will be lost as the new format does not work with transparency or works with a transparency channel, in which case we should make all pixels having the the color defined as representing transparent, fully transparent.
- gif -> gif: the transparent color will be copied to the new resource (exact duplicate): thus resulting in a no-op.
I think it is OK to loose transparency when coming from or going to gif, we do not want to make it to complex here.
So, we are only testing conversion to jpeg? The interesting cases are formats that support transparency, Please add these as well and we will notice that we have to better describe what happens with transparency with this operation (and tests are a way to specify behavior).
Comment #40
attiks CreditAttribution: attiks commentedComments fixed and a bit of code, will discuss tests with Jelle
Comment #41
fietserwinAnd the review of the imagestyle/effect part:
This will invalidate the error message in case there was no convert operation. Something like Source image at sample not found instead of Source image at sample.png not found.
I don't see the need for 2 methods here and on ImageEffectInterface. Extension and mime-type are interchangeable. We can go from one to the other using ExtensionMimeTypeGuesser or MimeTypeExtensionGuesser.
The only case where it can be useful to distinguish is to give the user the option to define the extension (JPG or jpg or Jpeg), but IMO we don't have to provide that feature here. Being able to change (and query upfront) the image format is the goal of this issue. So let's go for a specific not configurable extension per mime-type. In fact the GdConvert operation also does so.
Next, we have to decide what to store in the ImageEffect: mime-type or extension?
- Mime-type: change the operation to accept a mime-type instead of extension.
- Extension: change the picture module (or other users of this new feature) to do the mime type lookup based on extension, but do not bother image effects with that task.
FWIW: I Prefer to use extension in the image system, because we already have an getAllowedExtensions() method on toolkits over there. So a convert operation should have a parameter in the set of allowed extensions. That is more logical than introducing yet another concept in the toolkit layer to represent image format.
This is what I don't want in the image system but in the picture module. 1 line of code in picture can replace 2 methods on interfaces and about 60 lines of code in the image system!
Setting back to NW, though the tests may run for the changes posted in between.
Comment #42
fietserwinThis is a remnark on 1 of the changes in #40
Sorry, I was not clear with my remark. what I meant is that the execute() method should become something like:
Comment #43
attiks CreditAttribution: attiks commented#41
1/ Will fix it
2/ Adding both methods will make it easier to use the class, otherwise every module wanting use this class has to replicate code. The other reason is that there's is - like you said - that there's is no 1 on 1 mapping between extensions and MIME types.
Comment #44
attiks CreditAttribution: attiks commented#42 No I'm lost, you want to remove this as well?
Comment #46
Jelle_S#42, #44:
That code can't be removed (if you want the convert operation to keep functioning):
createTmp
creates a new resource of the correct type (e.g. for a conversion of gif to jpeg it creates an empty jpeg with the correct dimensions, for png and gifs it also determines the transparency, as you described in #39/6). Then we copy the old resource to the new (which can be seen as the actual conversion) and set the new resource as the toolkit's resource. ThesetType
method is merely informative and does not do any conversion by itself. We set it here so it is in sync with the actual resource that is set on the toolkit.Comment #47
attiks CreditAttribution: attiks commentedNew patch, discussed with Jelle
We added supportedTypes to the interface and made it public.
Comment #48
attiks CreditAttribution: attiks commentedConvert tests added for gif and png
Interdiff is against #36
Comment #49
mondrake#47 Hm. Fine to make that method public in GDToolkit so it can be used by GD operations, but why also on the interface? Other toolkits may decide not to use the PHP constants internally (see for instance this sandbox with a test conversion of Imagemagick). We already have
getResource(), setResource(), getType()
andsetType()
public in GDToolkit but not on the interface, can we follow the same pattern?Comment #50
mondrakeSorry, I must have xposted with #48 and the patch file disappeared. I can't seem to be able to restore it :(
Comment #51
attiks CreditAttribution: attiks commented#49 You're right new patch, also fixing #50
Comment #53
fietserwinRE #43.2:
We are talking either:
- 1 line in Picture
Or
- 8 lines in ImageStyleInterface
- 8 lines in ImageEffectInterface
- 9 lines in ImageStyle
- 6 lines in ImageEffectBase
- 9 lines in ConvertImageEffect.php
- 22 lines in ImageStyleTest
For a total of 62 lines for a feature that on top of this is also in the wrong place: the image system should not be concerned with mime types, see issue #2257163: Restrict image system to image processing.
Oh and yes there is a 1-to-n mapping between mime type and extensions, but we can leave that task up to the mime type guesser (as this patch also does).
So, PLEASE, let's keep the interfaces clean and lean: NW
RE #46:
- No, createTmp is there to create true color resources regardless the type. thus also on opening gif images we get a true color resource instead of a color palette resource. True color resources are in essence image format free. I have my doubts about the imagesavealpha() only called for png, but that would be a bug that you cannot solve here. The place of the convert image effect in an image style should not have influence on the end result.
- setType() should be called by createTmp() (this is a bug that you can solve here.
So, OK, for now let that call to createTmp in, I will file a follow-up to make GD less type specific.
RE #47:
- supportedTypes() cannot be in the interface as IMAGETYPE is a gd specific thing and will restrict all toolkits to support only those formats that GD can support (read: no webp support): NW. Even for PHP 5.5. GD webp support we might have to revisit this whole type thing, as GD forgot to introduce an IMAGETYPE constant for webp: NW
So you can make it public to allow access from an operation, because an operation knows which class it works for. BTW: the PHP doc states: Static properties cannot be accessed through the object using the arrow operator ->..: NW
Comment #54
fietserwinI see a file attached to my comment in #53 which I didn't post. d.o.seems a bit confused...
Comment #55
attiks CreditAttribution: attiks commented#54 To avoid further confusion, can you reroll the patch as you described?
Comment #57
mondrakeOne thought - sorry if it comes late.
In this patch we have a concept of 'format' which makes me to struggle with. Why? Because we already have the concepts of 'extension' and of 'MIME type' in the image system (let alone the GD internal 'type' represented by IMAGETYPE_* constants).
Everywhere in the patch it seems to me that the 'format' equals the 'extension' - this is what is presented in the image effect form, stored in the effect configuration, and what is passed from the effect to the toolkit operation.
Now, if this gets in, with the GDToolkit in its current status, our 'format' dropdown in the effect form will have
which is OK, it can match a concept of 'format'.
But if, later on, #2340699: Let GDToolkit support WEBP image format gets in, the 'format' dropdown will have sth like
and here the concept of 'format' would blur... one may say that JPEG, JPG and JPE are the same 'format' with different 'extensions'.
So, to cut short: my suggestion is to just have the concept of 'extension' internally throughout the code (names of variables and arguments), with maybe the convert effect to present in the UI a field titled 'Derivative image file extension' (or the likes) where we have now 'Format'.
Then, the cherry on the cake could be to have in the dropdown displayed to users both the extension and the MIME type, à la
Thoughts?
Comment #58
attiks CreditAttribution: attiks commented#57 Good thinking, I'll change the naming to extension.
Comment #60
attiks CreditAttribution: attiks commentedrenaming format to extension
Comment #61
Jelle_SRE #53
I don't think it should. createTmp() just creates the resource, it doesn't change the resource the toolkit instance is working with, so it shouldn't change the type based on the created resource.
Comment #63
attiks CreditAttribution: attiks commentedStrange error: PHP Fatal error: Call to a member function warning() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php on line 134
Comment #64
attiks CreditAttribution: attiks commentedgo bot
Comment #65
attiks CreditAttribution: attiks commentedfile
Comment #67
mondrakeRe #63
'jpg' is not (yet ;)) a supported extension, so convert's validateArguments throws an exception which the toolkit base class tries to log as a warning - but there's no logger object so you get that failure. Changing 'jpg' to 'jpeg' PHPUnit tests pass locally.
Comment #68
fietserwinThere are also fails in other tests, see the log.
@attiks: do you still want me to reroll the patch?
@mondrake: do you prefer mime type or extension/format, both here and in your patch in #2063373: Cannot save image created from scratch (I think both issues should use the same)?
Comment #69
mondrake#68 I'll be fine to use 'extension' on the other issue too. Yes, we should have consistency.
Comment #70
attiks CreditAttribution: attiks commented#67 Thanks for the pointer, new version attached
#68 Yes please, the goal of this issue was just to add a way to allow image extensions to change the extension/MIME type, you're right this has to be solved, but it does not has to happen in this issue. So for me it does not really matter how you want to solve this, but keep in mind that there's a beta tag, so only API additions are possible.
Comment #71
mondrakeComment #73
fietserwinOK, reroll coming soon.
Comment #74
fietserwinOK, I rerolled the patch from #70, so the interdiff is agains that patch. Most notable changes:
- Removed all mime type related functions. Consumng compoinentes can call the mime type guesser themselves on the result they get from getExtension().
- Changed the getExtension() methods to actually return the extension instead of passing by reference. This feels more natural given the name. (in fact, I think that transformDimensions should be adapted and renamed to getDimensions(), but that is for a follow up.)
- Extracted a part of the GDConvert::execute() method to a mehod GDToolkit::ExtensionToMimeType() so getSupportedTypes can remain protected and issue #2063373: Cannot save image created from scratch was doing the same int eh create_new operation introduced over there.
- Changes some comments and reduced them to 80 chars max.
Some notes:
- As expected (#39.6), some GDtoolkit tests still fail (I got 3 failures locally). This has to do with the way that GD handles transparency for various formats and how we handle image loading. so simply converting the extension and image format works, but what do we do with transparency when going from gif to png, etc. I will have another look at the tests to see if the tests need to be adapted or if the operation needs to be adapted. This should not stop others form reviewing.
- In ImageStyleTest::testGetDerivativeExtension() I get the idea that we are testing the mock method, not the method on ImageStyle itself. Can someone confirm or deny?
Comment #75
mondrakeI just cannot judge the implications of removing the mime type functions, so I'll leave to @attiks / @Jelle_S to comment.
From an image system point of view this seems ok, couple of points
1.
this can then be simplified:
2.
the assert should check for 'jpeg', not 'jpg'
Comment #77
attiks CreditAttribution: attiks commented#74, #75 I'm ok with it, nice job
Comment #78
mondrakeCouple more points
1.
At this point, why to pass the $extension as an input parameter? This method could just return NULL if no changes are needed, and a string with a new extension if any effect specifies it. It seems to me that the original extension is not needed in input.
2.
Please put the @see in a separate line so that docs can pick it up as a reference
Comment #79
attiks CreditAttribution: attiks commented#78 1/ it needs to be passed by reference, so it can go through all the effects. It is the same principle as the image dimensions.
Comment #80
mondrake#79 I see, but in #74 it's no longer passed by ref - should be reverted then?
Comment #81
Jelle_SSince you changed it not to be by reference anymore, this won't actually do anything. And also what @attiks said in #79
I can deny. We mock the imageStyle, yes, but a mock is just a class extending the class it is mocking. Since we didn't overwrite the getDerivativeExtension method, it is testing that method on the original class.
Also we add an image effect mock to the image style that changes the extension. Because what we want to test is that if there is an image effect in the image style that changes the extension, that the getDerivativeExtension method on the imageStyle returns the correct extension. It doesn't really matter if the imageEffect is a real one or a mocked one.
Comment #82
Jelle_SPS: We could start a discussion about the by reference passing of the dimensions and extension here, but since changing transformDimensions would be an API change and we are already in beta, it's a no-go. I think we should just be consistent (and IMHO, as @attiks explained it's the best or even only way to go) and pass the extension by reference as well.
Comment #83
fietserwinThanks all for reviewing, especially given that there were still failures.
#75.1: done
#75.2: done (I get the idea that unit tests don't run in the UI).
#77: OK, we stay with the current idea.
#78.1: I think that in #1826362: ImageEffects of the same image style should be able to pass variables between them we are going to add $style as well: if we want an image effect to "replay" what it is going to do, these methods need all the information that is available in the apply as well. E.g: contrib now has an aspect switcher: depending on if the derivative so far is portrait or landscape execute one or another action. If I want to create an effect that only changes jpeg to png but leaves gif as is (because I want transparency, not a specific format), I need the input.
#78.2: done
#79/#80: I solved the bug (see below), I stay with pass and return.
#81.1: Bug solved: return value is now used (did I already say that I think that unit tests don't run in the UI?)
#82: image is not a critical API, the only consumers are in image module/system itself and in contrib image effects. The latter being represented by me and @mondrake: if we say we want it, it might still get in.
Regarding the test failures in ToolkitGdTest: there was an error in the convenience method ExtensionToMimeType() I added. But more problematic is that the gif to png conversion (and vice versa) cannot handle transparency correctly. I propose that for now we accept this as a "documented feature" (the tests document it) as this might be very hard to solve, if solvable at all with GD.
i also solved another problem with expects($this->once()) in testConvert() in ImageTest. I reduced the number of tests to 1 as in the other tests. We don't need to test all possible (and correct) inputs, even if we can as in this case.
This should turn green (interdiff wrt to #74).
Comment #84
attiks CreditAttribution: attiks commentedThis is looking good, marking as RTBC. Thanks all for your help.
Comment #85
Dave ReidThis seems reasonable to me and good test coverage to back it up. The only thing to me was the 'addExtension' method which without reading the docblock was confusing to me. Once I understood what it actually did, it might have been better to just name it something like getPathWithExtension, but considering that path isn't really "stored" on the image style class, it makes sense the way it is. +1
Comment #86
fietserwinYeah, the term extension often leads to confusion #1308152-92: Add stream wrappers to access extension files :)
Comment #87
webchickOk, read this patch over in-depth and couldn't find anything to complain about. This also unblocks progress on the picture stuff, so...!
Committed and pushed to 8.0.x. Thanks!
Comment #88
fietserwinHmm, I don't see the commit in Drupal 8.0.x, can you check if there was a problem?
Comment #89
Jelle_SYeah, the code is nowhere to be found in the latest checkout either.
Comment #91
webchickUgh I am so sorry. This was the last commit of the day and d.o rejected it with Permission denied as it sometimes does. :(
Pushed now!
Comment #93
Gábor HojtsyFix media tag.
Comment #94
bendale CreditAttribution: bendale at Axelerant commentedHey everyone, I'm curious to know if there's a particular reason for keeping both the old extension and the new one when using the convert effect.
Comment #95
Chris Burge CreditAttribution: Chris Burge at Acquia commented@bendale - That's an excellent question. The source of the issue is potential namespace conflicts. Let's say you have image.jpg and image.png. If both are converted to webp, then you have an problem. I've dug into this very issue over the past week and should probably open a new issue against core. I don't have a solution, but I think it deserves its own issue for discussion.
Comment #96
Chris Burge CreditAttribution: Chris Burge at Acquia commentedOn a related note, this issue introduced a bug, #3412154: ImageStyleDownloadController::deliver() infers source image when generating derivatives with image format conversion, which would need to be addressed if we wanted to solve for replacing an image file extension instead of appending it when converting.