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

The filename filter is now matching on callsite instead of implemsite #167

Merged
merged 7 commits into from Mar 19, 2018

Conversation

4 participants
@kkadosh
Contributor

kkadosh commented Mar 14, 2018

Currently, the .filename and .filename_r filters are matching in the file where the function is defined, and not where the function is called. This pull-request inverts this behaviour.

This should close #143

kkadosh and others added some commits Mar 14, 2018

jvoisin
@jvoisin

LGTM

@jvoisin jvoisin changed the title from Match filename called to The filename filter is now matching on callsite instead of implemsite Mar 14, 2018

@jvoisin jvoisin requested a review from blotus Mar 14, 2018

@jvoisin jvoisin self-assigned this Mar 14, 2018

@jvoisin jvoisin added this to the 0.3 - Dentalium elephantinum milestone Mar 14, 2018

@jvoisin jvoisin requested a review from buixor Mar 15, 2018

zend_execute_data* const execute_data,
sp_disabled_function const* const config_node,
char const* const current_filename) {
#define ITERATE(ex) \

This comment has been minimized.

@buixor

buixor Mar 19, 2018

Member

better check that ex != NULL before ex = ex->prev, smells null deref in weird context (ie. ob_start etc)

This comment has been minimized.

@jvoisin

jvoisin Mar 19, 2018

Collaborator

This check should be added right at the beginning on should_disable in my opinion.

@buixor

buixor approved these changes Mar 19, 2018

good for me 👍

@blotus

This comment has been minimized.

Contributor

blotus commented Mar 19, 2018

I'm not sure that iterating like this on execute_data will work in all cases (for example, what about functions that take a callback as a parameter ?)

@jvoisin

This comment has been minimized.

Collaborator

jvoisin commented Mar 19, 2018

I don't think that this is in the scope of this PR. What would be the intended behaviour anyway? Technically, the function isn't used in any file :/

@blotus

blotus approved these changes Mar 19, 2018

blotus added some commits Mar 19, 2018

@blotus blotus merged commit 39929ce into master Mar 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jvoisin jvoisin deleted the match_filename_called branch Mar 19, 2018

smagnin pushed a commit to smagnin/snuffleupagus that referenced this pull request Mar 26, 2018

The filename filter is now matching on callsite instead of implemsite (
…nbs-system#167)

* Add match on the file where the function is called

* Add the test

* Constify some params

* Fix potentiel null deref

* Return more before if execute_data is NULL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment