Skip to content

Excluding a sniff by path is not working #2465

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

Closed
wants to merge 1 commit into from

Conversation

lmanzke
Copy link
Contributor

@lmanzke lmanzke commented Mar 28, 2019

This is one of the places where the strict comparison breaks things. If you exclude a sniff in the XML via this line, the strict = true causes in_array to return false as it is iterating over XMLElements.

There might be other places where this is also wrong, so I suggest doublechecking every place affected by c280c14 if the behavior is still the desired one.

@lmanzke
Copy link
Contributor Author

lmanzke commented Mar 28, 2019

To still use strict comparison, one could also just use forEach to determine if the element is in the array.

@jrfnl
Copy link
Contributor

jrfnl commented Mar 28, 2019

@lmanzke Could you provide an example so I can reproduce what was broken by this ?

@gsherwood
Copy link
Member

I think this might be caused by excluding sniffs by path and not code. But the required change here is not to remove the strict check - I need to figure out why a SimpleXMLElement object is being included in the array at all.

@gsherwood gsherwood changed the title Do not use strict comparisons when working with XMLElements Excluding a sniff by path is not working Apr 1, 2019
@gsherwood gsherwood added this to the 3.4.2 milestone Apr 1, 2019
@gsherwood
Copy link
Member

The ruleset logic for including sniffs already cast these objects to strings, but the excluding logic didn't. I've fixed this now, so the strict comparison will now pass. Thanks for reporting this.

@gsherwood gsherwood closed this Apr 1, 2019
@lmanzke
Copy link
Contributor Author

lmanzke commented Apr 3, 2019

Thanks for fixing - sorry I didn't respond ealier @jrfnl - was too busy...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants