Please upgrade here. These earlier versions are no longer being updated and have security issues.
HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.

Broken with mysql strict mode

fcheslackfcheslack New
edited February 2016 in Vanilla 2.0 - 2.8

Vanilla now supports mysql strict mode in 2.2. BetterNotifications does not because it is passing a bool to a tinyint resulting in an error.

Gdn_ErrorException: Incorrect integer value: '' for column 'Notified'

It looks like this was supposed to be addressed in the SetNotified function, by using $FlagValue, but after it is set, it never gets used.
No more error thrown if you change
->Set('Notified', $Notified)
to
->Set('Notified', $FlagValue)

I'd submit a pull request, but https://github.com/jamesinc/BetterNotifications is empty

related: https://github.com/vanilla/vanilla/issues/3507

Comments

  • This got caught in the spam filter.

    Sorry.

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

  • Hey, I've just seen this. Let me get the github repo working. I'm looking at this plugin anyway as I've noticed a 2.2 compatibility issue.

  • Okay, sorry about the repo not being there. I think I threw this plugin together in a day or two and never pushed it to origin. Unfortunately I have no idea where the original repo was.

    It's now here: https://github.com/jamesinc/better-notifications/

  • @fcheslack BetterNotifications 2.0.3 has now been released, and addresses the issue you raised.

  • @jamesinc Awesome. Thanks. And thanks for putting the license in there too. I had just put a copy up on github myself and was feeling a little uneasy about doing so without there being a license specified.

    Can I also suggest that the description be updated? It says it's only for bookmarked discussions but (as far as I can tell) it works for any comment notifications. That's exactly the behaviour I was looking for, but I had to check the code to make sure it actually did what I wanted.

  • @fcheslack I can't remember off the top of my head how comment notifications are organised. If I recall, for my use case, users were receiving notifications for bookmarked threads. Any thread that they had started or commented in was bookmarked automatically (using a plugin called AutoBookmark I think).

    But yes, I suppose if you set your notification preferences to maximum verbosity, you'd be receiving notifications for comments in all threads, and the plugin would quash those too.

    I'll update it all over the weekend and make it more release-y.

  • Yeah, what I meant was all comments you'd normally be notified for, however a user set those preferences.

  • Compatible with 2.2? Thanks!

Sign In or Register to comment.