Catch DataWindow exceptions in PelIfd #114
Conversation
# Conflicts: # examples/dump-image.php
weberhofer
left a comment
There was a problem hiding this comment.
Thanks for analyzing those issues. IMHO it's good to skip broken structures if necessary.
src/PelIfd.php
Outdated
| try { | ||
| $this->sub[$type]->load($d, $o); | ||
| } catch (PelDataWindowOffsetException $e) { | ||
| unset($this->sub[$type]); |
There was a problem hiding this comment.
If you "unset" the array element a hole will be kept in the array. It could be a good idea to assign the new PelIfd object to a temporary object and insert it into the array only on access instead of removing it from the array.
test/Bug1730993Test.php
Outdated
| } catch (Exception $e) { | ||
| $this->fail('Test should not throw exception: ' . $e->getMessage()); | ||
| } | ||
| $jpeg = new PelJpeg($tmpfile); |
There was a problem hiding this comment.
Please inline the filename here, too.
test/Bug1730993Test.php
Outdated
|
|
||
| function testWindowOffsetExceptionIsCaught() | ||
| { | ||
| $tmpfile = dirname(__FILE__) . '/images/27303092-d9d72838-54f6-11e7-943c-78933f9c9fbe.jpg'; |
There was a problem hiding this comment.
Please rename that file to match it's name with the test. Could be bug1730993-2.jpg
test/Bug1730993Test.php
Outdated
| function testThisDoesNotWorkAsExpected() | ||
| function testWindowWindowExceptionIsCaught() | ||
| { | ||
| $tmpfile = dirname(__FILE__) . '/images/bug1730993_tmp.jpg'; |
There was a problem hiding this comment.
Removing the "_tmp" part of the file name would be good...
weberhofer
left a comment
There was a problem hiding this comment.
Found some more or less cosmetic issues
test/Bug1730993Test.php
Outdated
| } catch (Exception $e) { | ||
| $this->fail('Test should not throw exception: ' . $e->getMessage()); | ||
| } | ||
| $jpeg = new PelJpeg($tmpfile); |
There was a problem hiding this comment.
You can also remove the "$jpeg =" part as it's not used later on.
test/Bug1730993Test.php
Outdated
| function testWindowOffsetExceptionIsCaught() | ||
| { | ||
| $tmpfile = dirname(__FILE__) . '/images/27303092-d9d72838-54f6-11e7-943c-78933f9c9fbe.jpg'; | ||
| $jpeg = new PelJpeg($tmpfile); |
There was a problem hiding this comment.
You can also remove the "$jpeg =" part as it's not used later on.
test/Bug1730993Test.php
Outdated
| $jpeg = new PelJpeg($tmpfile); | ||
| } | ||
|
|
||
| function testWindowOffsetExceptionIsCaught() |
test/Bug1730993Test.php
Outdated
| class Bug1730993Test extends TestCase | ||
| { | ||
| function testThisDoesNotWorkAsExpected() | ||
| function testWindowWindowExceptionIsCaught() |
There was a problem hiding this comment.
Please declare the method "public"
|
@weberhofer I did a bit more refactoring to tests - I hope this is ok. The idea is to have a single test class for broken images, rather than one per bug, and to name test methods in a way to make clear which bug they cover. |
|
Great! Thanks a lot! |
Fixes #10.
Let's face it... these test files are badly broken in terms of IFD structure. This PR just captures exceptions thrown by DataWindow and skips parts of the processing, trying to load the rest of the file one way or another, but without trying to repair anything.