Add rule suppression annotations

[redbeardcreator]

This is a first pass at adding annotations to handle suppressing rules, as per #10. I do not consider it done, or even close to it. I expect there will be at least one or two refactoring passes, maybe more, as this is kind of brute force. I also expect that the annotation format might change.

That said, it does work and implements ignore and no-ignore annotations (namespaced to @psecio\parse\ignore and @psecio\parse\no-ignore). The annotations can be applied to any code level as long as you are using a doc block. The annotation parsing is also case-insensitive at the moment. So the following should all be valid:

/**
 * @psecio\parse\ignore DisplayErrors
 */
class Foo
{
    /**
     * @psecio\parse\ignore GlobalsUse
     */
    public function bar()
    {
        /** @Psecio\Parse\Ignore logicaloperators */
        if ($a and $b) {
            echo "BAZ!";
        }
    }
}

I've included a couple of files to test against in tests/testFiles/annotation-tests/. There's also in-code notes on how the annotation parser can be made more versatile.

What's missing

(that I'm aware of)

• reporting errors on invalid annotations or bad rule names
• unit tests
• --ignore-annotations

For rule names, I'd like to eventually have it complain if either of the following occur:

• Disabled rule: rule exists but disabled on the command line
• Invalid rule: rule does not exist

Possible refactorings

(in no particular order but numbered for reference):

1. Pull annotation parsing into a helper class.
2. Somehow "plug in" to CallbackVisitor rather than occupying it. This could be hooks, or additional callbacks added. It's a little tricky, because the visitor needs to know what the annotation parsing adds and removes.
3. Something else?

Conclusion

I'm open to any and all suggestions. I'll probably be working on this again tomorrow night, but it's hard to say for sure.

[hanneskod]

Wow, first of all: this is really awesome!

I am looking at the travis build, and it fails in just the right places 😄

A few humble thoughts:

• @psecio\parse\no-ignore could maybe be called @psecio\parse\force
• I agree that the annotation parsing should be in a separate class. It could be used something like:

$docBlock = new DockBlock($node->getDocComment());
$docBlock->getIgnoredRules();
$docBlock->getForcedRules();

I am sure there are many docblock parsing libraries out there that could be used for this, if you are interested that is..

• Case-insensitive is good.
• I am totaly fine with adding this kind of behaviour to CallbackVisitor. The reason a added the callback was I wanted to dispatch events when issues were found, but I didn't want to inject the dispatcher into the visitor. But if you want to add error reporting on invalid annotations or bad rule names I think it could be an idea to inject the dispatcher after all. Then you could maybe emit debug events on annotation failures, or something like that. And CallbackVisitor would be just a Visitor, responsible for keeping track of and evaluating ruls. It's an idea anyway.
• Documentation is also missing.

Great work!

[redbeardcreator]

Thanks for the feedback...

I am looking at the travis build, and it fails in just the right places 😄

Duh! 😳 The good news, those are just samples that I had intended to remove before completion. Maybe we should put them in an example directory. Or, tell Travis to ignore tests/testFiles when scanning 😉

Of course, the unit tests will take the place of these files.

@psecio\parse\no-ignore could maybe be called @psecio\parse\force

Yeah, I don't like no-ignore much, but that's how many CLI tools negate an option. Not sure I like force either, though. Maybe include? Or something else? It needs to feel like the opposite of ignore. Or maybe ignore needs to be renamed too.

I agree that the annotation parsing should be in a separate class.
...
I am sure there are many docblock parsing libraries out there that could be used for this, if you are interested that is.

I will take those parsing ideas in mind. Still do not know for sure where I'll go with it. As far as parsing libraries...I haven't found one that isn't part of an enormous library and not really separable. I will look more, of course.

But if you want to add error reporting on invalid annotations or bad rule names I think it could be an idea to inject the dispatcher after all.

I'll keep that in mind. I'm still getting used to the modern architecture. I've only ever worked with what can now be called legacy PHP applications. I like it, it's just a different way of thinking. Not that I didn't know about it when I first started doing PHP (having read the GoF book), just that I've never applied it.

Documentation is also missing.

It's coming, I promise! But thanks for the reminder 😄

I'm going to try to work some more tonight, but we'll see. It can be tough when I have to get four kids to bed before working 😀

[hanneskod]

Yeah, I don't like no-ignore much, but that's how many CLI tools negate an option. Not sure I like force either, though. Maybe include? Or something else? It needs to feel like the opposite of ignore. Or maybe ignore needs to be renamed too.

Hm. include sounds like the best suggestion so far.

PHPMD uses @SupressWarnings http://phpmd.org/documentation/suppress-warnings.html

PHPCS uses @codingStandardsIgnoreStart and @codingStandardsIgnoreEnd

As far as parsing libraries...I haven't found one that isn't part of an enormous library and not really separable.

I would like to suggest https://github.com/eloquent/blox. I have not used it myself. But it's small and has no dependencies. It can also parse the text body of a dockblock, and I am thinking about using class level docblocks for long Rule descriptions. Its just an idea at the moment, but if blox is good for you I could probably use it as well.

If this is something you are happier implementing yourself you should of course do that!

I'm going to try to work some more tonight, but we'll see. It can be tough when I have to get four kids to bed before working

Wow, I can imagine! I have a five months baby that wakes every 90 minutes or so. I get my stuff done while waiting for him to wake 😄

[hanneskod]

Maybe @enable/@disable?

[redbeardcreator]

I like @enable/@disable. I'll keep it namespaced, of course.

I'll look into blox. There's no point implementing a parser if there's already something usable available.

As far as kids go, eventually the adapt to a "regular" sleeping schedule. But right now I'm getting onne sick after another. But this, too, shall pass.

[redbeardcreator]

After a quick overview, it looks like blox will work...with one small fix. It needs to handle namespaced tags. Seems to be an easy fix, so I'll do the PR against it tonight probably and use my clone in the meantime. Or semi-permanently if the PR isn't accepted 😁

[redbeardcreator]

Some improvements:

• Switch to using @\psecio\parse\enable and @\psecio\parse\disable.
• Implement --disable-annotations. I switched this from ignore because I think it fits with the other verbiage better.
• Move the examples into an example directory. This has two purposes. One, to provide a specific examples to the user of Parse. Two, pass the Travis build 😉
• Add some documentation in including/excluding rules. Please let me know what you think on that.
• Add single-letter aliases for all the scan options.

Still to do:

• Unit tests!
• Use blox for DocBlock parsing.
• Error reporting (e.g. bad rule name).

Given that blox will probably be handling the annotation parsing, I don't know if I'll split out DocBlock handling as much as @hanneskod suggested. I just don't think it's necessary at this time. But maybe when I get there I'll change my mind.

That's it for tonight. Hopefully there will be more tomorrow.

[hanneskod]

For ultimate coolness you could move the examples to README.md. If the code is surrounded by php tags scanning the README itself will work once #53 is merged.

<?php
/**
 * Turn off the rule for the following function
 *
 * @psecio\parse\disable EvalFunction
 */
function needsEval()
{
    eval('echo "I need eval()\n"');
}

/**
 * Previous rule should not affect this one.
 */
function evalIsBad()
{
    eval('echo "Eval is Evil!"');
}
?>

[hanneskod]

Given that blox will probably be handling the annotation parsing, I don't know if I'll split out DocBlock handling as much as @hanneskod suggested.

Agreed.

Add some documentation in including/excluding rules. Please let me know what you think on that.

I think it looks good.

[redbeardcreator]

Putting the examples in the README is an interesting idea. What about putting short examples there, single functions and a larger file to demonstrate the interaction of multiple levels of blocks?

[redbeardcreator]

I just did an initial cut using blox. Unfortunately, blox does not support comment blocks inside the method body. So I don't think I can use it with out extensive modification.

[redbeardcreator]

See my comment on #56... I think blox is still going to work; it's just going to take some more work.

[redbeardcreator]

Still not done...but I successfully added blox. As I've mentioned before, I had to modify BloxParser to get it to work with the namespaced tags. I also had to modify it to work with short doc blocs: /** @psecio\parse\enable RuleName */ which I think will be very handy if you want to mark single statement.

What's left:

• Unit tests (requires refactoring CallbackVisitor to inject the DocBlock parser)
• Logging invalid rule names; possibly also log invalid tags in the psecio\parse namespace.
• Add a few examples into the main README.

I think this is functionally done. There's still clean up, but the basic usage is complete. The only end-user visible code change will be logging errors.

[hanneskod]

Oh cool. Blox it is then!

If I understand it correct \PhpParser\Node::getDocComment() also handles comments like // @tag ... and even # @tag .... How would the current blox implementation handle that?

[hanneskod]

And about logging invalid rule names. #56 contains a few updates to RuleCollection that maybe will come in handy. Specifically a case insensitive has($ruleName) method.

[redbeardcreator]

Right now blox won't handle those other comment types. I'll have to work on that. For now it will be in the comtext of a blox PR.

I agree, the has() method should be useful. I should be able to look at it more later today.

[hanneskod]

https://gist.github.com/hanneskod/2a159e00af69c0cebdec

What do you think about that? It's very crude and maybe there are so many edge cases that it's not worth pursuing (I was just having a bit of fun I guess..). But if using blox means you have to rewrite it we could maybe just as well do something ourselves that work both in #52 and #56.

[redbeardcreator]

I don't think blox needs a rewrite...just some tweaks. There's a regex that just needs to be taught the additional possibilities.

However, I just started looking into blox and found that it's relatively out-of-date as per current standards. I wasn't even able to easily run the test suite as there's no bootstrap for it and no phpunit.xml file. And while I just merged #56, I'm not opposed to switching out now.

I'll take a look at your gist this evening, as I'm out of time on my lunch break 😄

I'm still not opposed to using blox, either. I just think it needs to be brought up-to-date a bit. And it certainly works fine for #56 as is.

[redbeardcreator]

Just an FYI. Obviously this is progressing. I'm working on adding unit tests but have hit a snag or two. I think I have it figured out, but won't be able to work on it again until later this weekend (I don't have my most up-to-date work with me). If I can't work it out I'll push up my problem code and get some help.

[redbeardcreator]

Things left:

• Warnings on bad rules
• Warnings on bad annotations (e.g. something other than disable or enable)
• Additional unit tests

Tests left are for nested annotations, case sensitivity, and the allowing of comments; e.g.

/**
 * @psecio\parse\Disable Rule  We know what we're doing
 */
class C
{
    /**
     * @psecio\parse\enable rule  But here, it shouldn't be allowed
     */
    public function f()
    {
    }
}

[hanneskod]

I haven't looked at this code for I while now... but I wonder what the annotation comments are used for? Would they be presented to the user in some way?

Another idea could be to disable/enable multiple rules using a single annotation:

 /**
  * @psecio\parse\enable rule1 rule2 rule3
  */

[redbeardcreator]

My initial thought was to allow the user of the annoation to explain why it was being endabled or disabled right there with the annotation. So:

/**
 * @psecio\parse\disable MysqlRealEscapeString Needed until full transition to prepared statements
 */

This lets you better document the use of an unsafe construct.

Your idea of multiple rules in a single annotation has some merit, although I wonder how often it would be useful. I know in my experience, either the use is bad and should be fixed or there's a single exception. Or the entire system relies on an identifed construct and it should be disabled at the project level. Hopefully the latter gets resolved eventually.

[redbeardcreator]

I think this is ready to go. Possible sticking points:

• Doesn't provide warnings on invalid rules. This is going to take some refactoring and change the basic operation of CallbackVisitor so it can issue events and/or messages. @hanneskod mentioned some ideas on this previously.
• Comments. It may be better to require a mark between the doc block entry and the comment, so we can expand the meaning of the doc block later. I'd propose //. Another option is to not allow anything to come after the rule. The second option is probably easier to refactor later.

If you're ok with both those points, then it's ready. If either of the points causes problems, then more discussion is needed.

To reiterate, this is ready to go, but there will be refactoring later.

[hanneskod]

Cool. I'll have a look some time this weekend. This will be a great addition!

[redbeardcreator]

I made comments on annotations explicit. In other words, to add a comment, add a comment separator:

/**
 * @psecio\parse\disable requestuse // Right now it can come from either $_GET or $_POST.
 */

As the commit note says, not only does this make the comment explicit, it also makes backward compatibility easier in the future.

[redbeardcreator]

I believe this feature is done, sans warning/errors on invalid rules, and maybe some debug logging. But it will be too big of a jump to get there from here without first getting this branch merged in. So, if I could get a review, I would appreciate it.

In the meantime, I'm working on some improved code coverage (see my humbug branch). Part of that branch is improving bug coverage using mutation testing via humbug. It seems to be going really well, with 97.8% coverage overall, and 100% coverage on all but the Rules, which get 88.9% coverage. And with the mutation testing, it's really close to a true 100% coverage.

[enygma]

Nice, I'll try to take a look at this later on. :)

[redbeardcreator]

This is just a ping. If no one's opposed, I'd like to merge this this weekend.

[hanneskod]

I am sorry I have been absent here. And I am sad to say I will have to stay absent for a while longer...

[redbeardcreator]

Been quiet for awhile...I'm confident in these changes, so I'm merging. Will work on getting #62 in shortly.