Ensure context lines option is used (fixes #740)

[mattzuba]

Tests included, ensures that the context options that are set are used, rather than being hard-coded to a constant.

[mattzuba]

Looks like the get/setContextLines methods allow for possible null, but the OptionsResolver only allows an int, do you mind if I change the method signatures of these to be int only?

[ste93cry]

Looks like the get/setContextLines methods allow for possible null, but the OptionsResolver only allows an int

That's indeed another bug and I agree that the parameter should not be nullable. Maybe we can also optimize a bit the code in Stacktrace::getSourceCodeExcerpt() by checking if $maxLinesToFetch is greater than 0 and if not return early instead of opening the file anyway to read nothing. There is also another bug I'm seeing right now that is being reported by Scrutinizer: we're calling that method as if it was a static one but it's not

sentry-php/src/Stacktrace.php

Line 134 in 5ec1fbe

$sourceCodeExcerpt = self::getSourceCodeExcerpt($file, $line, self::CONTEXT_NUM_LINES);

[mattzuba]

Even if $maxLinesToFetch was 0, you'd still want to open the file to read the line that was a part of the stack though, right?

[Jean85]

Very good changes, thanks for spotting this bug!

I'm requesting just a little test improvement.

[ste93cry]

Even if $maxLinesToFetch was 0, you'd still want to open the file to read the line that was a part of the stack though, right?

As a user I would expect no lines to be sent to Sentry if the value of that option is set to 0. I may be wrong but I think that actually the number of lines also includes the line that originated the frame itself

[HazAT]

Setting it to 0 should just remove the pre and post context lines, the line of the frame in the stacktrace should still be there.
btw thanks for the work 👍

[mattzuba]

I didn't forget about this guys, just been busy at work. I'll try to get a data provider setup on the test and merge the other test and update the PR today or tomorrow.

[ste93cry]

@mattzuba are you willing to make the requested changes? We would like to release beta 2 asap and it would be cool if this bugfix was part of that version

[Jean85]

I agree with @ste93cry review comments. Apart from those, LGTM! 👍

[mattzuba]

Made the requested changes and rebased against master as well.

[Jean85]

Thanks a lot for your contribution!

I should have to add this to the changelog, but since I haven't merged #769 yet, I'll do it myself there.