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.

CommentFileSizeAuthor
#197 1129642_image_file_update.patch1.72 KBaklys
#183 1129642-fix-php-notices-followup.patch1.03 KBadamdicarlo
#174 1129642_174.patch2.01 KBBTMash
#173 1129642_173.patch2.01 KBBTMash
#172 1129642_172.patch2.01 KBBTMash
#169 populate-image-width-height-followup.patch2.31 KBjbrown
#165 1129642_164.patch1.42 KBBTMash
#164 1129642_164.patch1.43 KBBTMash
#162 1129642_162.patch1.07 KBBTMash
#154 whitespace-correction-1129642-154.patch1.02 KBpillarsdotnet
#150 populate-image-width-height.patch35.62 KBjbrown
#149 populate-image-width-height-1129642-149.patch36.45 KBpillarsdotnet
#123 populate-image-width-height.patch31.93 KBjbrown
#123 populate-image-width-height-d7.patch35.62 KBjbrown
#121 populate-image-width-height.patch31.88 KBjbrown
#121 populate-image-width-height-d7.patch35.56 KBjbrown
#109 populate-image-width-height.patch31.01 KBjbrown
#109 populate-image-width-height-d7.patch34.76 KBjbrown
#97 colorbox7-1.1-populate-image-width-height-d7_7.patch527 bytesOnkelTem
#93 error.png99.64 KBAdamGerthel
#88 populate-image-width-height-d7.patch34.74 KBjbrown
#84 populate-image-width-height-d7.patch34.64 KBjbrown
#83 populate-image-width-height.patch30.99 KBjbrown
#83 populate-image-width-height-d7.patch34.67 KBjbrown
#67 populate-image-width-height.patch31.02 KBjbrown
#67 populate-image-width-height-d7.patch33.22 KBjbrown
#62 populate-image-width-height.patch31.02 KBjbrown
#62 populate-image-width-height-d7.patch33.22 KBjbrown
#61 populate-image-width-height.patch29.69 KBjbrown
#61 populate-image-width-height-d7.patch31.89 KBjbrown
#56 populate-image-width-height.patch29.45 KBjbrown
#56 populate-image-width-height-d7.patch31.65 KBjbrown
#54 populate-image-width-height-d7.patch31.19 KBjbrown
#53 images_width_height-1129642-53.patch30.27 KBpillarsdotnet
#53 images_width_height-1129642-53-d7.patch30.27 KBpillarsdotnet
#31 populate-image-width-height.patch29.45 KBjbrown
#27 populate-image-width-height.patch28.87 KBjbrown
#26 populate-image-width-height.patch27.32 KBjbrown
#24 populate-image-width-height.patch26.73 KBjbrown
#21 populate-image-width-height.patch26.17 KBjbrown
#18 populate-image-width-height.patch25.81 KBjbrown
#16 populate-image-width-height.patch25.14 KBjbrown
#14 populate-image-width-height.patch19.67 KBjbrown
#13 populate-image-width-height.patch14.98 KBjbrown
#12 populate-image-width-height.patch14.9 KBjbrown
#11 populate-image-width-height.patch14.9 KBjbrown
#4 populate-image-width-height.patch15.43 KBjbrown
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jensimmons’s picture

Redndahead (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.

Dave Reid’s picture

I 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.

jensimmons’s picture

Status: Active » Closed (duplicate)

This 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.

jbrown’s picture

Status: Closed (duplicate) » Needs review
FileSize
15.43 KB
Jeff Burnz’s picture

jbrown, shouldn't this be D8 material, surely love it be in D7.

jbrown’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » jbrown

The 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.

Dave Reid’s picture

This is changing the APIs, so I'm not sure it could be accepted for D7 at this point.

jbrown’s picture

It doesn't change any APIs. Image effects can now expose dimension callbacks, but this is entirely optional.

Damien Tournoud’s picture

Very sweet patch!

+  foreach ($style['effects'] as $effect) {
+    if (!isset($variables['width']) || !isset($variables['height'])) {
+      break;
+    }
+    if (isset($effect['dimensions callback'])) {
+      $effect['dimensions callback']($variables, $effect['data']);
+    }
+  }

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.

jbrown’s picture

Correct on both counts!

jbrown’s picture

jbrown’s picture

Issue tags: +Needs tests
FileSize
14.9 KB

Implemented 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.

jbrown’s picture

Changed 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.

jbrown’s picture

Issue tags: -Needs tests
FileSize
19.67 KB

I wrote some tests.

Lars Toomre’s picture

I 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!!

jbrown’s picture

Thanks 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!

Lars Toomre’s picture

I 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.

jbrown’s picture

I 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?

aspilicious’s picture

Srry for being anoying

+++ includes/image.incundefined
@@ -214,19 +215,57 @@ function image_scale(stdClass $image, $width = NULL, $height = NULL, $upscale =
+ * Scales an image to the given width and height while maintaining aspect

Can this be written on one line only? (less than 80 chars?)

+++ modules/image/image.moduleundefined
@@ -760,6 +760,36 @@ function image_style_create_derivative($style, $source, $destination) {
+ * Determine the dimensions of the styled image.

Determines

Powered by Dreditor.

MustangGB’s picture

Subscribe

jbrown’s picture

Eric_A’s picture

Noticed a documentation issue...

+ *   - "upscale": A Boolean indicating that the image should be upscalled if
+ *     the dimensions are larger than the original image.

-wrap at 80
-upscalled/upscaled
A Boolean/A boolean (Not sure about this. There's one more if it needs to be changed.)

Sylvain Lecoy’s picture

subscribing

jbrown’s picture

Those comments for image_scale_dimensions() were just copied and pasted from image_scale_effect().

Updated patch fixes it for both functions.

Eric_A’s picture

You've fixed the source and one copy, but missed the second copy...

+ *   - "random": A Boolean indicating that a random rotation angle should be
jbrown’s picture

Ah!

I have now fixed that one and the place I copied it from.

jbrown’s picture

I 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.

TimG1’s picture

Subscribing.

Sylvain Lecoy’s picture

Issue tags: +DrupalWTF

Guys 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.

jbrown’s picture

These are the problems with reading in the dimensions from the image every time it is served:

  • 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.

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.

jbrown’s picture

Issue tags: +DrupalWTF
FileSize
29.45 KB

Updated 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.

scott.whittaker’s picture

This 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?

Damien Tournoud’s picture

Issue tags: -DrupalWTF

CPU is scalable. IO access isn't.

jbrown’s picture

@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.

jbrown’s picture

The patch works fine on both D7 and D8, but the D7 version needs a db update function.

Anyone want to RTBC this for D8?

jbrown’s picture

Issue tags: -DrupalWTF

#31: populate-image-width-height.patch queued for re-testing.

johsw’s picture

Works fine here for new imagefields, but I don't think I have the in-depth understanding of the patch to RTBC it.

jbrown’s picture

#31: populate-image-width-height.patch queued for re-testing.

jbrown’s picture

geerlingguy’s picture

Yeah, this'd be awesome; I always hated how that worked in D6.

Fidelix’s picture

Brilliant work, jbrown! I hope this gets in D7.1.

Sylvain Lecoy’s picture

In your test cases, you should add the RDFa module as a dependency, otherwise the tests fails.

jbrown’s picture

Sylvain: can you list the steps required for the tests to fail?

jbrown’s picture

Sylvain: DrupalWebTestCase defaults to using the standard install profile, so it isn't necessary for it to be a dependency.

Sylvain Lecoy’s picture

Oh I see, not actually tested but you was using type="foaf:Image" which is a RDFa descriptor.

dropbydrop’s picture

when drupal 7 will print image width and height at html alt attributes?

pillarsdotnet’s picture

#31: populate-image-width-height.patch queued for re-testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: ++patch

Reviewed 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

jbrown’s picture

I need to write an update function for the D7 patch - this isn't required for D8.

Fabianx’s picture


I need to write an update function for the D7 patch - this isn't required for D8.

Ah, sure. Good catch :-).

Best Wishes,

Fabian

RobW’s picture

Sub. 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.

RobW’s picture

Actually, 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 errors

Checking patch image.inc...
error: image.inc: No such file or directory
Checking patch image/image.api.php...
error: image/image.api.php: No such file or directory
Checking patch image/image.effects.inc...
error: image/image.effects.inc: No such file or directory
Checking patch image/image.field.inc...
error: image/image.field.inc: No such file or directory
Checking patch image/image.install...
error: image/image.install: No such file or directory
Checking patch image/image.module...
error: image/image.module: No such file or directory
Checking patch image/image.test...
error: image/image.test: No such file or directory
Checking patch image/tests/image_module_test.module...
error: image/tests/image_module_test.module: No such file or directory

So 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.

pillarsdotnet’s picture

Re-rolled #31 for 8.x and 7.x using git format-patch. No code changes, so leaving as RTBC.

jbrown’s picture

I added the database update functionality to the D7 patch.

RobW’s picture

D7 patch in #54 tested:

  • Works with image fields created before and after patch
  • Works with default image styles (including 'original')
  • Doesn't seem to work with user created image styles
jbrown’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
31.65 KB
29.45 KB

Here 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.

RobW’s picture

Did 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:

  1. Add a rotate effect.
  2. Specify any non-90 degree multiple. If you use 180, 90, 360, etc, everything works fine.
  3. The resulting image won't have width and height.
  4. If you add another filter afterwards, dimensions can be re-introduced. Crop, resize, and scale and crop work. Desaturate, scale, other rotates (90 degree multiples or otherwise), and crops and resizes that don't change the image size (cropping to the exact image size, effects that don't apply because of no-upscale, etc), don't.

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.

jbrown’s picture

Thanks 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

RobW’s picture

No problem, thank you for for the patch, explanation, and patch thread link.

aspilicious’s picture

+++ b/includes/image.incundefined
@@ -195,13 +196,12 @@ function image_scale_and_crop(stdClass $image, $width, $height) {
  * @return
- *   TRUE or FALSE, based on success.
+ *   Boolean indicating whether or not the dimensions were changed.

Reading 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.

+++ b/includes/image.incundefined
@@ -214,19 +214,56 @@ function image_scale(stdClass $image, $width = NULL, $height = NULL, $upscale =
+ * @return
+ *   TRUE or FALSE, based on success.
+ *

Same as above

Powered by Dreditor.

jbrown’s picture

jbrown’s picture

jbrown’s picture

#62: populate-image-width-height.patch queued for re-testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

I 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

jbrown’s picture

Issue tags: -Performance, -performance issue, -+patch

#62: populate-image-width-height.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Performance, +performance issue, ++patch

The last submitted patch, populate-image-width-height.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review
FileSize
33.22 KB
31.02 KB

Reroll.

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as before.

jbrown’s picture

Summary

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()

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.

AdamGerthel’s picture

Title: Images are not printed with height and width » Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O

The 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

pillarsdotnet’s picture

jbrown’s picture

The 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).

quicksketch’s picture

Status: Reviewed & tested by the community » Needs work

I 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:

+  // Determine table name.
+  if (!$revision) {
+    $table = 'field_data_' . $field;
+  }
+  else {
+    $table = 'field_revision_' . $field;
+  }

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.

+  foreach ($result as $file) {
+    $info = image_get_info($file['uri']);
+
+    if (is_array($info)) {
+      db_update($table)
+        ->fields(array(
+          $field . '_width' => $info['width'],
+          $field . '_height' => $info['height'],
+        ))
+        ->condition($field . '_fid', $file['fid'])
+        ->execute();
+    }
+  }

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!

mattjacobson’s picture

So 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?

slackrunner’s picture

This 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

VisualFox’s picture

Subscribe

quicksketch’s picture

@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).

jbrown’s picture

Thanks @quicksketch - I've almost got the changes implemented.

slackrunner’s picture

@quicksketch, thanks! It was as simple as running update.php

catch’s picture

Issue tags: +schema change

Just 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.

quicksketch’s picture

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.

@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?

catch’s picture

Sorry I'd missed that #73 was talking about non-core SQL storage engines.

The patch has this already:

+  $fields = _update_7000_field_read_fields(array(
+    'module' => 'image',
+    'storage_type' => 'field_sql_storage',
+  ));

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.

jbrown’s picture

Status: Needs work » Needs review
FileSize
34.67 KB
30.99 KB

I'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.

jbrown’s picture

Minor improvement to the D7 version...

catch’s picture

You can't use API functions in updates, that includes field_read_fields(), see #1134088: Properly document update-api functions.

jbrown’s picture

Status: Needs review » Needs work

Damn - 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?:

db_add_field($table, $field . '_width', $spec);
jbrown’s picture

Status: Needs work » Needs review

#83: populate-image-width-height.patch queued for re-testing.

jbrown’s picture

Okay - it doesn't use API functions anymore in the update.

adamdicarlo’s picture

Subscribing.

jbrown’s picture

Anyone want to test this?

bt82’s picture

i just did and i can confirm it is working with D7.7... thanks

OnkelTem’s picture

Thanks 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!

AdamGerthel’s picture

FileSize
99.64 KB

I 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.

OnkelTem’s picture

Looks like this patch doens't populate attrs of IMGs of Imagecache presets having only "Scale" action.

Drupal 7.7.

jbrown’s picture

@OnkelTem did you run update.php ?

jbrown’s picture

@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?

OnkelTem’s picture

@OnkelTem did you run update.php?

Sure.
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.

Status: Needs review » Needs work

The last submitted patch, colorbox7-1.1-populate-image-width-height-d7_7.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review

colorbox is not part of core

OnkelTem’s picture

Ok, edited previous post, pointing ppl to the proper place. Sorry.

AdamGerthel’s picture

@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).

Recoverable fatal error: Argument 1 passed to image_style_transform_dimensions() must be an array, boolean given, called in /--path-to-site--/modules/image/image.module on line 1180 and defined in image_style_transform_dimensions() (line 827 of /--path-to-site--/modules/image/image.module).

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.

OnkelTem’s picture

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.

Scale 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.

AdamGerthel’s picture

@OnkelTem no. I'm using the standard image display formatter.

OnkelTem’s picture

My guess is that image_style_load() in your case returned FALSE on some reason, while jbrown's patch of theme_image_style() doesn't check this, passing FALSE to image_style_transform_dimensions() awaiting array:

 $style = image_style_load($variables['style_name']);
  image_style_transform_dimensions($style, $variables);

Anyway, you need to know, why loading style fails. Maybe recreate preset?

Fabianx’s picture

@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

Dean Reilly’s picture

Subscribing

AdamGerthel’s picture

I 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.

OnkelTem’s picture

To 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:

kpr(debug_backtrace());

See how the theme_image() was called (args will be printed in the output either).

jbrown’s picture

Thanks 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

AdamGerthel’s picture

Edit: nvm tried the wrong patch

Will see if I can get the time to reproduce the problems properly on a clean install

quicksketch’s picture

This patch looks good but I've got a few small comments that would be great to take into consideration:

+    if (isset($variables['width']) && isset($variables['height'])) {
+      $element['width'] = array(
+        '#type' => 'value',
+        '#value' => $variables['width'],
+      );
+      $element['height'] = array(
+        '#type' => 'value',
+        '#value' => $variables['height'],
+      );
+    }

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:

+   $element['width'] = array(
+      '#type' => 'value',
+      '#value' => isset($variables['width']) ? $variables['width'] : NULL,
+    );
+    $element['height'] = array(
+      '#type' => 'value',
+      '#value' => isset($variables['height']) ? $variables['height'] : NULL,
+    );

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():

+  // Determine the dimensions of the styled image.
+  image_style_transform_dimensions($variables['style_name'], $variables);

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().

jarrodirwin’s picture

subscribing

drupal_was_my_past’s picture

Subscribe

MustangGB’s picture

Tested #109 on D7
- width and height attributes did not appear on any of my images
- clearing the cache presented the following notices (several times):

Notice: Undefined property: stdClass::$field_image_width in field_sql_storage_field_storage_load() (line 336 of /htdocs/modules/field/modules/field_sql_storage/field_sql_storage.module).
Notice: Undefined property: stdClass::$field_image_height in field_sql_storage_field_storage_load() (line 336 of /htdocs/modules/field/modules/field_sql_storage/field_sql_storage.module).

Edit: Fixed by running update.php

jbrown’s picture

did you run update.php ?

MustangGB’s picture

Thanks jbrown, working perfectly now

jamesbenison’s picture

This 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.

pillarsdotnet’s picture

Issue tags: -+patch +Needs issue summary update, +patch

Tagging; issue summary needed. Would jbrown please copy his excellent summary from #69 to the issue summary, using the issue summary template as a guide?

jbrown’s picture

There are some further improvements I need to make. I should have time at the weekend to work on this.

idflood’s picture

sub

jbrown’s picture

Okay everybody - try these!

Status: Needs review » Needs work

The last submitted patch, populate-image-width-height.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review
FileSize
35.62 KB
31.93 KB
Bikkne’s picture

Hi, 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!

pillarsdotnet’s picture

Status: Needs review » Needs work

You 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.

jbrown’s picture

@samuitv did you run update.php ?

Bikkne’s picture

@ 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.

jbrown’s picture

@samuitv the update needs to run or it wont work - did you install the D8 version of the patch by accident?

Bikkne’s picture

@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.

pillarsdotnet’s picture

Status: Needs work » Needs review

Back to "needs review" then.

Bikkne’s picture

Truly 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 !

jbrown’s picture

Thanks for testing!

Bikkne’s picture

Small 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?

aeroweb340’s picture

Hi,
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

jbrown’s picture

Did you run update.php?

pillarsdotnet’s picture

Any idea ?

Same problem as #124. Presumably, the same solution as #129, i.e.:

  1. Make sure you apply the D7 version of the patch if you're running D7 code.
  2. Immediately visit .../update.php after applying the patch.
aeroweb340’s picture

I think I applied the wrong patch THANKS

pillarsdotnet’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

I 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

dropbydrop’s picture

I hope to see it soon at D7, so that I don't need mod_pagespeed anymore.

crashtest_’s picture

I just wanted to say that comment #139 was a _fantastic_ review. GREAT JOB Fabian!

quicksketch’s picture

Patch 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.

Fidelix’s picture

I don't know how to report a spammer... Someone please do.
Just changing the title back...

jamesbenison’s picture

@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?

jbrown’s picture

#123: populate-image-width-height.patch queued for re-testing.

Fidelix’s picture

@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.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -performance issue

Great. 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.

pillarsdotnet’s picture

Status: Patch (to be ported) » Needs review
FileSize
36.45 KB

Re-rolled and re-uploaded 7.x patch from #128 with no changes.

jbrown’s picture

Awesome!

Status: Needs review » Needs work

The last submitted patch, populate-image-width-height.patch, failed testing.

jbrown’s picture

Looks like the D7 patch got committed by accident: http://drupalcode.org/project/drupal.git/commit/89ad342

;-)

pillarsdotnet’s picture

Status: Needs work » Fixed

Fixed, then.

pillarsdotnet’s picture

Title: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O » Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O (followup)
Status: Fixed » Needs review
FileSize
1.02 KB

The only remaining difference between core and #149 is a whitespace correction.

webchick’s picture

Oops. :P hehe

pillarsdotnet’s picture

@webchick -- is there any way you can git commit --amend that last change to have a more meaningful commit message?

webchick’s picture

Sadly, no. Not once it's been pushed. :(

jbrown’s picture

That's okay! Just glad it got committed!

Prob not worth reverting it if it will just get committed again.

webchick’s picture

Status: Needs review » Fixed

Ok, 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.

klonos’s picture

Status: Fixed » Active

Just upgraded to latest dev (20-Oct-2011) and got this error after running db update 7002:

The following updates returned messages
image module
Update #7002

Failed: PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'INNER JOIN file_managed file_managed ON ._fid = file_managed.fid ORDER BY file_m' at line 1: SELECT file_managed.fid AS fid, file_managed.uri AS uri FROM {} INNER JOIN {file_managed} file_managed ON ._fid = file_managed.fid ORDER BY file_managed.fid ASC LIMIT 100 OFFSET 0; Array ( ) in _image_update_7002_populate_dimensions() (line 305 of /var/www/my-site/modules/image/image.install).

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.

catch’s picture

Priority: Normal » Critical

If the update is broken that sounds critical.

BTMash’s picture

Status: Active » Needs review
FileSize
1.07 KB

This 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.

klonos’s picture

Ok, after applying #162 the 7002 update went fine and site status is green again. Thanx ;)

BTMash’s picture

FileSize
1.43 KB

There is still an issue...when there are no images, I get an exception from the simpletests. This should hopefully resolve that issue.

BTMash’s picture

FileSize
1.42 KB

argh...sandbox['total'] is not an array.

TR’s picture

#162 fixes the issue for me on a site with images.

webchick’s picture

Issue tags: +Release blocker

Tagging as a release blocker. Thanks for the work on this!

jbrown’s picture

Thanks #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?

jbrown’s picture

I fixed this in a cleaner way.

BTMash’s picture

From 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.

BTMash’s picture

The 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:

+      // Sometimes the table info is missing - don't know why.

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.

BTMash’s picture

FileSize
2.01 KB

Talking 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).

BTMash’s picture

FileSize
2.01 KB

@chx pointed me to _field_sql_storage_tablename and _field_sql_storage_revision_tablename. So reroll patch with these functions.

BTMash’s picture

FileSize
2.01 KB

Ack..wrong patch.

chx’s picture

Status: Needs review » Reviewed & tested by the community

That'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.

jbrown’s picture

I'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.

catch’s picture

Great 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.

Pasqualle’s picture

new issue title? image_update_7002() is broken..

catch’s picture

Title: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O (followup) » 7.x BROKEN: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O

This already has the release blocker tag, but we can make it more obvious yeah.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

webchick’s picture

Title: 7.x BROKEN: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O » Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O
adamdicarlo’s picture

Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Performance, +Release blocker, +Needs issue summary update, +patch, +schema change

Edit: 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:

Notice: Undefined index: width in theme_image_style() (line 1187 of modules/image/image.module).
Notice: Undefined index: height in theme_image_style() (line 1188 of modules/image/image.module).

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:

 *   - width: The width of the source image (if known).
 *   - height: The height of the source image (if known).

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.

adamdicarlo’s picture

Status: Active » Needs review
FileSize
1.03 KB

Well, 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).

marcingy’s picture

Status: Fixed » Postponed (maintainer needs more info)

@adamdicarlo how exactly are you calling the theme function?

adamdicarlo’s picture

Status: Postponed (maintainer needs more info) » Active

@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:

*   - width: The width of the source image (if known).
*   - height: The height of the source image (if known).

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.

Dave Reid’s picture

Confirmed, 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.

marcingy’s picture

These are set as defaults by the image theme hook.

// Theme functions in image.module.
    'image_style' => array(
      'variables' => array(
        'style_name' => NULL,
        'path' => NULL,
        'width' => NULL,
        'height' => NULL,
        'alt' => '',
        'title' => NULL,
        'attributes' => array(),
      ),
    ),

So something strange is going on

marcingy’s picture

Issue summary: View changes

update issue summary

xjm’s picture

Status: Active » Needs work
Issue tags: -Needs issue summary update

Updated 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?

jbrown’s picture

Status: Needs work » Active
Issue tags: +Needs issue summary update

Yes - 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?

jbrown’s picture

I 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.

ELC’s picture

I 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.

xjm’s picture

Fixed crosspost; adding note about steps to reproduce to summary.

xjm’s picture

Priority: Critical » Normal

Also, notices isn't critical; that was left over from the upgrade path bug. :)

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: -Performance, -Release blocker, -patch, -schema change

And fixing tags.

xjm’s picture

Status: Active » Needs work

Gr. Sorry.

adamdicarlo’s picture

Status: Needs work » Fixed

@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!

aklys’s picture

Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Performance, -Release blocker, -Needs issue summary update, -patch, -schema change
FileSize
1.72 KB

I 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)

Tor Arne Thune’s picture

Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs review

#197 deserves some attention.

Status: Needs review » Needs work

The last submitted patch, 1129642_image_file_update.patch, failed testing.

xjm’s picture

Looks 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. :)

jbrown’s picture

Is 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.

aklys’s picture

Status: Needs work » Needs review

#197: 1129642_image_file_update.patch queued for re-testing.

aklys’s picture

Yes, 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.

jbrown’s picture

So - 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.

Status: Needs review » Needs work

The last submitted patch, 1129642_image_file_update.patch, failed testing.

aklys’s picture

I 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:).

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Fixed

Please open a follow-up issue for this, we're already at over 200 comments here.

kolafson’s picture

subscribing

Fidelix’s picture

@kolafson
Stop subscribing, start following:
http://drupal.org/node/1306444

m4xjb’s picture

Great 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!

jbrown’s picture

These patches were applied before 7.9 was released, so you just need to upgrade to 7.9 .

m4xjb’s picture

THANK 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?

jbrown’s picture

This 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!

johnnynia’s picture

I 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.

BTMash’s picture

I've opened a followup issue regarding the image scaling funkiness at #1338428: image_dimensions_scale() and image_scale_effect() are ungrokable and buggy.

khiminrm’s picture

subscribe

webchick’s picture

jbrown’s picture

The second one is just someone not running update.php

syodash’s picture

I 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.

johnathankent’s picture

Status: Fixed » Needs review
johnathankent’s picture

same for me

marcingy’s picture

Status: Needs review » Fixed

As per Catch's comment in 207 please open a new follow up issue

marcingy’s picture

Issue summary: View changes

added note re: stpes to reproduce

jbrown’s picture

Issue tags: -Needs backport to D7
jbrown’s picture

I created a change notification: http://drupal.org/node/1353920

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Dave Reid’s picture

Argh, 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.

jbrown’s picture

I 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.

jbrown’s picture

Issue summary: View changes

Remove remaining tasks.

mikey_p’s picture

Status: Closed (fixed) » Active

While 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.

mikey_p’s picture

I just verified that this has been resolved in D8 by storing the height/width in the field storage or field instance config.

Michael_Lessard_micles.biz’s picture

I 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.)

  • webchick committed f15394b on 8.3.x
    Issue #1129642 by jbrown: Fixed Populate HTML image tags with dimension...

  • webchick committed f15394b on 8.3.x
    Issue #1129642 by jbrown: Fixed Populate HTML image tags with dimension...

  • webchick committed f15394b on 8.4.x
    Issue #1129642 by jbrown: Fixed Populate HTML image tags with dimension...

  • webchick committed f15394b on 8.4.x
    Issue #1129642 by jbrown: Fixed Populate HTML image tags with dimension...

  • webchick committed f15394b on 9.1.x
    Issue #1129642 by jbrown: Fixed Populate HTML image tags with dimension...
klonos’s picture

Status: Active » Fixed

Wow! ...I have re-read this entire thread, and here's the summary of it all:

Given all the above, and also that:

  • this issue here has become unmanageable with 200+ comments
  • there has been consensus to close this issue and deal with follow-up problems in separate issues
  • there has not been any update here over 7+ years, and no clear actionable items either

I am going to take the liberty to close it off.

Thanks everyone for their contributions here over time.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.