Skip to content
This repository has been archived by the owner on Jul 20, 2018. It is now read-only.

Add checks for batcache variant #282

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

devonliu
Copy link
Contributor

Add checks per issue #153:

A variant that includes one of
include|require|echo|print|dump|export|open|sock|unlink||eval or does
not include a reference to a superglobal (regex check for$_`) will
fail and we should flag those early.

Test case also added

Add checks per issue Automattic#153:
> A variant that includes one of
include|require|echo|print|dump|export|open|sock|unlink||evalor does
*not* include a reference to a superglobal (regex check for$_`) will
fail and we should flag those early.

Test cases also added
@@ -0,0 +1,12 @@
<?php

die(); //Don't actually run the following code.
Copy link
Contributor

Choose a reason for hiding this comment

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

We can safely omit the die()...this only gets read in as text, not included/required as PHP so it won't be executed.

devonliu added 4 commits May 27, 2015 13:56
Improve error descriptions on batcache variant error:
- indicate the illegal word that causes the warning
- mention superglobal that is missing
- delete "die()" as this file is not likely to be included/required as
php for execution
- add a no-error positive test case
- Assert only on the slug part from the error, so description can be
flexible
- Separate test files into two to allow for two separate testings
- Remove "assertFalse" from CodeCheckTestBase.php to allow for testing
of positive test cases
@devonliu
Copy link
Contributor Author

While updating the testing part I noticed a few oddities:

  • The document included in this repository contains some of the concerns that were commented earlier in this pull request, namely: comparing entire error array word for word, and including die() in test case files
  • CodeCheckTestBase.php does not support positive test case, there's an assert statement that checks to make sure error returned is non-empty before proceeding. I have removed the statement so that positive test case can be included, not sure if there's a reason it was there in the first place that I may have overlooked. This also means every test that extends this class (about half of all tests currently) has no positive test case.

As it currently stands almost all test classes for vip-scanner come in one of 2 kinds: the ones that extend CodeCheckTestBase, and the ones that extend CheckTestBase. The latter does not support PHP-Parser, but the former comes with the two oddities mentioned.

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

Successfully merging this pull request may close these issues.

2 participants