Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serializeable closure support #23

Merged
merged 10 commits into from
Apr 24, 2024
Merged

Conversation

rubenvanassche
Copy link
Member

@rubenvanassche rubenvanassche commented Apr 23, 2024

Serializeable closures can break backtraces, lets fix this!

Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Only some minor nitpicks 👍

$frames[] = new Frame(
$snippet = null;

if (substr($currentFile, 0, strlen(ClosureStream::STREAM_PROTO)) === ClosureStream::STREAM_PROTO) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of that constant 👍

src/Frame.php Outdated
@@ -41,22 +51,24 @@ public function __construct(
$this->class = $class;

$this->applicationFrame = $isApplicationFrame;

$this->snippet = $snippet;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename this to textSnippet? To make it really clear that this isn't a CodeSnippet object.

@@ -157,7 +159,7 @@ public function it_can_get_the_snippet_properties()
$snippet = $firstFrame->getSnippetProperties(5);

$this->assertStringContainsString('$firstFrame =', $snippet[2]['text']);
$this->assertEquals(154, $snippet[2]['line_number']);
$this->assertEquals(__LINE__ - 5, $snippet[2]['line_number']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better!

@rubenvanassche rubenvanassche merged commit a5834be into main Apr 24, 2024
42 checks passed
@vovayatsyuk
Copy link

Doesn't the "laravel/serializable-closure" dependency needs to be in the "require" section? It is in the "require-dev" section now.

@rubenvanassche
Copy link
Member Author

Good point, taught it was only used in testing but the constant is a problem here. I'll fix it!

@rubenvanassche rubenvanassche deleted the serializeable-closure-support branch April 24, 2024 13:14
@freekmurze
Copy link
Member

@vovayatsyuk good point

@rubenvanassche It might be better to add a class_exists check where appropriate instead of requiring the package.

@rubenvanassche
Copy link
Member Author

@freekmurze Yeah good idea!

@rubenvanassche
Copy link
Member Author

Fixed

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.

3 participants