allow for line breaks when splitting xrefs for id and position

[Connum]

When looking at #19 and testing with the last PDF file provided there, I found out that the issue came from the raw data having a line break between the xref id and position instead of a space. That resulted in an empty string instead of a number for the position, resulting in an invalid argument for the substr() function call.

This fix splits for any regex \s character(s) instead of just a space, which resolves the issue.

Please note that the CI check will expectedly fail for test case testGetDataTmIssue336 while PR #343 is pending.

edit: fixes #19

[k00ni]

Thank you!

• Can you provide a simple test that outlines the problem and demonstrates the fix? That would be great.

[Connum]

I tried before making the PR, but it's not that trivial, unfortunately... In order to unit test parseObject(), we need to provide $structure and $document as well, so we'd need to add a test file and do the complete parsing process on it.
I tried manipulating one of the existing sample files into having a line break in the xref position, but it seems those are not in the raw data that is human-readable, but somehow encoded... I don't know how else we could test this for now, other than parsing the provided file (#19 (comment)) manually before and after the fix (which I did of course).

[k00ni]

You could create a sub class in the test file, which calls the protected method for you, like:

// Parser.php
class Parser {
  protected function parseObject() {
    // secret stuff, you want to test
  }
}

// in the test file
class ParserSub extends Parser {
  public function exposedParseObject(...) {
    return $this->parseObject(...);
  }
}

class ParserTest ... {
   public function testParseObject()
   {
       $fixture = new ParserSub();
       $fixture->exposedParseObject(...);
   }
}

Source: https://stackoverflow.com/a/249776/5301527

This way you could inject a prepared Document instance, so the new code is being used. What do you think?

[Connum]

This way you could inject a prepared Document instance, so the new code is being used. What do you think?

Amazing, thanks for the input! That way, I managed to create a test case that reproduces the error message before the fix, and passes after the fix. It was a bit tricky, and I also extended the TestCase base class to allow catching errors that are normally not catchable via a try/catch statement, like E_WARNING or E_NOTICE (the latter of which we had in this case). This might come in handy for future tests as well!

[k00ni]

I think that is a little bit out of scope. Is it really necessary to create a list of PHP error types? Why not just print the error message and available meta data.

[Connum]

Yeah, it might be a bit over the top... I thought it could be helpful to see the type of error before the error message in a human-readable way when writing the tests. But if you think it's too much, I can remove it! Please let me know again.

[k00ni]

Please remove it here. But it could be worth looking into it again in another PR.

[k00ni]

Thank you for effort to add a test!

I made some in-code comments.

[Connum]

I think I found an easier test mechanism now, simply checking for the parsed object in the test data. I also merged the master branch so we can finally see the CI build passing.

[k00ni]

Good job! It looks so much cleaner now.

Is your work done here? If so, I will keep this open for a few days to allow our community to comment.

[Connum]

Yes, all done, thanks for your support!