Problem/Motivation
D6 imagefield uses the $getsize parameter of theme_image() to populate the dimension attributes (width and height) of the HTML image tag. However, this involves calling getimagesize(). This means performing I/O for every image in every page request.
There are a number of problems with this:
- It's slow. Even checking a file exists on a local file system every time you serve it has performance issues. I have personal experience of dramatically speeding up a site by removing this sort of code. Syscalls are expensive and to be avoided as much as possible. This is why we switched from time() to $_SERVER['REQUEST_TIME'].
- If the image is in a directory that is mounted as a remote filesystem (very common) then performance is really appalling.
- If the image URI is a streamwrapper to S3 or rackspace then it isn't possible to read in the image at all.
- If the image hasn't been generated yet then it isn't possible to determine the dimensions.
When imagefield made it into core, it did not use the $getsize facility. $getsize was later removed from core completely: #908282: Remove unnecessary I/O from theme_image()
Proposed resolution
The patch in this thread takes a different approach that doesn't have the scalability problems of D6 imagefield:
- Dimension columns are added to the schema for image fields. These are populated when the image is added to the field. The D7 patch creates and populates these columns in an update function.
- Image effects provide an addition callback that just transforms the dimensions of the image. When theming a styled image tag, these transformations are applied cumulatively to the dimensions of the source image in the same manner as the actual image transformation. Unlike D6 imagefield, this works for styled images that are yet to be generated and also for images that are not being stored locally.
So you get the best of both worlds - no I/O is performed. It works with remote streamwrappers. The first time an image style is rendered it has the correct dimensions in the tag. This prevents the page from jumping around during AJAX image upload and after node save.
Remaining tasks
none
User interface changes
none
API changes
Image effects may now (optionally) supply a dimensions transformation callback. The patch updates the API documentation. The patch also has a test to ensure that effects with no transformation callback still work correctly.
Original report by jensimmons
If I inspect source on a Drupal 6 website (that's using filefield, image field, image cache, etc) I see HTML that looks like this:
<img src="/sites/example.com/files/something/kitten.png" width="500" height="350" alt="a picture of a cute kitten">
But, when I go to a Drupal 7 website, the height and width are not specified. :(
This is a performance bug. The page has to be rendered twice when no height and width are specified — the first time in drops images into a small space, calculates their height and width, and then renders the page once it knows their actual size. It's terrible.
Is this a regression bug? The data for height and width is right there. It's known. It's just not printed. We want to fix this as soon as possible.
Comment | File | Size | Author |
---|---|---|---|
#197 | 1129642_image_file_update.patch | 1.72 KB | aklys |
#183 | 1129642-fix-php-notices-followup.patch | 1.03 KB | adamdicarlo |
#174 | 1129642_174.patch | 2.01 KB | BTMash |
#173 | 1129642_173.patch | 2.01 KB | BTMash |
#172 | 1129642_172.patch | 2.01 KB | BTMash |
Comments
Comment #1
jensimmons CreditAttribution: jensimmons commentedRedndahead (on twitter) pointed me to this issue: #908282: Remove unnecessary I/O from theme_image() It looks like that's where this data was removed for D7.
Comment #2
Dave ReidI am very sure this was removed because we have to load the image with getimageinfo() every time just to get its height/width. With Image styles, we cannot always pre-calculate what the derived image size would be (e.g. scale & crop) - and we would still have to keep track of the original image's dimensions, which we also don't currently do. It would be great if image.module could do this without causing a performance regression, but that would be D8 material. I bet a contrib module could do it somehow.
Comment #3
jensimmons CreditAttribution: jensimmons commentedThis was also reported here #1012416: Image styles don't add width/height attributes — so it is a known WTFDrupal?
And this issue is a duplicate. But this is still a problem! So comment at #908282: Remove unnecessary I/O from theme_image() where people are debating which choice is the better one for performance.
Comment #4
jbrown CreditAttribution: jbrown commentedComment #5
Jeff Burnz CreditAttribution: Jeff Burnz commentedjbrown, shouldn't this be D8 material, surely love it be in D7.
Comment #6
jbrown CreditAttribution: jbrown commentedThe above patch stores image dimensions in the image field.
Image effects now expose dimension callbacks. These are used to populate the width and height attributes on the fly.
It even works for images that haven't been generated yet.
@jeff yeah it would have to be committed to 8 first and then backported to 7.
Comment #7
Dave ReidThis is changing the APIs, so I'm not sure it could be accepted for D7 at this point.
Comment #8
jbrown CreditAttribution: jbrown commentedIt doesn't change any APIs. Image effects can now expose dimension callbacks, but this is entirely optional.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedVery sweet patch!
I think a better logic would be to consider that if there is no dimensions callback for a style, the style of the derivative cannot be determined. That means adding a noop callback to use with the styles we have in core that do not change the dimensions.
Comment #10
jbrown CreditAttribution: jbrown commentedCorrect on both counts!
Comment #11
jbrown CreditAttribution: jbrown commentedComment #12
jbrown CreditAttribution: jbrown commentedImplemented Damien's suggestion and removed whitespace.
There is now a 'dimensions passthrough' key for effects that do not affect the image dimensions.
We need tests and I'm not sure how to upgrade field schema.
Comment #13
jbrown CreditAttribution: jbrown commentedChanged the logic so that calculation doesn't abort when an effect can't provide dimensions, e.g. Rotate. This means that crop can be used to add dimensions in a later effect.
Comment #14
jbrown CreditAttribution: jbrown commentedI wrote some tests.
Comment #15
Lars Toomre CreditAttribution: Lars Toomre commentedI stumbled across this issue as a result of issue #908282: Remove unnecessary I/O from theme_image() and spent some time looking at the patch in #14 as a result. As a whole, I very much like the addition of the dimensions and effects callbacks defined herein so that we can get height and width dimensions back into D7 image HTML tags.
In reviewing the patch, I had a few questions though. These included:
1) In the new function image_dimensions_scale(), you are making an assumption that $dimensions['height'] and $dimensions['width'] always exist. In the code, I see that these are coming from the stdClass object $image. This makes me wonder whether there should not be some isset($image->info[]) checking in image_scale() before the call to image_dimensions_scale(). This in turn makes me wonder how an error condition should be handled in the event that either info['height'] or info['width'] was not set.
2) The above item was sparked by wondering what would happen if the height and width dimension were unknown (ie NULL). I see that in image_dimensions_scale(), there is a possibility of a divide by zero error in setting $aspect. This deserves an if statement and opens the question of how this error condition should be handled. Being persnickety, we probably always should ensure that we are dealing with positive width and length as negative dimensions probably do not make sense in an image tag.
3) The $upscale variable name confused me at first. Perhaps a more descriptive name might be $allow_upscale?
4) The current version of image_scale returns TRUE if $upscale is FALSE and the desired dimensions are greater than the current image dimensions. Your implementation always calls the relatively compute-intensive image_resize() function, even if the scaled dimensions are greater than or equal to the current image dimensions. I do not think this is correct. We should mimic the current behavior in D7 if this is to have any prospect of being back-ported.
5) Reading through the tests, I was confused at first on why you set the height element in the image_scale $effect to a value of 90. I think that you are testing for whether the image aspect ratio is less than the desired ratio and hence, the width dimension should dominate the height dimension to keep the aspect constant in the resulting scaled image. If this is so, the code could use some in-line documentation. Do we also want to test for completeness when the height dominates?
6) I was surprised by the rotate test and its results. This may need to be opened in another issue too. Generally, I would agree that the dimensions of a rotated image are unknown. However, on certain edge cases (including the case you selected), the dimensions are known. I would have expected a 180 degree rotation alone would have had the same height and width dimensions as the original image in the resulting tag. Similarly, a rotation of 90 degrees alone would result in an exchange of the original height and width parameters and these too should be output in the resulting img tag. Hence, I think that this particular implementation is incomplete.
6a) We probably also need to add a test for a 90 degree rotation and scale effect to ensure that the resulting img tag also produces the desired result.
As I said at the top, this is a great start and I very much like the approach. I hope these observations make the resulting patch even better. Let me know if you have any questions. Cheers!!
Comment #16
jbrown CreditAttribution: jbrown commentedThanks for the patch review Lars!
1 + 2) We don't do this sort of thing. It's up to the caller to pass valid data. Also, changing this would be outside the context of the patch.
3) It is currently $upscale. Changing this would be outside the context of the patch.
4) Good catch! I have fixed this in the updated patch + added a test.
5) I updated the tests to testing scaling of both wide and tall images.
6) I have added a rotate dimensions callback that works when the angle is a multiple of 90.
6a) Done!
Comment #17
Lars Toomre CreditAttribution: Lars Toomre commentedI looked at the revised patch and see that you added quite a bit to this version. Good job!
Regarding 3) above, since you are creating a new function, I thought you could use $allow_upscale for clarity. However, it is not an important point.
Regarding 4) above, thanks for the modification. Both the function change and test for it look good.
Regarding 5) above, I like the enhanced tests for scaling. I might suggest an inline comment reminding readers of the code that the effect after rotate is cumulative. It was not clear at first how you were getting the 45x60px results. Perhaps using an effect value of other than 120 would be less confusing?
Regarding 6) above, thanks for the rotate callback for when the angle is a multiple of 90. Reading through the tests, they look good as well.
I have not applied this patch so I am unable to RBTC and unable to comment on any performance implications. However, from a logic perspective, it appears that the height and width tags for images are now being set correctly.
Comment #18
jbrown CreditAttribution: jbrown commentedI added the comment recommended in 5 and broke out a function called image_style_transform_dimensions().
As I understand it, update code doesn't go into D8, but rather in the D7 backport.
Anything else? RTBC?
Comment #19
aspilicious CreditAttribution: aspilicious commentedSrry for being anoying
Can this be written on one line only? (less than 80 chars?)
Determines
Powered by Dreditor.
Comment #20
MustangGB CreditAttribution: MustangGB commentedSubscribe
Comment #21
jbrown CreditAttribution: jbrown commentedComment #22
Eric_A CreditAttribution: Eric_A commentedNoticed a documentation issue...
-wrap at 80
-upscalled/upscaled
A Boolean/A boolean (Not sure about this. There's one more if it needs to be changed.)
Comment #23
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedsubscribing
Comment #24
jbrown CreditAttribution: jbrown commentedThose comments for image_scale_dimensions() were just copied and pasted from image_scale_effect().
Updated patch fixes it for both functions.
Comment #25
Eric_A CreditAttribution: Eric_A commentedYou've fixed the source and one copy, but missed the second copy...
Comment #26
jbrown CreditAttribution: jbrown commentedAh!
I have now fixed that one and the place I copied it from.
Comment #27
jbrown CreditAttribution: jbrown commentedI improved the patch so it now tests that an effect without a dimensions callback unsets the dimensions.
This ensures that any contrib modules for 7.x that have not implemented dimensions callbacks (because the API didn't exist) will not cause img tags to have incorrect dimensions.
Comment #28
TimG1 CreditAttribution: TimG1 commentedSubscribing.
Comment #29
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedGuys I really don't understand why this was removed just for a few I/O. Really the majority of performance issues in Drupal, but in any software more generally involve compute-bound application bottlenecks, not I/O-bound or net-work-bound performance.
Comment #30
jbrown CreditAttribution: jbrown commentedThese are the problems with reading in the dimensions from the image every time it is served:
With my patch above you get the best of both worlds - no I/O is performed. It works with remote streamwrappers. The first time an image style is rendered it has the correct dimensions in the tag. This prevents the page from jumping around during AJAX image upload and after node save.
Comment #31
jbrown CreditAttribution: jbrown commentedUpdated the patch so it doesn't read dimensions of all the images in the field every AJAX upload / field save. This makes a big difference with remote streamwrappers.
Comment #32
scott.whittaker CreditAttribution: scott.whittaker commentedThis patch fails for me on image.module and image.test on D7. I'm guessing this is a D8 patch, and a D7 version won't be coming till it's done?
Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedCPU is scalable. IO access isn't.
Comment #34
jbrown CreditAttribution: jbrown commented@scott.whittaker in what way does it fail? You know you have to create the image field after patching for it to work? There is no upgrade code yet.
The patch works on both D7 and D8.
Comment #35
jbrown CreditAttribution: jbrown commentedThe patch works fine on both D7 and D8, but the D7 version needs a db update function.
Anyone want to RTBC this for D8?
Comment #36
jbrown CreditAttribution: jbrown commented#31: populate-image-width-height.patch queued for re-testing.
Comment #37
johsw CreditAttribution: johsw commentedWorks fine here for new imagefields, but I don't think I have the in-depth understanding of the patch to RTBC it.
Comment #38
jbrown CreditAttribution: jbrown commented#31: populate-image-width-height.patch queued for re-testing.
Comment #39
jbrown CreditAttribution: jbrown commentedComment #40
geerlingguy CreditAttribution: geerlingguy commentedYeah, this'd be awesome; I always hated how that worked in D6.
Comment #41
Fidelix CreditAttribution: Fidelix commentedBrilliant work, jbrown! I hope this gets in D7.1.
Comment #42
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedIn your test cases, you should add the RDFa module as a dependency, otherwise the tests fails.
Comment #43
jbrown CreditAttribution: jbrown commentedSylvain: can you list the steps required for the tests to fail?
Comment #44
jbrown CreditAttribution: jbrown commentedSylvain: DrupalWebTestCase defaults to using the standard install profile, so it isn't necessary for it to be a dependency.
Comment #45
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedOh I see, not actually tested but you was using type="foaf:Image" which is a RDFa descriptor.
Comment #46
dropbydrop CreditAttribution: dropbydrop commentedwhen drupal 7 will print image width and height at html alt attributes?
Comment #47
pillarsdotnet CreditAttribution: pillarsdotnet commented#31: populate-image-width-height.patch queued for re-testing.
Comment #48
Fabianx CreditAttribution: Fabianx commentedReviewed the code, tested it and it works fine.
Two caveats though:
* What about the upgrade path to fix existing image fields?
** Works out of the box with drupal 8 installation with patch already applied.
* Even after deletion and creation of a new image field, the cache needs to be cleared.
** drush cc all does the job.
Great Work!
This is really an important patch as it'll allow Drupal to pass YSlow and Pagespeed tests again.
Best Wishes,
Fabian
Comment #49
jbrown CreditAttribution: jbrown commentedI need to write an update function for the D7 patch - this isn't required for D8.
Comment #50
Fabianx CreditAttribution: Fabianx commentedI need to write an update function for the D7 patch - this isn't required for D8.
Ah, sure. Good catch :-).
Best Wishes,
Fabian
Comment #51
RobW CreditAttribution: RobW commentedSub. There's an issue with webkit browsers and javascript where the image dimensions are usually not available at document ready. I had always gotten around this by using the width and height attributes in D6 - it will be great to get these back in D7.
Comment #52
RobW CreditAttribution: RobW commentedActually, I'm having a lot of trouble patching using #31 on D7. Ftp the patch to drupal base directory, run
git apply -v populate-image-width-height_10.patch
, and I get the errorsSo it seems to be stripping the first directory off each file path. If anyone sees what I am doing wrong and want's to point it out, I'd appreciate it. I'll manually patch to test tonight.
Comment #53
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled #31 for 8.x and 7.x using
git format-patch
. No code changes, so leaving as RTBC.Comment #54
jbrown CreditAttribution: jbrown commentedI added the database update functionality to the D7 patch.
Comment #55
RobW CreditAttribution: RobW commentedD7 patch in #54 tested:
Comment #56
jbrown CreditAttribution: jbrown commentedHere are the latest patches (they are -p1).
@RobW I am unable to replicate the problem with user created styles - could you document some steps to demonstrate the bug.
Comment #57
RobW CreditAttribution: RobW commentedDid some testing today on a fresh install with the patch in #56. It looks like the bug I was experiencing is caused by the image style "rotate" effect.
So:
When reporting in #55, all my custom styles were rotated 45 degrees. P.S., the latest patch applies for me with no errors, my git problem in #52 was probably something odd in my previous sandbox.
Comment #58
jbrown CreditAttribution: jbrown commentedThanks Rob!
This is all by design - if the rotate isn't a right angle then it isn't possible to predict exactly what the image size will be. Different toolkits would probably produce different results. Typically you would add a crop after the rotate to get a consistent image size (and populated dimension attributes).
The reason you couldn't apply my earlier patches is because they were -p0 patches instead of -p1. See http://groups.drupal.org/node/140204
Comment #59
RobW CreditAttribution: RobW commentedNo problem, thank you for for the patch, explanation, and patch thread link.
Comment #60
aspilicious CreditAttribution: aspilicious commentedReading this comment it is not clear when you return false or true. Thats why the coding standards tell you to specify what is true and what is false.
For example:
* @return
* TRUE on success, FALSE on failure.
Same as above
Powered by Dreditor.
Comment #61
jbrown CreditAttribution: jbrown commentedComment #62
jbrown CreditAttribution: jbrown commentedComment #63
jbrown CreditAttribution: jbrown commented#62: populate-image-width-height.patch queued for re-testing.
Comment #64
Fabianx CreditAttribution: Fabianx commentedI reviewed the code again in #62 and tested the functionality again for D8 and it works great!
Also could find no other occurence of "TRUE or FALSE".
Nice work!
=> Back to RTBC
Best Wishes,
Fabian
Comment #65
jbrown CreditAttribution: jbrown commented#62: populate-image-width-height.patch queued for re-testing.
Comment #67
jbrown CreditAttribution: jbrown commentedReroll.
Comment #68
pillarsdotnet CreditAttribution: pillarsdotnet commentedRTBC as before.
Comment #69
jbrown CreditAttribution: jbrown commentedSummary
D6 imagefield uses the $getsize parameter of theme_image() to populate the dimension attributes (width and height) of the HTML image tag. However, this involves calling getimagesize(). This means performing I/O for every image in every page request.
There are a number of problems with this:
When imagefield made it into core, it did not use the $getsize facility. $getsize was later removed from core completely: #908282: Remove unnecessary I/O from theme_image()
The patch in this thread takes a different approach that doesn't have the scalability problems of D6 imagefield:
So you get the best of both worlds - no I/O is performed. It works with remote streamwrappers. The first time an image style is rendered it has the correct dimensions in the tag. This prevents the page from jumping around during AJAX image upload and after node save.
Comment #70
AdamGerthel CreditAttribution: AdamGerthel commentedThe patch in #67 isn't applicable to D7.2 - Will there be a working solution for this for D7?
Btw, great work put in here @jbrown
Comment #71
pillarsdotnet CreditAttribution: pillarsdotnet commented@#70 -- See Drupal core code freeze, code thaw, issue count thresholds.
Comment #72
jbrown CreditAttribution: jbrown commentedThe D7 patch in #67 applies to 7.x-dev. It will apply to 7.3 released Wednesday.
It would be good to get this committed straight after 7.3 is released (right at the beginning of the cycle).
Comment #73
quicksketchI did this same thing in the very original Image module patches, providing functions for adjusting the expected width/height after operations. The problem I ran into is what to do about "random" rotation levels, but this patch takes an excellent approach and basically just says "if we don't know what the dimensions will be, remove them", which is a great idea.
Minor code style gripes: Every single function, if, and foreach statement added all have an extra line for no real reason. New lines can be added for clarity, but adding a new line after every function declaration or IF statement is not standard for Drupal core.
Regarding the D7 update:
This is incorrect. You should be using
field_info_field()
to get the schema for a field (in the event that the field is storing data in a non-standard SQL storage mechanism). Not sure what would happen for non-SQL data stores, but that's probably out of scope for this issue.This should be using the $context parameter in the update and doing this update through BatchAPI. Users could have hundreds of thousands of images.
Functionality-wise, this looks and works great. Awesome!
Comment #74
mattjacobson CreditAttribution: mattjacobson commentedSo first off, I am a relative Drupal Newbie. That said, this was my first patch, so take it easy on me.
The reason why I applied it was because of the (aforementioned) bug in webkit browsers not properly rendering images without the given sizes specified.
The patch seemed to work when I applied it (no real errors that I saw), and it fixed my problem on the website (running 7.4). As a result though, I had to destroy and recreate several existing image-fields. (Is there a way around that in the future?)
Everything worked, except now I get this error when trying to load several images: "Fatal error: Call to undefined function image_scale() in /[...]/modules/image/image.effects.inc on line 114" (More specifically, the images are broken, and that error is given when I try to load the image url directly.)
Thanks! Also, is there any timeline for this being back ported and becoming a part of Drupal core?
Matt
EDIT: it seems that it is definitely my image.effects.inc file, as any attempt to modify the image styles brings up the same error. Does anyone else have this problem / anyone with working (7.4) code willing to share the file so I can compare it against mine?
Comment #75
slackrunner CreditAttribution: slackrunner commentedThis patch did not work for me on a fresh install of Drupal 7.4. Height and width are not added to the img tags. mattjacobson can you verify that height and width are being added to the img in question? I assumed the patch worked at first, but it turns out that Safari/Chrome display the images correctly if they are cached.
Thanks
-SR
Comment #76
VisualFox CreditAttribution: VisualFox commentedSubscribe
Comment #77
quicksketch@slackrunner: Make sure you run update.php after applying the patch, as Image module needs to go through and calculate height/width for all existing images.
@mattjacobson: The patch seemed to work great for me and I tested it just 2 days before 7.4 was released, perhaps the entire patch did not apply properly? Regarding timeline: as soon as the changes are made in #73, this can be applied to Drupal 8 and then applied to Drupal 7. If things move quickly on this it will be included in the next release of Drupal core (the last Friday of July).
Comment #78
jbrown CreditAttribution: jbrown commentedThanks @quicksketch - I've almost got the changes implemented.
Comment #79
slackrunner CreditAttribution: slackrunner commented@quicksketch, thanks! It was as simple as running update.php
Comment #80
catchJust found this, looks great!
Non core field storage engines will need to copy the update code from here most likely, tagging as 'schema change' so that gets added to update docs.
Lack of proper batch updates for entity/field values is really painful but it's not the job of this issue to fix that.
Comment #81
quicksketch@catch: Does that mean that using field_info_field() as I suggested in #73 is unnecessary (or even undesirable)? If we're only responsible for updating the schema of the core Field SQL Storage module, what's the common way to prevent these updates from running against tables that don't exist?
Comment #82
catchSorry I'd missed that #73 was talking about non-core SQL storage engines.
The patch has this already:
That looks fine to me.
I wonder a little bit if field_sql_storage should be doing this update, that is possibly a can of worms we don't want to open here though.
Comment #83
jbrown CreditAttribution: jbrown commentedI've implemented the changes suggested in #73.
field_read_fields() works much better than _update_7000_field_read_fields() as it leaves out deleted fields and has the updated schema.
I added a constant called IMAGE_UPDATE_7002_IMAGES_PER_PASS.
Comment #84
jbrown CreditAttribution: jbrown commentedMinor improvement to the D7 version...
Comment #85
catchYou can't use API functions in updates, that includes field_read_fields(), see #1134088: Properly document update-api functions.
Comment #86
jbrown CreditAttribution: jbrown commentedDamn - I forgot about that!
@quicksketch recommended in #73 that I use field_info_field(), but this is also an API function.
_update_7000_field_read_fields() doesn't include the column names for the new columns. Should I go back to determining column names via concatenation like in #67?
Is this safe?:
Comment #87
jbrown CreditAttribution: jbrown commented#83: populate-image-width-height.patch queued for re-testing.
Comment #88
jbrown CreditAttribution: jbrown commentedOkay - it doesn't use API functions anymore in the update.
Comment #89
adamdicarlo CreditAttribution: adamdicarlo commentedSubscribing.
Comment #90
jbrown CreditAttribution: jbrown commentedAnyone want to test this?
Comment #91
bt82 CreditAttribution: bt82 commentedi just did and i can confirm it is working with D7.7... thanks
Comment #92
OnkelTem CreditAttribution: OnkelTem commentedThanks a lot jbrown! It was a long long reading and finally I see the patch ) Look forward to apply it succsessfuly against 7.7!
UPD. WORKS!
Comment #93
AdamGerthel CreditAttribution: AdamGerthel commentedI applied the patch to D7.7, and ran update.php on an existing 7.4-based site with D7.7 as well as the patch applied and got an error (see screenshot).
Width and height was not added to images.
Comment #94
OnkelTem CreditAttribution: OnkelTem commentedLooks like this patch doens't populate attrs of IMGs of Imagecache presets having only "Scale" action.
Drupal 7.7.
Comment #95
jbrown CreditAttribution: jbrown commented@OnkelTem did you run update.php ?
Comment #96
jbrown CreditAttribution: jbrown commented@AdamGerthel I think there is something specific to your configuration that is causing this to happen. Can you provide as much information as possible about your setup? Are you using non-sql field storage?
Comment #97
OnkelTem CreditAttribution: OnkelTem commentedSure.
I found the problem - it was Colorbox's
theme_colorbox_image_formatter(...)
which is to be patched too to reflect new attributes of images.Trivial patch for Colorbox is attached.
UPD. Colorbox issue http://drupal.org/node/1237160 with patch.
Comment #99
jbrown CreditAttribution: jbrown commentedcolorbox is not part of core
Comment #100
OnkelTem CreditAttribution: OnkelTem commentedOk, edited previous post, pointing ppl to the proper place. Sorry.
Comment #101
AdamGerthel CreditAttribution: AdamGerthel commented@jbrown I'll try:
This is a rather big project involving Commerce and a lot of other contrib modules. The image field is a core type image field. Nothing special about the display really. The image style used uses Scale Height 400px. Everything is packaged as Features.
I'm gonna make a new go at it to see if I still get the same error.
Update: Still the same error.
Is there any specific information you'd like to have? It's kind of difficult to describe the whole site.
After the failed patch I get this error (when logged as user 1).
Update 2: I now see that width and height is being added to images that use cropping. The ones that use scale do not get any width/height.
Comment #102
OnkelTem CreditAttribution: OnkelTem commentedScale needs to know original image's dimensions. Are you using Colorbox, Fancybox, Lightbox? All of their formatters must patched to provide original dimensions of images when they call theme_image. See my patch.
Comment #103
AdamGerthel CreditAttribution: AdamGerthel commented@OnkelTem no. I'm using the standard image display formatter.
Comment #104
OnkelTem CreditAttribution: OnkelTem commentedMy guess is that
image_style_load()
in your case returned FALSE on some reason, while jbrown's patch oftheme_image_style()
doesn't check this, passing FALSE toimage_style_transform_dimensions()
awaiting array:Anyway, you need to know, why loading style fails. Maybe recreate preset?
Comment #105
Fabianx CreditAttribution: Fabianx commented@OnkelTem
I believe you are right about the problem.
@AdamGerthel
Could you check if your db scheme does correctly hold the width / height information and it is populated, too?
There maybe still some glitches in the update part.
Does it work on new imagefields you create / new images you upload?
Best Wishes,
Fabian
Comment #106
Dean Reilly CreditAttribution: Dean Reilly commentedSubscribing
Comment #107
AdamGerthel CreditAttribution: AdamGerthel commentedI tried the patch again, and didn't get any error messages this time. Like previously, it only works with presets that use scale & crop or crop. Images that only use scale don't get width/height.
Comment #108
OnkelTem CreditAttribution: OnkelTem commentedTo make things clear again:
* scale & crop by its nature needs no info about the original image w/h to calculate resulting w/h - they equal to the preset's.
* scale does need original image w/h to calculate resulting w/h.
theme_image() from theme.inc gets this information via $variables array. The lack of w/h attrs means probably only one thing - theme_image() didn't get w/h in params. To solve this, you need to know who called theme_image() w/o w/h.
UPD. If you have no IDE with debugging properly configured you can install Devel module and place this line in the top of the theme_image() function:
See how the theme_image() was called (args will be printed in the output either).
Comment #109
jbrown CreditAttribution: jbrown commentedThanks everyone!
Updated patch to fix problem in #101 / #104. It would only occur if you tried to render an image with a style that did not exist.
@AdamGerthel if you want to hold up this patch you need to write a proper bug report (including steps to reproduce). Please read http://www.chiark.greenend.org.uk/~sgtatham/bugs.html
Comment #110
AdamGerthel CreditAttribution: AdamGerthel commentedEdit: nvm tried the wrong patch
Will see if I can get the time to reproduce the problems properly on a clean install
Comment #111
quicksketchThis patch looks good but I've got a few small comments that would be great to take into consideration:
I think it would be better to have $element['width'] and $element['height'] always available even if #value is NULL. That would make variable checking seem a bit more sensible (checking $element['width']['#value'] instead of checking if $element['width'] exists). Something like this instead:
This bit of code could use adjustments so that only height and width are passed into image_style_transform_dimensions(). Otherwise the various dimensions callbacks could end up blowing away properties in $variables unrelated to dimensions because it's passed by reference into image_style_transform_dimensions():
Lastly, the IMAGE_UPDATE_7002_IMAGES_PER_PASS constant is only used within one function (
_image_update_7002_populate_dimensions()
), so there's no real reason for it to be a constant. Instead just make $images_per_pass a variable in _image_update_7002_populate_dimensions().Comment #112
jarrodirwin CreditAttribution: jarrodirwin commentedsubscribing
Comment #113
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedSubscribe
Comment #114
MustangGB CreditAttribution: MustangGB commentedTested #109 on D7
- width and height attributes did not appear on any of my images- clearing the cache presented the following notices (several times):
Edit: Fixed by running update.php
Comment #115
jbrown CreditAttribution: jbrown commenteddid you run update.php ?
Comment #116
MustangGB CreditAttribution: MustangGB commentedThanks jbrown, working perfectly now
Comment #117
jamesbenison CreditAttribution: jamesbenison commentedThis patch seems to be working well. Is this going to ever get into drupal 7 core, or has the code freeze ruled that out? It has been several months now.
I just started working on something related to adding attributes/links to images based on the (abandoned?) Link Image Field module. If height and width aren't going into d7 core, this might be a good way to get it added. We could call it 'Image Fields Extra' or something. A link to the sandbox code is here: http://drupal.org/sandbox/jamesbenison/1275344
It would be great to get maybe some direction and/or a little help on this. It is basic and badly needed functionality.
Comment #118
pillarsdotnet CreditAttribution: pillarsdotnet commentedTagging; issue summary needed. Would jbrown please copy his excellent summary from #69 to the issue summary, using the issue summary template as a guide?
Comment #119
jbrown CreditAttribution: jbrown commentedThere are some further improvements I need to make. I should have time at the weekend to work on this.
Comment #120
idflood CreditAttribution: idflood commentedsub
Comment #121
jbrown CreditAttribution: jbrown commentedOkay everybody - try these!
Comment #123
jbrown CreditAttribution: jbrown commentedComment #124
Bikkne CreditAttribution: Bikkne commentedHi, don't know if this information is relevant for Drupal 7 but it might...
I tried your latest patch @ #123 on my site:samuitv.com and it works wonders making the "Views jQFX Nivo Slider" work in webkit browsers on an install with Drupal 7 with the latest modules and updates.
Issues: After patching, it doesn't matter if I update,clear cache or run cron, on the tabs on the first page, I need to click through all of them, because they give an error message the first load "field_blablabla_width" etc.
After clicking them through, no errors come up on other browsers even on the first load. The tab pages are showing different view blocks with pictures, I haven't seen any errors on "normal nodes".
After patching the files, I can't create more nodes, drupal throws errors, continuing log after what I paste here:
------------------------
PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_gallery_width' in 'field list': INSERT INTO {field_data_field_gallery} (entity_type, entity_id, revision_id, bundle, delta, language, field_gallery_fid, field_gallery_alt, field_gallery_title, field_gallery_width, field_gallery_height) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5,
------------------------
To add or edit nodes now I change the patched files back to the original ones in maintenance mode, do the work and then put the patched ones back. This is the only solution I found to make the slider work in Webkit browsers so far :)
Great work, keep it happening!
Comment #125
pillarsdotnet CreditAttribution: pillarsdotnet commentedYou need to add the missing "field_gallery_width" field your "field_data_field_gallery" table.
Alternatively, you can delete all nodes that use that field, remove the node type that uses that field, remove the field, then re-create all of the above.
Meanwhile, the patch under review obviously needs to implement an update hook that does this sort of clean-up. Marking as "needs work" unless someone can demonstrate that the problem is unrelated.
Comment #126
jbrown CreditAttribution: jbrown commented@samuitv did you run update.php ?
Comment #127
Bikkne CreditAttribution: Bikkne commented@ pillarsdotnet: Thanks for the info, I will check out the first option you mentioned.
@ jbrown: Yes, I ran update.php several times, read earlier in the thread that it could make issues go away. Anyway, while running the update.php it didn't ask to do any update, just go back to front page or administrator page.
Comment #128
jbrown CreditAttribution: jbrown commented@samuitv the update needs to run or it wont work - did you install the D8 version of the patch by accident?
Comment #129
Bikkne CreditAttribution: Bikkne commented@jbrown, @pillarsdotnet:
Damn :) you're right, excuse me, I missed the file under the bright green line, need to improve my cognitive perception, I'll try the other file and report back.
Comment #130
pillarsdotnet CreditAttribution: pillarsdotnet commentedBack to "needs review" then.
Comment #131
Bikkne CreditAttribution: Bikkne commentedTruly sorry for the inconvenience, I've now changed to the other patch and updated the site, it all works, I can edit nodes and save without error and the images display correctly even in Konqueror which had the most difficulties displaying the right layout before patching (flawless results also in Opera, Chromium, rekonq and Firefox). I don't have IE so I haven't checked that browser. I created unnecessary job for myself with the wrong patch, I hope I didn't create too much of the same for you guys.
Thanks again for a great job done !
Comment #132
jbrown CreditAttribution: jbrown commentedThanks for testing!
Comment #133
Bikkne CreditAttribution: Bikkne commentedSmall question: While updating Drupal core in the future, is it normally safe to just move those patched files to the next version and then do an update, or do you absolutely need to patch for example Drupal 7.10 before uploading to the server?
Comment #134
aeroweb340 CreditAttribution: aeroweb340 commentedHi,
Since I applied the populate image patch I'm getting this error when I insert an image in my article:
PDOException : SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_image_width' in 'field list': INSERT INTO {field_data_field_image} (entity_type, entity_id, revision_id, bundle, delta, language, field_image_fid, field_image_alt, field_image_title, field_image_width, field_image_height) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10); Array ( [:db_insert_placeholder_0] => node [:db_insert_placeholder_1] => 118 [:db_insert_placeholder_2] => 123 [:db_insert_placeholder_3] => article [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => fr [:db_insert_placeholder_6] => 374 [:db_insert_placeholder_7] => [:db_insert_placeholder_8] => [:db_insert_placeholder_9] => 3008 [:db_insert_placeholder_10] => 2000 ) dans field_sql_storage_field_storage_write() (ligne 424 dans /homez.467/bridgena/www/modules/field/modules/field_sql_storage/field_sql_storage.module).
Any idea ?
Many thanks
Comment #135
jbrown CreditAttribution: jbrown commentedDid you run update.php?
Comment #136
pillarsdotnet CreditAttribution: pillarsdotnet commentedSame problem as #124. Presumably, the same solution as #129, i.e.:
.../update.php
after applying the patch.Comment #137
aeroweb340 CreditAttribution: aeroweb340 commentedI think I applied the wrong patch THANKS
Comment #138
pillarsdotnet CreditAttribution: pillarsdotnet commented@#133: see http://drupal.org/node/1299198
Comment #139
Fabianx CreditAttribution: Fabianx commentedI re-reviewed the patch and am now using it in production:
Review
The patch:
* solves the problems discussed in the issue summary
* without introducing unnecessary code
* including test cases for all problematic and unique cases
The patch is important for Drupal as specifying image dimesions is important for the speedy rendering of the webpage (Google Pagespeed criteria)
The patch uses two additional columns in the database for images to store the width and height.
The patch does not decrease I/O performance as the image is just read from the disk / network / whatever when it is imported into the imagefield and the preview is shown.
The patch does not decrease overall performance as the computations (dimensions callback) introduced in this patch are quick compared to reading I/O from disk (D6 method) or from the database or cache.
The patch changes the API of image.module, but documents this changes and remains backwards compatible. All core code is changed to make use of the new API structure.
All comments are doxygen compliant and also no longer use constructs like "TRUE or FALSE depending on outcome", but rather "TRUE on success, FALSE on failure", which was one important point to change.
Feedback from #111 has been integrated into the patch and both problems have been solved.
Conclusion
Given all the above facts and the fact that I already reviewed this some months ago and it was already then almost suitable for inclusion and the remaining problems got fixed, I say:
This patch is suitable for inclusion into Drupal Core and can be commited to D8.
Note: I did not review the upgrade path for the D7 version, but this needs to go into D8 first anyway and I propose to first commit it to D8, then finish possible remaining install and upgrade path problems.
It would be great to have this in D8 (and then for D7, too)!
Best Wishes,
Fabian
Comment #140
dropbydrop CreditAttribution: dropbydrop commentedI hope to see it soon at D7, so that I don't need mod_pagespeed anymore.
Comment #141
crashtest_ CreditAttribution: crashtest_ commentedI just wanted to say that comment #139 was a _fantastic_ review. GREAT JOB Fabian!
Comment #142
quicksketchPatch looks great to me too. A few areas where I'd say, "I wouldn't have written it like that", but the code works and I can't argue with it. "Better is the enemy of done" as they say. :)
So this gets my +1 on RTBC. Absolutely fantastic work jbrown, super kudos for your resilience on getting this extremely important issue through the ringer.
Comment #144
Fidelix CreditAttribution: Fidelix commentedI don't know how to report a spammer... Someone please do.
Just changing the title back...
Comment #145
jamesbenison CreditAttribution: jamesbenison commented@Fildelix
Please clarify your spam concern.
Speaking on behalf of drupal 7 this patch is long overdue.
This issue with the core image module hamstrings developers and users. Are core changes going to be made or do we need to create new modules that allow us to incorporate additional image attributes?
Add-on modules are a bad way to go because it requires people to create new "image" fields for their existing content. It also adds dependencies.
Can we get a road map please?
Comment #146
jbrown CreditAttribution: jbrown commented#123: populate-image-width-height.patch queued for re-testing.
Comment #147
Fidelix CreditAttribution: Fidelix commented@jamesbenison, a spammer changed this issue title and posted a lot of spammy links.
Apparently someone removed the spammer and the spam between our replies.
Comment #148
webchickGreat. This seems to solve the issue while only introducing two optional indexes to the image info array, has some lovely tests to prove it works, plus sign-off from the image module maintainer. Awesome work!
Committed and pushed #123 to 8.x. This needs a re-roll and testbot confirmation for D7.
Comment #149
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled and re-uploaded 7.x patch from #128 with no changes.
Comment #150
jbrown CreditAttribution: jbrown commentedAwesome!
Comment #152
jbrown CreditAttribution: jbrown commentedLooks like the D7 patch got committed by accident: http://drupalcode.org/project/drupal.git/commit/89ad342
;-)
Comment #153
pillarsdotnet CreditAttribution: pillarsdotnet commentedFixed, then.
Comment #154
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe only remaining difference between core and #149 is a whitespace correction.
Comment #155
webchickOops. :P hehe
Comment #156
pillarsdotnet CreditAttribution: pillarsdotnet commented@webchick -- is there any way you can
git commit --amend
that last change to have a more meaningful commit message?Comment #157
webchickSadly, no. Not once it's been pushed. :(
Comment #158
jbrown CreditAttribution: jbrown commentedThat's okay! Just glad it got committed!
Prob not worth reverting it if it will just get committed again.
Comment #159
webchickOk, I think I fixed it up now. I reverted the accidental commit, then re-committed it with the whitespace fix. We'll see if testbot is happy about it in a half hour or so.
Comment #160
klonosJust upgraded to latest dev (20-Oct-2011) and got this error after running db update 7002:
Let me note that this is a test installation and there were no files uploaded, so the file_managed table is empty. This could be it.
...also, the update still shows as pending.
Comment #161
catchIf the update is broken that sounds critical.
Comment #162
BTMash CreditAttribution: BTMash commentedThis is indeed critical because it is causing the failure for #1182296: Add tests for 7.0->7.x upgrade path (random test failures). Attaching patch which should fix this. I have talked about this with @yched and with help from @mbrandon at #badcamp, I have taken a bit of how things are done in text.install but updated it in the format that the update requests. From the manual tests, this seems to go through successfully.
Comment #163
klonosOk, after applying #162 the 7002 update went fine and site status is green again. Thanx ;)
Comment #164
BTMash CreditAttribution: BTMash commentedThere is still an issue...when there are no images, I get an exception from the simpletests. This should hopefully resolve that issue.
Comment #165
BTMash CreditAttribution: BTMash commentedargh...sandbox['total'] is not an array.
Comment #166
TR CreditAttribution: TR commented#162 fixes the issue for me on a site with images.
Comment #167
webchickTagging as a release blocker. Thanks for the work on this!
Comment #168
jbrown CreditAttribution: jbrown commentedThanks #badcamp. Reviewing this from DrupalCamp Ireland.
This needs comments.
The call to _update_7000_field_read_fields() specifies 'deleted' => 0, so we shouldn't need to be checking for $field['deleted'] later on.
Why is $field['storage']['details'] sometimes empty?
Comment #169
jbrown CreditAttribution: jbrown commentedI fixed this in a cleaner way.
Comment #170
BTMash CreditAttribution: BTMash commentedFrom talking with @sun and @yched, some of this information may be missing if fields are created programmatically (saving via the UI adds them in correctly). So in my instance (for the simpletests), I installed the standard profile which created the fields (so the profile did not add them in). But since I had not re-saved them, this additional info was never added.
Mind you, also from talking with @yched, solving this in the image profile won't solve the issue on what might happen with other profiles/other folk creating this programmatically. Hence the fix doing what it does.
I like your patch but I'll test out if its working correctly with the upgrade simpletests.
Comment #171
BTMash CreditAttribution: BTMash commentedThe code looks good to me and the tests from #1182296: Add tests for 7.0->7.x upgrade path (random test failures) now pass. The only thing I would consider changing is the comment you have regarding why the table info is missing:
Changing it to a better version of what wrote above would be great. But aside from this minor piece, I think this is good to go.
Comment #172
BTMash CreditAttribution: BTMash commentedTalking with chx, we are removing references to $field['storage']['details'] and going with the assumption on how field tables are named (also based on prior discussion with @yched).
Comment #173
BTMash CreditAttribution: BTMash commented@chx pointed me to
_field_sql_storage_tablename
and_field_sql_storage_revision_tablename
. So reroll patch with these functions.Comment #174
BTMash CreditAttribution: BTMash commentedAck..wrong patch.
Comment #175
chx CreditAttribution: chx commentedThat's a lot better. What a sad mess! I have missed the D7 version of the patch on RTBC patrol, it contained an update and I wasnt pinged over the update :/ and it contained a construct I am actively fighting against. I really would like to know who started this brilliant notion of using field[storage][details] :(
Edit: while usually we do not allow editing updates this was not yet released and is still very fresh and is likely that most trying got a PDO Exception anyways.
Comment #176
jbrown CreditAttribution: jbrown commentedI'm happy with the patch in #174. I did some further testing including updating fields with no content and multi-pass batch update.
We really need some better infrastructure to help with updating field schema.
Comment #177
catchGreat that the tests from #1182296: Add tests for 7.0->7.x upgrade path (random test failures) caught this and verified the fix, looks good to me too.
Comment #178
Pasquallenew issue title? image_update_7002() is broken..
Comment #179
catchThis already has the release blocker tag, but we can make it more obvious yeah.
Comment #180
webchickCommitted and pushed to 7.x. Thanks!
Comment #181
webchickComment #182
adamdicarlo CreditAttribution: adamdicarlo commentedEdit: The theme cache needs to be cleared after updating core, so that core will provide the NULL values when width and height are not passed in.
Arguably, this patch made an API change for theme_image_style(): it now requires the "width" and "height" variables, but basically says it doesn't. With existing code (which doesn't pass these in), PHP notices occur:It doesn't seem like the lack of width and height affect the resulting image. What's the official way this should be fixed? The header comment for theme_image_style() says:But what is supposed to be passed if these are not known? 0? NULL? FALSE? 42? This needs some kind of work in core, I'd say, even if it's as simple as changing the function's header comment.Not sure what I should set priority to now or whether this would be better in another issue, so here it is.Comment #183
adamdicarlo CreditAttribution: adamdicarlo commentedWell, having looked at the comments in this issue and the code, width and height are supposed to be optional.So here's a tiny follow-up patch that removes the PHP notices for me (which I'd encountered from a View formatting a File field with a File Style that uses an image style preset).Comment #184
marcingy CreditAttribution: marcingy commented@adamdicarlo how exactly are you calling the theme function?
Comment #185
adamdicarlo CreditAttribution: adamdicarlo commented@marcingy see for example http://drupalcode.org/project/styles.git/blob/refs/heads/7.x-2.x:/contri... -- this submodule of the Styles module invokes theme('image_style') and does not supply width or height.
From what I can tell, width and height are supposed to be optional. I don't want to repeat myself here, but if they're supposed to be optional, then core shouldn't generate PHP notices if they're not included. But core generates PHP notices if they're not included. If they're not optional, point me to where it says so -- I can't find it. As I posted above:
The "if known" sure sounds optional to me.
Also, if anyone is to declare they're not optional -- what the hell? Why would it be required after not having existed before? This issue's patch wasn't supposed to make an API change.
Comment #186
Dave ReidConfirmed, I'm getting a bunch of PHP notices now via the Media module. Storing image height and width data associated with fields rather than the actual file entities seems a little wrong since I can't easily look it up without a field context.
Comment #187
marcingy CreditAttribution: marcingy commentedThese are set as defaults by the image theme hook.
So something strange is going on
Comment #187.0
marcingy CreditAttribution: marcingy commentedupdate issue summary
Comment #188
xjmUpdated summary to explain the history of this issue since it's kinda sorta been three issues. The current issue is the notices caused by the original change (see #182 on.)
There is a proposed patch in #183, so I believe the correct status should be NW?
Comment #189
jbrown CreditAttribution: jbrown commentedYes - image_theme() defines the default values of the width and height variables to be NULL, so this shouldn't be happening.
Is everybody running update.php (even to clear the cache)?
Dave Reid - this change was for Drupal 7 and so was designed to be as uninvasive as possible - perhaps images should be entities in D8?
Comment #190
jbrown CreditAttribution: jbrown commentedI just tried editing theme_image_formatter() so that the height and width variables are not set before calling theme('image_style', $image) and I am not getting the warnings mentioned in #182.
So either people are not having their theme cache cleared after updating core or something weird is going on.
We need steps to reproduce.
Comment #191
ELC CreditAttribution: ELC commentedI found the issue to be that my modules were not passing the width/height into the theme function because it was never needed before. These values are not stored in the database along with the files .. so I had to call image_get_info() anyway. Image Field seems to keep the values itself ok, but I wasn't passing them on.
Double check was is passing the values to the theme function and ensure that it is passing on all information.
Comment #192
xjmFixed crosspost; adding note about steps to reproduce to summary.
Comment #193
xjmAlso, notices isn't critical; that was left over from the upgrade path bug. :)
Comment #193.0
xjmUpdated issue summary.
Comment #194
xjmAnd fixing tags.
Comment #195
xjmGr. Sorry.
Comment #196
adamdicarlo CreditAttribution: adamdicarlo commented@jbrown, you're completely correct about how the theme system will set those to NULL (once the cache has been cleared and the theme registry refreshed). Sorry for jumping the gun here!
Comment #197
aklys CreditAttribution: aklys commentedI noticed that the height and width of the image are not updated after uploading the new version of the file (eg by file_copy function). It seems to me that this is a bug and I prepared a patch that adds to image.module implementation of hook_file_update, which, after each change of file updates the height and width in all fields that are associated with this file.
I am rather new to Drupal, so it may not be the best solution to this problem, but it works for me.
Please verify this patch (or provide a better solution to this problem)
Comment #198
Tor Arne Thune CreditAttribution: Tor Arne Thune commented#197 deserves some attention.
Comment #200
xjmLooks like it needs to be rerolled for
core/
. Edit: And going forward can we open new, separate followup issues? Makes it a bit easier to sort out what's going on. :)Comment #201
jbrown CreditAttribution: jbrown commentedIs this for FILE_EXISTS_REPLACE?
I _totally_ disagee with the notion that is should be possible for the database to updated so that an existing fid can refer to a new file. If the file is new then it should have a new fid.
Bypassing the field system and replacing a file out from under it is architecturally incorrect...
This patch just adds cruft.
Comment #202
aklys CreditAttribution: aklys commented#197: 1129642_image_file_update.patch queued for re-testing.
Comment #203
aklys CreditAttribution: aklys commentedYes, this is for file_copy with FILE_EXISTS_REPLACE.
Example of use: the site is several hundred photos that are too large and contain unnecessary EXIF information. I can write a script that will reduce these images to the desired size, remove exif data and update data in drupal (by file_copy). I expect that after such operation will also be updated height and width stored in all existing fields associated with these files.
Comment #204
jbrown CreditAttribution: jbrown commentedSo - the correct way to do this is to configure image styles - no programming required.
If you really need to shrink the source images (so they take up less hard drive space), your script should be using node_save to update the files stored in the node. This way the integrity of the db is assured.
Comment #206
aklys CreditAttribution: aklys commentedI had about 500 images in the table file_managed that were assigned to different fields in several different types of nodes. I needed to improve most of them (to improve the originals stored files, and not the individual styles of images). I read the documentation on api.drupal.org and came to the conclusion that the easiest way to improve these images is by the script executed on the server, and then upgrade them using drupal function file_copy with an argument FILE_EXISTS_REPLACE.
The operation went well, but I noticed that the information about the dimensions of images stored in tables of data fields (which were introduced in drupal 7.9) have not been updated. Perhaps you could improve these images in any other way, but it is not important.
More important is that there is a function file_save in Drupal api (which is used by file_copy), which allows you to save a new version of the file to the database, so in the particular case a new version of the image, which can be used for any field in some nodes. In my view, in this situation should be updated information about the dimensions of the image in the tables with data fields, because otherwise it may lead to inconsistencies in the database.
It seems to me that the best place for such an operation is to implement hook_file_update in image module.
By the way: I do not know why my patch does not pass the tests. It seems to me that I prepared it as described in the documentation (using git). Perhaps the problem is that the patch is for the 7.x branch? In any case, please for understanding, because this is my first patch here:).
Comment #207
catchPlease open a follow-up issue for this, we're already at over 200 comments here.
Comment #208
kolafson CreditAttribution: kolafson commentedsubscribing
Comment #209
Fidelix CreditAttribution: Fidelix commented@kolafson
Stop subscribing, start following:
http://drupal.org/node/1306444
Comment #210
m4xjb CreditAttribution: m4xjb commentedGreat to see that you guys have been fixing this issue...
Is there a working patch to use as I am desperate to get image dimensions into image tags.
I am running Drupal 7.8, 2011-08-31 and have never patched core before. (I'm relatively new to Drupal) Although I have been reading the Drupal docs on how to do so. It seems the patch files above are dependent on each other, is that right? Which directory do I need to be in to apply the patch? I have been following the directions in this article: http://drupal.org/node/60818
Are they correct for this situation?
Any help is greatly appreciated!
Comment #211
jbrown CreditAttribution: jbrown commentedThese patches were applied before 7.9 was released, so you just need to upgrade to 7.9 .
Comment #212
m4xjb CreditAttribution: m4xjb commentedTHANK YOU JBROWN!!
I have installed Drupal 7.9 and the image tags are populated with width and height dimensions, great!
When using a custom image style (in my case, fixed width and variable height), some thumbnails are rendered correctly. But others display incorrectly calculated widths and heights. For example, what should be 280px wide & 414px high are generated with tags width="128" and height="187.022900763"
Obviously there are two problems here:
1) The calculations are way off
2) The units should not have decimals.
I notice there is a proposed fix in comment #183. By amending the theme_image_style function. I tried this without success.
Does anyone know if there a fix available or have I not implemented it correctly?
Comment #213
jbrown CreditAttribution: jbrown commentedThis is very weird.
Can you list the effects you have configured in your image style along with their settings and also the dimensions of the source images that are being affected?
Thanks!
Comment #214
johnnynia CreditAttribution: johnnynia commentedI experienced the same strange behavior as m4xjb. Disabling "allow upscaling" in the style settings made everything work again. Re-enabled it and all the width and height attribute values were wrong again. (None of the pics was actually upscaled, the are large enough.)
Hope that helps.
Comment #215
BTMash CreditAttribution: BTMash commentedI've opened a followup issue regarding the image scaling funkiness at #1338428: image_dimensions_scale() and image_scale_effect() are ungrokable and buggy.
Comment #216
khiminrm CreditAttribution: khiminrm commentedsubscribe
Comment #217
webchickHm. Seeing some disturbing reports about 7.9 from places like:
#1327498: The content (body field) of some nodes is lost after updating from 7.8 to 7.9
#1343794: Unknown column 'field_image_width' on Drupal 7.8 to 7.9 upgrade
#1326484: Problem updating my drupal 7.7 installation to 7.9
Could someone from this issue please take a look?
Comment #218
jbrown CreditAttribution: jbrown commentedThe second one is just someone not running update.php
Comment #219
syodash CreditAttribution: syodash commentedI ran update.php when upgrading to 7.9 did all the steps. The problem is not resolved, even when I tried the dev 14nov current version. When I run update.php tells me no updates are pending.
Comment #220
johnathankent CreditAttribution: johnathankent commented#183: 1129642-fix-php-notices-followup.patch queued for re-testing.
Comment #221
johnathankent CreditAttribution: johnathankent commentedsame for me
Comment #222
marcingy CreditAttribution: marcingy commentedAs per Catch's comment in 207 please open a new follow up issue
Comment #222.0
marcingy CreditAttribution: marcingy commentedadded note re: stpes to reproduce
Comment #223
jbrown CreditAttribution: jbrown commentedComment #224
jbrown CreditAttribution: jbrown commentedI created a change notification: http://drupal.org/node/1353920
Comment #226
Dave ReidArgh, adding these height and width attributes to the field schema rather than a table that can be joined off of {file_managed} and therefore re-used whenever you call file_load() just completely screwed over the Media project and I'm kicking myself that I didn't push harder against this approach from #186. Anyone involved in this original issue I apologize for updating a closed issue but it's important that we get feedback on doing this properly with #1448124: Image dimensions should be available from file_load() for images, and not stored in field data.
Comment #227
jbrown CreditAttribution: jbrown commentedI posted the original patch on 17th October. It got committed on 17th April, so you had exactly 6 months to change it. See you over in #1448124: Image dimensions should be available from file_load() for images, and not stored in field data.
Comment #227.0
jbrown CreditAttribution: jbrown commentedRemove remaining tasks.
Comment #228
mikey_p CreditAttribution: mikey_p commentedWhile working on the Retina Images module I found an issue with this implementation. The problem is that when a field specifies a default image, it does not store height/width with the field settings. This means that when image_style_transform_dimensions() is called on the style, no data is passed in which causes the resulting image to have no height/width attribute. This causes module like Retina Images to behave erratically since user uploaded images display correctly, but default images do not.
See #2052279: Does not work on default images for more info.
Comment #229
mikey_p CreditAttribution: mikey_p commentedI just verified that this has been resolved in D8 by storing the height/width in the field storage or field instance config.
Comment #230
Michael_Lessard_micles.biz CreditAttribution: Michael_Lessard_micles.biz commentedI am a bit confused by the issue (not the patch) for the following reason... Image fields in my D7.43 have the HTML width="xyz". The original message in this issue says such width HTML code is missing, while my issue is that it does have an imposed width.
( Edit : removed secondary info which was not relevant to this issue here.)
Comment #236
klonosWow! ...I have re-read this entire thread, and here's the summary of it all:
Given all the above, and also that:
I am going to take the liberty to close it off.
Thanks everyone for their contributions here over time.