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

Rollback country check #29 #30

Closed
wants to merge 1 commit into from
Closed

Rollback country check #29 #30

wants to merge 1 commit into from

Conversation

sergejmueller
Copy link

@sergejmueller sergejmueller commented May 2, 2016

Fixes #29

Copy link
Member

@bueltge bueltge left a comment

Choose a reason for hiding this comment

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

Looks fine, but we should change small thinks for the WP Codex, but this is more a global topic, so that we merge it before.

@Zodiac1978
Copy link
Member

Version has to be changed to 2.7.0 and the conflict has to be resolved, but the PR/feature itself is fine and I would be very happy if we can bring this feature back to the plugin.

@websupporter
Copy link
Contributor

websupporter commented Jan 17, 2017

I have checked this feature out and tested it a bit. I think too it is good to go. The only thing I was thinking about:

035e26d#diff-33ae96de74050319957e1ac65615cf29R1697
Basically when you do activate this feature without giving a black or whitelist every comment will be spam.

And right now, when the API is not available, all comments will be marked as spam (1713 - 1720):
035e26d#diff-33ae96de74050319957e1ac65615cf29R1713

If we can't reach the API because it might be down, maybe we should let it pass? But for both my points I think you could also go with the opposite view. I just would be a bit more lose here.
[Reviewer mislead. See Sergejs comment below, 🤦‍♀️ ]

@sergejmueller
Copy link
Author

Basically when you do activate this feature without giving a black or whitelist every comment will be spam.

return false; will marked the comment as no-spam, see the function doc.

@schlessera
Copy link
Contributor

Replaced by #61

@schlessera schlessera closed this Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants