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

Script recursion detection #29

Merged
merged 2 commits into from
Oct 3, 2016
Merged

Conversation

jedkirby
Copy link
Contributor

@jedkirby jedkirby commented Oct 3, 2016

The $parentName variable isn't ever used, so the comparison on L86 would never return true.

I've modified this to use the name of the script as the comparison against the value of the command minus the @ character.

I've also removed the recursion of calling hasInfiniteRecursion back on it's self as this throws warning because the value of $scripts[$command] is not a multi-dimensional array.

Tests have not been modified because the check for infinite recursion is replicated within the ScriptRunner and the hasInfiniteRecursion method isn't ever checked within any tests - might be a good idea to test this, though.

James Kirby added 2 commits October 3, 2016 11:42
…for more detail.

- Removal of the $parentName parameter, as it's not used.
- Checking whether the key of the commands array is a string/integer, string denotes a key => value array, which is likely to be a named function.
- Removal of the recursion call as it didn't initially work, and it doesn't need to happen as we can't nest that far anyway.
@tompedals
Copy link
Contributor

The hasInfiniteRecursion method was tested within ScriptsExtensionTest when loading the config, but previously passed and still passes. Not sure what the failing test case would be. This change looks good to me though.

@tompedals tompedals merged commit 3c3e729 into master Oct 3, 2016
@tompedals tompedals deleted the feature/script-recursion-detection branch October 3, 2016 12:59
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.

2 participants