Add warnings for undesired changes to plugin hook return values

[TheWitness]

Describe the bug

The Cacti plugin API is dependent on plugin authors returning certain structures to be used by other plugin authors. These structures should ALWAYS be returned. Otherwise errors can be thrown by plugins that assume that they are set.

In the event this happens, Cacti should make every effort to detect this and warn the user who can then either correct this issue, or report it to the plugin maintainer.

Expected behavior

Cacti should do a better job of detecting cases where plugin authors are not using good form when authoring plugins.

Screenshots

Below is an example of the new error that will be logged in the case of a chaining event when there is greater than one plugin using the hook function.

[TheWitness]

The WARNINGS above are just an example. Since this plugin was the only plugin calling the hook, it will not generate a warning even though it is using bad form.

[jdcoats]

So after updating, I noticed that the monitor plugin no longer showed the monitor tab. I disabled monitor and re-enabled and got your warnings. So syslog, monitor and weathermap need some love.

2021/09/06 16:24:18 - CMDPHP WARNING: Plugin hook 'syslog_top_graph_refresh' from Plugin 'syslog' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:18 - CMDPHP WARNING: Plugin hook 'weathermap_top_graph_refresh' from Plugin 'weathermap' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:18 - CMDPHP WARNING: Plugin hook 'monitor_top_graph_refresh' from Plugin 'monitor' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:16 - CMDPHP WARNING: Plugin hook 'syslog_top_graph_refresh' from Plugin 'syslog' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:16 - CMDPHP WARNING: Plugin hook 'weathermap_top_graph_refresh' from Plugin 'weathermap' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:14 - CMDPHP WARNING: Plugin hook 'syslog_top_graph_refresh' from Plugin 'syslog' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:14 - CMDPHP WARNING: Plugin hook 'weathermap_top_graph_refresh' from Plugin 'weathermap' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:14 - CMDPHP WARNING: Plugin hook 'monitor_top_graph_refresh' from Plugin 'monitor' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:06 - CMDPHP WARNING: Plugin hook 'syslog_top_graph_refresh' from Plugin 'syslog' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:06 - CMDPHP WARNING: Plugin hook 'weathermap_top_graph_refresh' from Plugin 'weathermap' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:24:06 - CMDPHP WARNING: Plugin hook 'monitor_top_graph_refresh' from Plugin 'monitor' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:23:25 - CMDPHP WARNING: Plugin hook 'syslog_top_graph_refresh' from Plugin 'syslog' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:23:25 - CMDPHP WARNING: Plugin hook 'weathermap_top_graph_refresh' from Plugin 'weathermap' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.
2021/09/06 16:23:25 - CMDPHP WARNING: Plugin hook 'monitor_top_graph_refresh' from Plugin 'monitor' must return the calling array or variable, and it is not doing so. Please report this to the Plugin author.

[TheWitness]

Okay. Let me dig in to these.

[netniV]

Part of the functionality of the API was that a plugin can effectively cancel a hook by returning false. As such, I do not think we should be reporting these warnings except at a debug level (or maybe high at a push). It's been useful to have it log always here as we have had immediate feedback from @jdcoats (tanks for that!) but we should correct the logging level for release.

[TheWitness]

@jdcoats, can you post the contents of this function weathermap_top_graph_refresh?

[TheWitness]

In the case of the top_graph_refresh, an array is sent in, but an integer is sent back. So, the test case would be null only I think.

[jdcoats]

    function weathermap_top_graph_refresh( $refresh )
    {
        if ( basename( $_SERVER[ "PHP_SELF" ] ) != "weathermap-cacti-plugin.php" )
            return $refresh;

        // if we're cycling maps, then we want to handle reloads ourselves, thanks
        if ( isset( $_REQUEST[ "action" ] ) && $_REQUEST[ "action" ] == 'viewmapcycle' ) {
            return ( 86400 );
        }
        return ( $refresh );
    }

[TheWitness]

It's bad logic in the lib/plugins.php commit something later tonight. Keep an eye out.

[TheWitness]

Should be all set now.

[jdcoats]

updated and warnings are gone, will continue on testing.

[netniV]

Commit 63e1998 is still wrong. As I commented on there, there are scenario's where the changing of the return value type between array and string is expected and desired.

Also, the part about if (cacti_sizeof($result) > 1) would always be true as $result is the hook array data

[netniV]

Why has this been closed without addressing the above?

[TheWitness]

Re-read my comment. We need to log a new issue. This one is closed for the reasons I stated above.

[netniV]

You haven't stated any reason above, just that it's all set. I'm going to assume that the $result only kicks in if there are more than one listeners for a given hook? But if that's the case, that would be better as the outer IF statement since it's less complex and not as likely to be true unless you have a lot of plugins that all use the same hook.

[TheWitness]

Yea, it was in the commit comment. Sorry about that.

[netniV]

That's OK, just trying to make sure we are both following things correctly and not missing something. My understanding of the hooks logic there has changed each time I read it but I think I have it now. It will be curious to see who exactly does see the warning.