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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Oct 22, 2015
Merged

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

merged 1 commit into from
Oct 22, 2015

Conversation

LaurentEsc
Copy link
Contributor

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
Copy link
Contributor Author

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

@garygreen
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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

garygreen
garygreen reviewed Oct 22, 2015
View reviewed changes
src/Msurguy/Honeypot/HoneypotServiceProvider.php
@@ -49,8 +49,14 @@ public function boot()
$translator = $app['translator'];

// Add honeypot and honeytime custom validation rules
$validator->extend('honeypot', 'Msurguy\Honeypot\HoneypotValidator@validateHoneypot', $translator->get('honeypot::validation.honeypot'));
$validator->extend('honeytime', 'Msurguy\Honeypot\HoneypotValidator@validateHoneytime', $translator->get('honeypot::validation.honeytime'));
$validator->extend('honeypot', function($attribute, $value, $parameters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't need anonymous functions anymore. Just Msurguy\Honeypot\Honeypot@validateHoneypot

@garygreen
Copy link
Contributor

Looks good to me. Tests pass?

garygreen
garygreen reviewed Oct 22, 2015
View reviewed changes
README.md
@@ -69,6 +69,28 @@ Please note that "honeytime" takes a parameter specifying number of seconds it s

That's it! Enjoy getting less spam in your inbox. If you need stronger spam protection, consider using [Akismet](https://github.com/kenmoini/akismet) or [reCaptcha](https://github.com/dontspamagain/recaptcha)

## Testing
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs updating

@garygreen
Copy link
Contributor

Once updated I would suggest to squash the commits.

@LaurentEsc
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

Indeed, seems to work.

@LaurentEsc
Copy link
Contributor Author

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
Copy link
Contributor

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

@LaurentEsc
Copy link
Contributor Author

Documentation updated and commits squashed.

@LaurentEsc LaurentEsc changed the title Make testing easier by adding a facade for the validator Make testing easier by adding a disable() method Oct 22, 2015
@garygreen
Copy link
Contributor

Looks good. 👍

msurguy added a commit that referenced this pull request Oct 22, 2015
@msurguy
Merge pull request #40 from LaurentEsc/master
dec0a28 
Make testing easier by adding a disable() method
@msurguy msurguy merged commit dec0a28 into msurguy:master Oct 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LaurentEsc @garygreen @msurguy