If I update an image file which had image dimage dimensions of 1:1 and replace it with one of 4:3 the image dimensions did not get updated. It will be displayed at 1:1 (at least with image styles).
The values in the db field_data_field_image > field_image_width and field_image_height are not changed either.
This results in "stretched" images.

CommentFileSizeAuthor
#11 2211937-post-commit-fix.patch888 bytesgarphy
#6 update-image-dimensions-2211937-3.patch1.12 KBDevin Carlson
#3 update-image-dimensions-2211937-3.patch778 bytesDevin Carlson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grossmann’s picture

grossmann CreditAttribution: grossmann commented

As I found out this is fixed if I use the media file selector widget (an not the image widget) and just resave the node.
So this may be a bug not related to file_entity.

redndahead’s picture

redndahead CreditAttribution: redndahead commented

We saw this while studying the code for something else.

This is because in file_entity_file_update there is this if statement.

if ($file->metadata['width'] != $file->metadata['width'] || $file->metadata['height'] != $file->metadata['height']) {
  _file_entity_update_image_field_dimensions($file);
}

Which is always false because it's comparing the same variable against itself. Which of course means this will always be the same. So it needs to compare to the old file.

Devin Carlson’s picture

Devin Carlson CreditAttribution: Devin Carlson commented
Version: 7.x-2.0-alpha3 » 7.x-2.x-dev
Status: Active » Needs review
FileSize
update-image-dimensions-2211937-3.patch778 bytes

@redndahead Nice find!

A patch to compare the current entity's metadata vs. the original entity's metadata.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I wonder if we should check $file->filesize and $file->original->filesize as well, in cases where the user uploads a different image with the same dimensions?

Status: Needs review » Needs work

The last submitted patch, 3: update-image-dimensions-2211937-3.patch, failed testing.

Devin Carlson’s picture

Devin Carlson CreditAttribution: Devin Carlson commented
Status: Needs work » Needs review
FileSize
update-image-dimensions-2211937-3.patch1.12 KB

Added an additional filesize check.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Reviewed & tested by the community

Looks great. I think we should file a follow-up to actually set a $file->is_changed property on hook_file_presave() by using a helper method that contains our logic we're putting in here. It could have an alter hook to let other modules figure out if it should be marked as a 'changed' file too.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x-2.x and added a follow-up issue: #2350427: Add a helper method to determine if a file has changed

garphy’s picture

garphy CreditAttribution: garphy commented

I just came accross a case where $file->metadata and $file->original->metadata are not set in file_entity_file_update()

The introduced += statements break with a fatal error.
I don't know if my database is somewhat "not valid" by not having metadata on some file entities, but we should take care somehow.

New issue or new patch in this issue ?

garphy’s picture

garphy CreditAttribution: garphy commented
FileSize
2211937-post-commit-fix.patch888 bytes

Here is what I think we should add to protect the code.

Status: Fixed » Closed (fixed)

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

garphy’s picture

garphy CreditAttribution: garphy at ICI LA LUNE commented
Status: Closed (fixed) » Needs review

Re-opening since we got no answer from the maintainers on #10 & #11.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Postponed (maintainer needs more info)

Looking at http://cgit.drupalcode.org/file_entity/tree/file_entity.file.inc#n168, I do not see how $file->metadata could not be defined, unless for some reason a complete file object is not being used, not from file_load().

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Postponed (maintainer needs more info) » Closed (fixed)