msurguy / Honeypot

Simple spam prevention package for Laravel applications
maxoffsky.com/code-blog/implementing-honeypot-spam-prevention-in-laravel-applications/
MIT License
429 stars 44 forks source link

Make testing easier by adding a disable() method #40

Closed LaurentEsc closed 9 years ago

LaurentEsc commented 9 years ago

This PR is related to issue #39 and is a suggestion to allow easy Validator mocking:

HoneypotValidator::shouldReceive('validateHoneytime')
    ->once()
    ->andReturn(true);

I have updated the documentation.

LaurentEsc commented 9 years ago

Moving the HoneypotFacade to another namespace is a breaking change, probably not the best idea ...

garygreen commented 9 years ago

To be honest I think at this point it would make sense to just get rid of the HoneypotValidator.php and move those 3 functions into Honeypot.php and update the tests. That way there isn't a need for two facades which seems overcomplicated imo.

garygreen commented 9 years ago

Also I think I would prefer to see something like Honeypot::enable() and Honeypot::disable() as oppose to the suggestion of mocking the validator functions.

LaurentEsc commented 9 years ago

Something like this ? I also like Honeypot::disable() for testing.

garygreen commented 9 years ago

Looks good to me. Tests pass?

garygreen commented 9 years ago

Once updated I would suggest to squash the commits.

LaurentEsc commented 9 years ago

The problem with using Class@Method instead of a closure is that is creates a new instance to validate, so disable() doesn't work.

garygreen commented 9 years ago

Gotcha. I'm pretty sure the validator resolves the class thru the IOC so in theory if you did honeypot@method it should work.

LaurentEsc commented 9 years ago

Indeed, seems to work.

LaurentEsc commented 9 years ago

Tests didn't pass after I cloned:

1) Msurguy\Tests\HoneypotTest::test_get_honeypot_form_html BadMethodCallException: Method Mockery_0_Msurguy_Honeypot_Honeypot::getFormHTML() does not exist on this mock object

garygreen commented 9 years ago

It should be generate, tests prob has been failing on that for a while.

LaurentEsc commented 9 years ago

Documentation updated and commits squashed.

garygreen commented 9 years ago

Looks good. :+1: