@Zeichen32 opened this Pull Request on July 24th 2015 Contributor

See issue #8422

@tsteur commented on July 27th 2015 Owner

Looks good to me. I reckon we should maybe add some tests after this merge see https://github.com/piwik/piwik/issues/8422#issuecomment-124572582 . It's possible we break someone's plugin (or our own plugins) with this change in case one had a typo etc. We'd have to mention it in the changelog. Don't think it needs to wait till Piwik 3.0, any other opinions?

@Zeichen32 commented on July 27th 2015 Contributor

I am on holiday this week, but I will add some tests next week. So we can tests all core plugin events.

@diosmosis commented on August 24th 2015 Member

Hi @Zeichen32 are you still planning to add a test? If you don't have time, let me know and I'll add it before merging.

@Zeichen32 commented on August 24th 2015 Contributor

I have started to write the tests. But i still have problems to get a list of all plugin event. :cry:

@diosmosis commented on August 24th 2015 Member

I believe @tsteur's comment was about going through all plugins via Plugin\Manager::getLoadedPlugins() and calling getListHooksRegistered() to get the events plugins listen to, then making sure that, if they are Controller... events, that the controller action exists.

Does this help you?

@tsteur commented on August 25th 2015 Owner

Exactly. I haven't tested it but something like this should roughly work:

foreach (Plugin\Manager::getLoadedPlugins() as $plugin) {
    $hooks = $plugin->getListHooksRegistered();

     foreach ($hooks as $hook => $callback) {
           if (0 === strpos($hook, 'Controller.')) {
                 // here you explode $hook to get controller module and action
                list($controller, $module, $action) = explode('.', $hook);
                 // then you can assert the controller exists like this:
                try {
                    $resolver   = new ControllerResolver(StaticContainer::getContainer());
                    $controller = $resolver->getController($module, $action);
                } catch (\Exception $e) {
                    $this->fail("$hook is listening to a controller method that does not exist");

@Zeichen32 commented on August 26th 2015 Contributor

Ah okay, thanks for the hint. During the course of the day i will add the commit.

@Zeichen32 commented on August 27th 2015 Contributor

Ive add the test and squash the commits. :+1:

@tsteur commented on August 28th 2015 Owner

Looks good to merge. Have you tested whether it actually fails if there was an event listening to it? I think in Piwik itself there's none so far.

I'm wondering if it's good to merge for 2.15 or 3.0. It may break other plugins if the event wasn't correctly spelled or if the URL doesn't match the case

@Zeichen32 commented on August 28th 2015 Contributor

Yes, I have tested it. For test purpose i had added some test events because currently Piwik does not listening on any Controller Event.

@tsteur commented on August 30th 2015 Owner

@mattab I reckon it's your call whether we should merge in 2.15 or 3.0 re possibly breaking URL's and maybe events

@mattab commented on August 31st 2015 Owner

Thanks for reminding me re: BC break. We'll target 3.0.0 then :+1:

@mattab commented on September 18th 2015 Owner

Hi @Zeichen32 - would you mind re-opening the PR against the 3.0.0 branch?

@Zeichen32 commented on September 21st 2015 Contributor

@mattab Done :+1:

This Pull Request was closed on September 18th 2015
Powered by GitHub Issue Mirror