Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.First of all I must say that I'm really not sure, if this issue is related to the ImageCache Actions module or if it is a side effect of the "problematic" default image handling of Drupal when using the private file system, but here goes...
After upgrading to the 7.x-1.0 version the default image of an image field is displayed with the wrong image size attributes. F.ex. when the default image is to be shown, the following HTML is generated:
<img typeof="foaf:Image" src="[correct path to default image]" width="20" height="20" alt="" title=""/>
When a "normal" image is shown - i.e. NOT the default image, it looks like this:
<img typeof="foaf:Image" src="[correct path to non-default image]" width="320" height="200" alt="" title=""/>
The latter HTML is correct, having the correct width and height attributes.
It turns out that the 20 pixels width and height comes from a padding action in the image style definition. All images are to be padded with a 10 pixel border on all sides, and if change the padding size to say 15 pixels, the default image tag gets a width and height of 30 pixels (2 x 15 pixels). The image style does only two things: A scale to 300 pixels, and then the mentioned padding, so it seems the system thinks the output of the first scaling action has dimensions 0x0 pixels. I am using GD2.
As I mentioned, I have big problems with these default images. They are not generated in the styles folder, so I have to generate (resize and so on) the images manually and copy them to the appropriate folder. I suspect this could have something to do with the issue I'm describing above.
If you have any suggestions as to what I can do to correct this, please, help me? :-)
Thanks!
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | ScreenHunter_73 May. 23.jpg | 32.23 KB | fietserwin |
| #6 | canvasactions_with_null_dimensions.1591198.2.patch | 1.15 KB | dman |
| #5 | canvasactions_with_nul_dimensions.1591198.patch | 735 bytes | dman |
Comments
Comment #1
dman CreditAttribution: dman commentedI'm not sure why image processes would be run directly on a 'default' (original?) image.
In most cases, the original image always stays unchanged, on file as the original, and all displays use derivatives as needed.
What setting have you configured that chooses to use an image style to process the 'default' ?
It sounds like at some point it is being told to read and write to the same file. Generally this would be a case to avoid.
The rest of your deductions sound probable - if an image process runs over an image object it finds invalid, then I guess a 0x0 may be created and then padded. However, I would have expected the system to be triggering or at least logging errors in such a case, not just going ahead on an invalid item.
Comment #2
jrhansen CreditAttribution: jrhansen commentedThanks for the quick reply! :-)
First, I had to take on the fight with the default image derivatives not being generated when using the private file system, so on my test site I changed all DB entries to point to public files instead, and now the derivatives of default images are created correctly.
I can see that I did not explain the original issue adequately. The derivatives of the default image are created correctly, but the HTML generated for displaying the derivates defines a wrong width and height. The psychical file in the styles folder is 320x320 pixels, but the HTML image tag states 20x20, which is the relative change done by the Canvas Actions module.
It turns out that I can make the whole thing work by copying "canvasactions.inc" and "imagecache_canvasactions.module" files from the 7.x-1.x-dev version dated 2012-01-12 to the 7.x-1.0 module and running "update.php". Replacing the original files from the 1.0 release results in the error come back.
So to answer your question, the derivatives are created correctly, and the image style is not applied directly to the original default image. Also, "normal" images uploaded to the specific image field work correctly - i.e. derivatives and the width and height attributes are created correctly with both the 7.x-1.0 and 7-x-1.x-dev versions. This issue affects the default image only.
Regarding error messages... when installing the new 7.x-1.0 version, I do get a warning message when running update.php:
Don't know if it is relevant?
Comment #3
dman CreditAttribution: dman commentedThanks for going to the trouble of trying a roll-back, and demonstrating that you can replicate the different results with different versions.
To clarify, when you mention 'default' - is this an image style label you created yourself?
Out-of-the-box image styles on a new install are (thumbnail, medium,large). Normally available displays are these plus "none (original image)"
I'm still unclear exactly what you mean by "the default image of an image field". When we check the display settings for content types with images, (Manage fields) you should choose a named image style. And (on a clean install at least) there is no style called 'default' until you create one of that name.
OH - do you mean you added a file field to a node and then set a 'default' value for it in the image field settings to show when you haven't uploaded anything there yet? Sorry, if that's it, it took me a while to figure that meaning out. I hardly ever use that feature, and I had to scan my entire drupal install for mentions one the keyword 'default' before I guessed this was what you may have meant. kinda obvious in retrospect I guess.
Steps to replicate:
* On a clean site, edit the filefield settings of the article content type (manage fields) and add a "Default image" where it says
"If no image is uploaded, this image will be shown on display."
* Enable imagecache canvas actions, and edit the 'large' style (normally shown on full view for the Article content type in "manage fields")
* Add a padding to the "large" image style, by using "define canvas"
* Now create two articles, one with, and one without an attached image.
Confirm that in full view, the one WITH an image attached uses the style with border as expected.
Confirm that in full view, the one without an attached image - which should show the placeholder "default" image with the modified style applied renders a version of that image, technically a properly-generated derivative image but with HTML dimensions that seem to match the amount of padding applied not the actual image size.
Hm.
Comment #4
dman CreditAttribution: dman commentedConfirmed that we don't see this side-effect when using other core image actions that would be expected to change dimensions - eg, use rotate not define_canvas.
Confirmed this does not seem to happen with other imagecache_actions processes like color shift.
It's hard to figure where this 20x20 html guess at the dimensions would be coming from, as the file is clearly available to be inspected for size if needed (theme_image usually does that itself) and OTOH this bogus dimension doesn't seem to be in the database.
... mnyah.
I trace it through to (core) theme_image_style() where I see a call to a function called image_style_transform_dimensions() . This in turn triggers our canvasactions_definecanvas_dimensions() function which adds the extra padding to ... well, to something incorrect, it would seem. It's being fed a null width and a null height.
So - that is what is returning the bogus calculation.
Tracing further, it seems that there are a few instances where code upstream may set dimensions to NULL. core rotate is one of them - you can trigger this error also by using core rotate to an odd number, then trying a define_canvas on top of that. This will effectively screw ALL image derivative HTML sizes, not just 'default' images.
It seems that filefield 'default' images also don't bother to feed us real domensions - which is where you picked up this problem.
Comparing our current code with the way core now does it, eg image_scale_dimensions(), it looks like we have to detect that invalid input ourselves and not even try to calculate sizes if we were not given enough data. Current code did not allow for the fact that we may be fed bad input - and that image module somehow thinks that's a legal thing to do.
core does have a fallback in theme_image that gets called at the end - and if the dimensions haven't been calculated in the lead-up to that point, it will go and check the real file. This is normally avoided for efficiency purposes, but our case looks like it should be - leave the dimensions alone if they were NULL to begin with - and let the fallback handle it.
Comment #5
dman CreditAttribution: dman commentedLooking at the 7.x-0.0 version - we didn't even use the canvasactions_definecanvas_dimensions hook at all. It didn't exist in previous versions - so you were ALWAYS seeing the "unknown dimensions - check the file instead" results.
7.x-1.0 has introduced support for this hook - to try and do things right. However we were unaware of the error conditions this hook needed to be able to handle.
A patch for this case - canvasactions only so far is attached.
Comment #6
dman CreditAttribution: dman commentedSlightly fuller patch - found another place : canvasactions_canvas2file_dimensions() : where this bug could surface.
A number of other actions - eg customactions - will probably never be able to predict size changes arbitrarily, so are currently deliberately triggering the fallback behavior. This is correct.
Remaining question is just ... how come 'default' images never gave us their size data in the first case? Take that up with image_field I guess.
Comment #7
dman CreditAttribution: dman commentedI misunderstood the behavior of "dimensions callback" a little.
Clarified in #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O - the results of 'NULL' are not calculated as a fallback like I assumed from earlier versions. They are in fact just "dunno" and the img is sent to the browser without any height or width.
This leads me to wonder if the current behavior of imagefield with 'default' images is by design. Looks like a small bug in imagefield to me.
In the normal case, even without imagecache_actions and define canvas involved - imagefield 'default' images have no height and width in the HTML img
The error we see in this thread grew out of the assumption that height and width would be available there - a reasonable expectation.
Comment #8
fietserwin1 remark:
It is true that the dimensions callback was rushed in, partly via issue #1214860: Imagecache Actions should use dimensions callback and dimensions passthrough. So very well possible that it still contains errors. But in this patch is a bit premature in failing if we do not get dimensions in. Because, if underlay will set the new dimensions to the background we do not have to know the original dimensions to know the resulting dimensions.
Make me think that if someone uses a define canvas as last effect to make all (e.g. randomly rotated) images the same size, we still cannot return that. So we could/should add some dimensions parameter to that effect as well (but that is a different issue).
Aside 1:
Warning: class_implements() [function.class-implements]: Class ImageCacheActionsModuleStreamWrapper does not existClear the (registry) cache after installing the 7.x-1.0 version. Until the (file that contains the) class is known to the class registry you will get this error message. Seems unavoidable as Drupal will always be quicker to see there is new stream wrapper (some hook) than it will be able to actually find it.
Aside 2 (aka "push my own work"): if you want more effects to properly implement the dimensions callback: please review issue #1551686: Image rotate dimensions callback can calculate new dimensions for every angle (and a bit related #1554074: image scale does not work when current dimensions are unknown).
Comment #9
dman CreditAttribution: dman commentedTrue - rather than bail-out, I guess it's possible to check if the canvas being defined is :
absolute size - which we can then define ourselves
or
relative size - which we can't guess and should return immediately from.
In much the same way that core 'rotate' currently does. Though I agree with you that rotate can fairly easily do its own maths much like the patch you linked to above does (I did see it earlier)
I did the same thing back in 2009 #372497: imageapi_imagemagick_image_rotate() doesn't update width and height in $image
The key part of this patch/issue highlights that "sometimes the system gives us NULL" there, and we need to start being able to handle that case that appears to be unanticipated.
Anyway, what we are looking at here is an edge case inside an edge case. NULL inputs only happen for 'default' images - which I now feel is an upstream bug, or if a previous process gave up on calculations.
In none of those cases should we be returning false dimensions of 20x20. It's better to fall back to "don't know" than to state a wrong answer.
In the case of "underlay" action setting the dimensions - then that should be able to do so with its own dimensions callback. If it did so, then define_canvas would be getting it right.
In the LONG run - I'd actually hope we'd have been able to do an end-run around #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O and behave like I incorrectly assumed it would - by checking the actual real dimensions (not guess) when it was really needed.
This would produce optimized guesses for 80% of the cases and some I/O work for the hard ones, giving us back D6 behavior. I'm annoyed at the idea that now my HTML will *sometimes* have sizes on images and sometimes not, based on shortcuts taken deep in the system.
Since discovering this,
I AM THINKING ABOUT
An imagecache_action stand-alone process that repairs dimensions. It does no actual action, but it provides a stand-in for a unique 'dimensions callback' that re-introduces D6 behavior. If the image dimensions are unknown, then find them out the normal way. By checking the damn file.
Placed at the end of an image styls chain only if really needed it would restore what I think is better behavior than the current situation with NULL.
This becomes an option that site admins can use or not, depending on their trade-off between performance and HTML validity. And would only affect edge cases anyway.
Comment #10
fietserwinI committed some changes that, I think, solve this issue. It handles the cases where NULL is passed in (instead of an integer value) and even then handles the case where the new dimensions can be known anyway.
There is also a remark in roadmap.txt about checking all effects against the image effect API, so this will be looked at in more detail in the future, hopefully with tests as well.
Setting to fixed here, the roadmap will remind us of the work to do.
Comment #12
greenmother CreditAttribution: greenmother commentedUnfortunately, the problem is not fixed. My action:
"Define canvas left:10 right:10 top:10 bottom:10 #e5e5e5 under image"
makes display of all images in 20x20 size.
Fixed it by this row in canvasactions_definecanvas_dimensions()
Comment #13
greenmother CreditAttribution: greenmother commentedComment #14
fietserwinStyle to reproduce:
I will have a look at it later today.
Comment #15
fietserwinFixed for the define canvas effect.
Furthermore:
- Added it for the subroutine effect (that effect still contains other errors though).
- Improved it for the aspect switcher (with absent or incomplete info we cannot know which style will be applied, but if both styles return the same dimensions (even with incomplete info passed in) we can use that).
Comment #16
dman CreditAttribution: dman commentedYep
http://drupalcode.org/project/imagecache_actions.git/commitdiff/18d997cc...
looks more robust that before.
As a code thing in PHP, would it be more correct to look for
rather than
(triple "=" comparison) ?
In our current, anticipated environment, it's almost certainly an error if we have a dimension of zero on an image (because duh)
But according to the comments and CHANGELOG (BTW,
thanksfor keeping a CHANGELOG up to date, I'm not good at that!)then comparing explicitly for NULL means just that.
Personally, If I'm looking for invalid/empty values (0 or NULL) I use empty() to make that clear.
implicitly matches 0 due to PHP silent casting - which can be opaque.
This is not a criticism, because a fix for this was clearly needed. It's a discussion point.
In my head, I'd read NULL to mean "dunno the dimensions yet" and 0 to mean "something returned an empty image, it's zero"
I still think there will be cases in image_effects where we cannot reliably pre-calculate the dimensions without fully going through the transform again (rotate was fun, rotate-random is impossible) - and those cases deserve to make use of the 'undefined, let the browser sort it out' option. Until we can store the generated dimensions in the file table somewhere for next time. Which AFAIK is not a thing yet, HOOK_dimensions is a kludge to work around that deficit IMO.
discuss?
Comment #17
fietserwinTo correct you on the first point: In PHP it is != versus !==. So the code is explicitly and only looking for NULL, not 0, '' or array().
The checking could be more relax (thus using != instead of !==) as an image dimension of 0 indeed probably indicates an error. But then: if I can calculate new dimensions based on a current dimension of 0, why not? Disadvantage may be that other effect dimension callbacks erroneously use "just an empty" value instead of NULL to indicate that they could not calculate the transformed dimensions.
As my background is in strongly typed languages (C++, C#), I prefer to work with rather strict type information. Therefore all the gradual updates to our PHPDoc (@param and @return) and, since recently, type hinting (fail early, fail fast, fail often). Using PHPStorm as IDE also aids in correcting type info and even typos in comments.
Storing dimensions of managed images is already done in the image table, but is not done for derivatives as:
- they are not stored in tables
- they may even not have been created yet when the html img tag is generated (and thus when the width and height attributes are needed).
So caching it somehow might be good idea, but the transform_dimensions() will still be be necessary for the first time a derivative is listed on a page (and thus before it is created).
Difficult stuff, that's for sure.
Comment #18
dman CreditAttribution: dman commentedI'm sorry - you are quite right. I'd had Friday beers and misinterpreted that comparison - not considering the leading "!" there making it into a different operator.
http://php.net/manual/en/language.operators.comparison.php
My confusion totally.