Skip to content

PHP Warning: Invalid argument supplied for foreach() in Image.php on line 155#23

Merged
meenie merged 1 commit into
meenie:masterfrom
elkangaroo:fix-php-warning-for-uncached-images
Aug 22, 2013
Merged

PHP Warning: Invalid argument supplied for foreach() in Image.php on line 155#23
meenie merged 1 commit into
meenie:masterfrom
elkangaroo:fix-php-warning-for-uncached-images

Conversation

@elkangaroo

Copy link
Copy Markdown
Contributor

Requesting an image, which has not been cached yet, leads to a PHP warning:

request: "GET image.jpg?resize=h[100] HTTP/1.0"
PHP message: PHP Warning:  Invalid argument supplied for foreach() in ../vendor/meenie/Munee/src/Munee/Asset/Type/Image.php on line 155
PHP message: PHP Stack trace:
PHP message: PHP   1. {main}() ../www.static/assetic.php:0
PHP message: PHP   2. Munee\Dispatcher::run() ../www.static/assetic.php:16
PHP message: PHP   3. Munee\Asset\Type->init() ../vendor/meenie/Munee/src/Munee/Dispatcher.php:69
PHP message: PHP   4. Munee\Asset\Type\Image->setupFile() ../vendor/meenie/Munee/src/Munee/Asset/Type.php:131
PHP message: PHP   5. Munee\Asset\Type\Image->checkNumberOfAllowedFilters() ../vendor/meenie/Munee/src/Munee/Asset/Type/Image.php:95

This is because glob() may return false instead of an empty array which is then passed to foreach (leading to a warning):

On some systems it is impossible to distinguish between empty match and an error.
... from the PHP documentation (http://php.net/glob#refsect1-function.glob-returnvalues)


Here's my pull request which fixes this warning:

  • Fixed "PHP Warning: Invalid argument supplied for foreach()" in Image::checkNumberOfAllowedFilters() when glob() returns false
  • Moved Image::checkNumberOfAllowedFilters() and Image::checkReferrer() calls into Image::setupFile() function
  • Removed duplicate code in Image::checkCache()

…ge::checkNumberOfAllowedFilters() when glob() returns false

- Moved Image::checkNumberOfAllowedFilters() and Image::checkReferrer() calls into Image::setupFile() function
- Removed duplicate code in Image::checkCache()
@meenie

meenie commented Aug 17, 2013

Copy link
Copy Markdown
Owner

Thanks for this, I'll have a look now.

@ghost ghost assigned meenie Aug 19, 2013
@elkangaroo

Copy link
Copy Markdown
Contributor Author

This also fixes 404s on uncached images, if you set the option "numberOfAllowedFilters" to 1 (in the case where glob() returns false, because count(false) == 1).

@meenie

meenie commented Aug 22, 2013

Copy link
Copy Markdown
Owner

Hi Alexander,

I haven't had the time to completely look at your pull request do to work commitments. I hope to get it all merged in this weekend :).

Just wondering though, the return from glob(), can you not just check to make sure it's an array with is_array() before trying to use it? And if it's not, then we know that it should be an empty array instead of false and change it to an empty array()? Then there isn't the need to make all the other changes to accommodate?

@elkangaroo

Copy link
Copy Markdown
Contributor Author

can you not just check to make sure it's an array

That's exactly what I am doing :) (lines 157-159)

  •    if (! is_array($cachedImages)) {
    
  •        $cachedImages = array();
    
  •    }
    

The other parts is more or less just refactoring (removing duplicate code, move filter checks out of "checkCache()" method, don't call "checkNumberOfAllowedFilters()" if there are filter requested).

I'll annotate them.

@meenie

meenie commented Aug 22, 2013

Copy link
Copy Markdown
Owner

Well I feel silly now haha. I only had a very brief look ><. Okay well, I'll merge this in on the weekend.

Thanks again!

@elkangaroo

Copy link
Copy Markdown
Contributor Author

Well I feel silly now haha. I only had a very brief look ><.

Haha don't feel silly! Thanks for having a look.

I hope the annotations help you see my intentions for the changes.

@meenie

meenie commented Aug 22, 2013

Copy link
Copy Markdown
Owner

I've just went through it and these are great changes! Can't believe I didn't use parent::checkCache() for the placeholder before ><. Guess it was a rushed implementation when I put it in.

I'm just going to merge these now and create a new release.

Thanks a ton! Your pull requests are welcome any time 👍.

meenie added a commit that referenced this pull request Aug 22, 2013
…mages

Fixing a PHP Warning: Invalid argument supplied for foreach() due to glob() sometimes returning false rather than an array.  Plus some refactoring changes to remove duplicate code.
@meenie meenie merged commit 8b5be4b into meenie:master Aug 22, 2013
@meenie

meenie commented Aug 22, 2013

Copy link
Copy Markdown
Owner

1.5.15 is now released :) - http://mun.ee/Changelog

Thanks again!

@elkangaroo elkangaroo deleted the fix-php-warning-for-uncached-images branch August 22, 2013 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants