Warning: Division by zero in canvasactions_aspect_dimensions() (line 956 of /sites/all/modules/contrib/imagecache_actions/canvasactions/canvasactions.inc).

CommentFileSizeAuthor
#10 2719661-10-division-by-zero.-change-from-isset-to-em.patch1.09 KBSKAUGHT
#8 card-bg-echo.jpg12.69 KBSKAUGHT
#2 2719661-Division-by-zero_comment.patch1.08 KBSKAUGHT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

SKAUGHT created an issue. See original summary.

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
FileSize
2719661-Division-by-zero_comment.patch1.08 KB
SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
Status: Active » Needs review
SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
Issue summary: View changes
fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
Status: Needs review » Needs work

IMO, <em>expression</em> == NULL is one of these PHP things to avoid. Better use empty() as that more explicitly (and more clearly IMO) tells what you want and to also catch the case where the key is not set at all.

But I think there is more to this warning: this could also occur while processing (autorotate using IM, with EXIF not enabled). So, we should specify what to do in case 1 or 2 of the dimensions are missing (not set or NULL) or 0. The latter is possible in edge cases where a scale or resize rounded 1 side to 0. I need to think about this, for now setting to NW.

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented

i've only based the patch to at least deal with the endless php/watchdog notice i'm getting.

i would've preferred to use is_int() -- but i haven't backtracked enough to know why it's NULL in the first place, of course there should be a size. I actually think this whole
if condition is a sign of a bigger thing that should be addressed. personally, i'm not that in-touch with the code for this module.

dman’s picture

dman CreditAttribution: dman as a volunteer commented

IIRC, NULL is an indicator that one of the previous actions (they are numerous, and pluggable) did something to the image which means we no longer know its dimensions.
Sometimes that was just through laziness, but there were a couple of genuine cases (like random rotation, or auto-clip) where the only pre-calculated answer was *shrug*.
Due to the upstream pipeline, the dimension calculation happens in a different place from the actual transform, so rand() simply stuffs things up there.

Then there were a few issues with invalid images - those we should be sure to stamp on if we can.

The real question is - what was the actual image and the preceding actions that got you to uncovering the div-by-zero?

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
FileSize
card-bg-echo.jpg12.69 KB

if it helps, this is my preset

BASE: portfolio-grid

a:3:{s:4:"name";s:14:"portfolio-grid";s:5:"label";s:14:"portfolio-grid";s:7:"effects";a:1:{i:9;a:3:{s:4:"name";s:20:"canvasactions_aspect";s:4:"data";a:3:{s:8:"portrait";s:23:"portfolio-grid-portrait";s:9:"landscape";s:24:"portfolio-grid-landscape";s:16:"ratio_adjustment";s:1:"1";}s:6:"weight";s:1:"1";}}}

portfolio-grid-portrait

a:3:{s:4:"name";s:23:"portfolio-grid-portrait";s:5:"label";s:23:"portfolio-grid-portrait";s:7:"effects";a:4:{i:4;a:3:{s:4:"name";s:11:"image_scale";s:4:"data";a:3:{s:5:"width";s:3:"220";s:6:"height";s:0:"";s:7:"upscale";i:0;}s:6:"weight";s:3:"-10";}i:5;a:3:{s:4:"name";s:11:"image_scale";s:4:"data";a:3:{s:5:"width";s:0:"";s:6:"height";s:3:"138";s:7:"upscale";i:0;}s:6:"weight";s:2:"-9";}i:7;a:3:{s:4:"name";s:26:"canvasactions_definecanvas";s:4:"data";a:4:{s:3:"RGB";a:1:{s:3:"HEX";s:0:"";}s:5:"under";i:1;s:5:"exact";a:4:{s:5:"width";s:3:"260";s:6:"height";s:3:"173";s:4:"xpos";s:6:"center";s:4:"ypos";s:2:"23";}s:8:"relative";a:4:{s:8:"leftdiff";s:0:"";s:9:"rightdiff";s:0:"";s:7:"topdiff";s:2:"-9";s:10:"bottomdiff";s:0:"";}}s:6:"weight";s:2:"-8";}i:6;a:3:{s:4:"name";s:25:"canvasactions_canvas2file";s:4:"data";a:5:{s:4:"xpos";i:0;s:4:"ypos";i:0;s:5:"alpha";s:3:"100";s:4:"path";s:34:"portfolio-grid-bg/card-bg-echo.jpg";s:10:"dimensions";s:8:"original";}s:6:"weight";s:2:"-7";}}}

portfolio-grid-landscape

a:3:{s:4:"name";s:24:"portfolio-grid-landscape";s:5:"label";s:24:"portfolio-grid-landscape";s:7:"effects";a:4:{i:1;a:3:{s:4:"name";s:11:"image_scale";s:4:"data";a:3:{s:5:"width";s:0:"";s:6:"height";s:3:"138";s:7:"upscale";i:0;}s:6:"weight";s:3:"-10";}i:2;a:3:{s:4:"name";s:11:"image_scale";s:4:"data";a:3:{s:5:"width";s:3:"220";s:6:"height";s:0:"";s:7:"upscale";i:0;}s:6:"weight";s:2:"-9";}i:8;a:3:{s:4:"name";s:26:"canvasactions_definecanvas";s:4:"data";a:4:{s:3:"RGB";a:1:{s:3:"HEX";s:0:"";}s:5:"under";i:1;s:5:"exact";a:4:{s:5:"width";s:3:"260";s:6:"height";s:3:"173";s:4:"xpos";s:6:"center";s:4:"ypos";s:2:"23";}s:8:"relative";a:4:{s:8:"leftdiff";s:0:"";s:9:"rightdiff";s:0:"";s:7:"topdiff";s:2:"-9";s:10:"bottomdiff";s:0:"";}}s:6:"weight";s:2:"-8";}i:3;a:3:{s:4:"name";s:25:"canvasactions_canvas2file";s:4:"data";a:5:{s:4:"xpos";i:0;s:4:"ypos";i:0;s:5:"alpha";s:3:"100";s:4:"path";s:43:"public://portfolio-grid-bg/card-bg-echo.jpg";s:10:"dimensions";s:8:"original";}s:6:"weight";s:2:"-7";}}}

card bg img

what setup does:
  • image field is checked for orientation.
  • image is sized to fit within the whitespace
  • image is layed ontop of card bg. -- filling whitespace
dman’s picture

dman CreditAttribution: dman as a volunteer commented

Thanks for the info. Looking at it visually, I can now see what is going on.
Yup, as the comment says, when the calculations are being delegated to sub-actions, I guess the dimension become un-guessable. Though it's probably still possible for that bit of code to have tried to run the delegated sub-actions calculations also :-]

Maybe something upstream has changed to switch the dimensions from unset to NULL. I agree with fietserwin that empty() is the thing to use here. That would be a patch that could go in immediately IMO. If additional logic or error-cases were identified, that would be a follow-up to add.

I recognise your concern about that whole clause looking like an indicator of a larger problem, but in my mind, it's by design.
I don't think we are likely to be looking at a rounding error that needs to be dealt with or anything that extreme, I just think we need to be resilient enough to deal with "sometimes, the dimensions are unknown". Do your best, and don't let that break things.
At the time these rules were written, it was legal to pass back unset dimension to the image renderer and let the browser deal with it as usual. That's a fall-back state. Not ideal, but robust enough to survive unknown or unpredictable input into the future.

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
FileSize
2719661-10-division-by-zero.-change-from-isset-to-em.patch1.09 KB

in future, it would be nice if the current image path would be carried along with the $data. then a routine could just re-discover it if needed.

SKAUGHT’s picture

SKAUGHT
Primary language English
CreditAttribution: SKAUGHT as a volunteer commented
Status: Needs work » Needs review

fietserwin’s picture

fietserwin
Location Le Mont-Dore
CreditAttribution: fietserwin commented
Status: Needs review » Fixed

OK, fixed and committed. empty() is enough for those cases where the dimensions are not known. A real height of 0 will be ignored but that is such an edge case and should, I think, be prevented by the effect that causes it.

Thanks for reporting and writing the patch.

Status: Fixed » Closed (fixed)

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