Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Avoid division by zero (for issue #708) #1095

Merged
merged 7 commits into from

5 participants

@Buddhima

File: libraries/joomla/image/image.php
Method: prepareDimensions()

Issue: If zero is given for the one of the arguments $height or $width, following section will produce an error.
Solution: Treat 3 instances where $height=0,$width=0 and both not equals zero

Signed-off-by: Buddhima Wijeweera

Buddhima added some commits
@Buddhima Buddhima for issue #708
avoid division by zero

Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
fe13727
@Buddhima Buddhima commit 2 for issue #708
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
70f50da
@Buddhima Buddhima comments are aligned and changes to one elseif
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
6d8ea6c
@eddieajau

There are a lot of style errors in this file. Have a look at this report:

http://developer.joomla.org/pulls/logs/1095checkstyle.xml

Search for the following line:

Any error that is not complaining about an underscore needs to be fixed. Thanks.

libraries/joomla/image/image.php
((4 lines not shown))
+
+ // To avoid dividing by $width if it is zero
+ if($width==0 && $height!=0){
+ $rx = $this->getWidth() ;
+ $ry = $this->getHeight() / $height;
+ }
+
+ // To avoid dividing by $height if it is zero
+ elseif ($width!=0 && $height==0) {
+ $rx = $this->getWidth() / $width;
+ $ry = $this->getHeight() ;
+ }
+
+ // Both $height and $width are zero
+ elseif($width==0 && $height==0){
+ $rx = $this->getWidth() / $width;

This cannot be correct: $width is 0, so this will divide by 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@benjaminpick

Also, it would be good to explain in the Doc-Header what a zero value means.

@Buddhima

Hi,

What I thought regarding this matter was there should be a value for rx when width =0. And I know when values getting closer to 0 , rx should tends to infinity. Therefore I can't specifically say a value. Then I thought of as a standard, assign rx with the value given by getWidth(). Same thing goes for ry too.
But I got to know that throwing exceptions from classes like this is allowed.

So my new solution would be this.
Since this this method consider 2 cases for SCALE_INSIDE and SCALE_OUTSIDE there mustn't be a situation where user is given a chance of entering values less than 1.
Anything designing on top of this should obey that rule. If this zero values are coming from an application, it should be a mistake done by developer.
Another case might be issues with web browser. Following links [1], [2] would show that.

My solution is to throw exceptions in those places where height or width would be zero.

[1] http://name1price.com/how-to-get-customers-with-singapore-web-design-basic/jquery-tutorials/149-jquery-get-image-height-or-width-returns-0-problem-.html
[2] https://getsatisfaction.com/galleria/topics/could_not_extract_width_height_from_image_in_webkit_browsers

Special thank goes to Elin for providing necessary guidance.

@Buddhima Buddhima changed to throw LogicException
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
fb45362
@elinw

Couldn't this be simplified to
if ($height == 0 || $width == 0)

@Buddhima Buddhima Replace 3 if cases with one and corrected style mistakes
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
7df127c
@elinw elinw referenced this pull request
Closed

images scaled by Edge #1130

libraries/joomla/image/image.php
@@ -654,6 +654,8 @@ protected function getFilterInstance($type)
*
* @return object
*
+ * @throws LogicException If width,height or both given as zero

I think you have space indenting on these two lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
libraries/joomla/image/image.php
@@ -671,9 +673,20 @@ protected function prepareDimensions($width, $height, $scaleMethod)
case self::SCALE_INSIDE:
case self::SCALE_OUTSIDE:
- $rx = $this->getWidth() / $width;
- $ry = $this->getHeight() / $height;
-
+
+ // Both $height or $width cannot be zero
+ if($width == 0 || $height == 0)

Space needed after the if.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
libraries/joomla/image/image.php
@@ -671,9 +673,20 @@ protected function prepareDimensions($width, $height, $scaleMethod)
case self::SCALE_INSIDE:
case self::SCALE_OUTSIDE:
- $rx = $this->getWidth() / $width;
- $ry = $this->getHeight() / $height;
-
+
+ // Both $height or $width cannot be zero
+ if($width == 0 || $height == 0)
+ {
+ throw new LogicException(' Both height or width cannot be zero ');

This should be an InvalidArgumentException

throw new InvalidArgumentException('Width or height cannot be zero with this scale method.');

Also adjust the DocBlock. Thanks.

@Buddhima
Buddhima added a note

thanks eddieajau ,
Corrected all mistakes and committed !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Buddhima Buddhima Changed to invalidArgException and indentings
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
8a4bdda
@elinw

The pull tester says you have white space on three lines.

http://developer.joomla.org/pulls/pulls/1095.html

@chdemko chdemko merged commit 3e0aaa6 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 1, 2012
  1. @Buddhima

    for issue #708

    Buddhima authored
    avoid division by zero
    
    Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
  2. @Buddhima

    commit 2 for issue #708

    Buddhima authored
    Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
  3. @Buddhima

    comments are aligned and changes to one elseif

    Buddhima authored
    Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
Commits on Apr 4, 2012
  1. @Buddhima

    changed to throw LogicException

    Buddhima authored
    Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
Commits on Apr 6, 2012
  1. @Buddhima

    Replace 3 if cases with one and corrected style mistakes

    Buddhima authored
    Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
Commits on Apr 9, 2012
  1. @Buddhima

    Changed to invalidArgException and indentings

    Buddhima authored
    Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
Commits on Apr 15, 2012
  1. @Buddhima
This page is out of date. Refresh to see the latest.
Showing with 15 additions and 2 deletions.
  1. +15 −2 libraries/joomla/image/image.php
View
17 libraries/joomla/image/image.php
@@ -654,6 +654,8 @@ protected function getFilterInstance($type)
*
* @return object
*
+ * @throws InvalidArgumentException If width,height or both given as zero
+ *
* @since 11.3
* @throws InvalidArgumentException
*/
@@ -671,8 +673,19 @@ protected function prepareDimensions($width, $height, $scaleMethod)
case self::SCALE_INSIDE:
case self::SCALE_OUTSIDE:
- $rx = $this->getWidth() / $width;
- $ry = $this->getHeight() / $height;
+
+ // Both $height or $width cannot be zero
+ if ($width == 0 || $height == 0)
+ {
+ throw new InvalidArgumentException(' Width or height cannot be zero with this scale method ');
+ }
+
+ // If both $width and $height are not equals to zero
+ else
+ {
+ $rx = $this->getWidth() / $width;
+ $ry = $this->getHeight() / $height;
+ }
if ($scaleMethod == self::SCALE_INSIDE)
{
Something went wrong with that request. Please try again.