tierra / topicsolved

phpBB Topic Solved extension: Allows posting questions, and accepting answers as solved.
https://www.phpbb.com/customise/db/extension/topic_solved/
GNU General Public License v2.0
23 stars 19 forks source link

Add events tierra.topicsolved.mark_solved|unsolved_after #43

Closed kasimi closed 8 years ago

kasimi commented 8 years ago

As mentioned in #38, here are the events. For moving your solved and locked topics only the tierra.topicsolved.mark_solved_after event is required, but for the sake of completeness I've also added tierra.topicsolved.mark_unsolved_after. Maybe it'll come in handy in the future.

Regarding the @since annotation, let me know if you need me to change the version.

tierra commented 8 years ago

This looks pretty good initially here, I can look at updating the affected unit tests soon here if you don't get to those first.

tierra commented 8 years ago

EPV is complaining about the event name, but I think it's actually because the version of EPV I have pinned in Composer is getting old most likely, and just needs a bump.

The remaining unit test failures are just because the tests need to inject the dispatcher everywhere they use the TopicSolved class you just added it to, so they should be easily fixed.

Lastly, I'd like to get in the necessary assertions that verify that the new events are in fact being triggered when they should be, and with the appropriate data.

tierra commented 8 years ago

Oh, nevermind about EPV being outdated, it's actually using exactly the latest version, on the same commit that I actually contributed to EPV myself, specifically phpbb/epv@a0545399

I'll have to investigate that a little further, but the event names look perfectly valid to me based on the EPV checks themselves, so I don't know why it's not matching them correctly.

tierra commented 8 years ago

Oh, and the @since tags should be perfect with 2.2.0. I think I was planning on making that the next release.

kasimi commented 8 years ago

EPV expects the data to be extract()ed, see here. I left that out because the events are at the very end of the methods.

kasimi commented 8 years ago

It seems there are more problems. I'm not really familiar with testing and I I'm not sure what these errors are about. Is there anything I need to fix?

kasimi commented 8 years ago

Well, that was easier than I thought.

tierra commented 8 years ago

Just wanted to say thanks one more time for bringing this idea to my attention, it's a very elegant solution to handle moving topics in an extensible way.

This looks good enough to merge now, but I just need a little more time to actually test it out myself when I have a minute, and also so that I can finish writing up new unit tests to cover the new hooks. Once that's done, I've actually been overdue to get a new release out the door with 4 or 5 new translations since the last release as well, so I'll be planning on getting that submitted very shortly. It should be a quick turnaround time on an official release that administrators can use.

tierra commented 8 years ago

I ended up using $this->getMockBuilder('\phpbb\event\dispatcher') after all since it turns out that phpBB's phpbb_mock_event_dispatcher isn't actually a mock class :disappointed:

Anyway, I've tested this now, and have test coverage for it now. It's now merged in 15629681