Skip to content

Add dd_trace_reset function, and stop throwing exceptions when overriding missing methods #222

Merged
pawelchcki merged 12 commits into
masterfrom
do_not_fail_on_missing_methods
Jan 8, 2019
Merged

Add dd_trace_reset function, and stop throwing exceptions when overriding missing methods #222
pawelchcki merged 12 commits into
masterfrom
do_not_fail_on_missing_methods

Conversation

@pawelchcki
Copy link
Copy Markdown
Contributor

Description

  • dd_trace_reset simply resets the lookup hashes to initial state, allowing removing of existing overrides - mostly usable for tests

  • Exception throwing behavior is controlled by ddtrace.ignore_missing_overridables ini setting that defaults to true

Readiness checklist

@pawelchcki pawelchcki requested review from SammyK and labbati January 7, 2019 18:00
@pawelchcki pawelchcki force-pushed the do_not_fail_on_missing_methods branch from 14198c0 to 3d50896 Compare January 7, 2019 18:04
@pawelchcki pawelchcki force-pushed the do_not_fail_on_missing_methods branch from 3d50896 to 817478d Compare January 7, 2019 18:07
@pawelchcki pawelchcki force-pushed the do_not_fail_on_missing_methods branch from 03991cd to 6655feb Compare January 7, 2019 19:05
Copy link
Copy Markdown
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Thanks @pawelchcki!
Just a added a couple of comments if they make sense to you.

Comment thread src/ext/ddtrace.h Outdated
@@ -0,0 +1,55 @@
--TEST--
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think that makes sense to add a test to make sure that the exception is not thrown? Something like

--TEST--
Check tracing of non existing classes, methods and functions.
--FILE--
<?php

class ClassWithNoMethods
{
}

dd_trace("ClassNotExist", "method" ,function(){});
dd_trace("ClassWithNoMethods", "method", function(){});
dd_trace("function_not_exist", function(){});

?>
--EXPECT--

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ops I missed two files when submitting this PR. I'll do better next time. One of those files checked exactly what, you've been proposing.

@labbati labbati added this to the 0.9.1 milestone Jan 8, 2019
@labbati labbati added the c-extension Apply this label to issues and prs related to the C-extension label Jan 8, 2019
labbati
labbati previously approved these changes Jan 8, 2019
Copy link
Copy Markdown
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@@ -1,13 +1,38 @@
--TEST--
Toggle checking if overridable method/function exists or not
Do not throw exceptions when veryfying if class/method and function existst.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

existststststst 😄

Copy link
Copy Markdown
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this! 👍

@pawelchcki pawelchcki merged commit c69f9eb into master Jan 8, 2019
@pawelchcki pawelchcki deleted the do_not_fail_on_missing_methods branch January 8, 2019 12:14
bwoebi pushed a commit that referenced this pull request Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c-extension Apply this label to issues and prs related to the C-extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants