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

Add events tierra.topicsolved.mark_solved|unsolved_after #43

Closed
wants to merge 6 commits into from

Conversation

kasimi
Copy link
Contributor

@kasimi kasimi commented Jan 12, 2016

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

tierra commented Jan 13, 2016

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

tierra commented Jan 13, 2016

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

tierra commented Jan 13, 2016

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

tierra commented Jan 13, 2016

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

@kasimi
Copy link
Contributor Author

kasimi commented Jan 13, 2016

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

kasimi commented Jan 13, 2016

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?

@@ -40,9 +43,12 @@ public function setUp()

$this->user = $this->getMock('\phpbb\user', array(), array('\phpbb\datetime'));
$this->auth = $this->getMock('\phpbb\auth\auth');
$this->dispatcher = $this->getMockBuilder('\phpbb\event\dispatcher')
->disableOriginalConstructor()
->getMock();
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can use phpBB's builtin method for mocking the dispatcher here as more shorthand, just like main_controller_test does:

$phpbb_dispatcher = new \phpbb_mock_event_dispatcher();

@kasimi
Copy link
Contributor Author

kasimi commented Jan 14, 2016

Well, that was easier than I thought.

@tierra
Copy link
Owner

tierra commented Jan 14, 2016

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

tierra commented Jan 17, 2016

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 😞

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

@tierra tierra closed this Jan 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants