Don't be ignorant, The correct way to hook.

Discussion in 'Plugin Development' started by Nijikokun, Mar 9, 2011.

Thread Status:
Not open for further replies.
  1. Offline

    Nijikokun

    A lot of plugin authors are going about hooking into other plugins in a bad and disastrous way (Enabling the plugin before it does itself inside their plugins (can cause data corruption or worse)). Most of this is to blame to the change where the initializer was removed and a lot of plugins handle data in onEnable, yes there is still a initializer; However, it's not of much use since you cannot get any bukkit functions inside of it.

    So, since that happened a lot of plugin authors force plugins to enable, or badly hook into them by checking once and not checking when the plugin is actually enabled. This causes plugin issues, here I will teach plugin authors how to correctly hook.

    First, you will need to create a ServerListener, to do this you need the imports:

    Code:
    import org.bukkit.event.server.PluginEvent;
    import org.bukkit.event.server.ServerListener;
    import org.bukkit.plugin.Plugin;
    After that you'll want to create these privately static variables, change "PluginToHookInto" with the plugin's name / main class name.

    Code:
    private SListener SListener = new SListener(this);
    private PluginToHookInto pluginToHookInto = null;
    The ServerListener will look like so:
    (Once again change "PluginToHookInto" with the plugin's name / main class name.)
    Code:
        private class SListener extends ServerListener {
    
            private MyPlugin origin;
    
            public Listener(MyPlugin thisPlugin) {
                this.origin = thisPlugin;
            }
    
            @Override
            public void onPluginEnabled(PluginEvent event) {
                if (origin.pluginToHookInto == null) {
                    Plugin plugin = plugin.getServer().getPluginManager().getPlugin("pluginToHookInto");
    
                    if (plugin != null) {
                        if (plugin.isEnabled()) {
                            origin.pluginToHookInto = (PluginToHookInto)plugin;
                            System.out.println("[MyPlugin] hooked into PluginToHookInto.");
                        }
                    }
                }
    
            public void onPluginDisabled(PluginEvent event) {
                if (origin.pluginToHookInto != null) {
                    String plugin = event.getPlugin().getDescription().getName();
    
                    if (plugin.equals("PluginToHookInto") {
                            origin.pluginToHookInto = null;
                            System.out.println("[MyPlugin] un-hooked from PluginToHookInto");
                        }
                    }
                }
            }
        }
    This will check to see if the plugin exists (Initialized), if so check to see if the plugin is enabled (Returns false if not) and if it is, just go ahead and pass it back to our main class. It's ready to be used. It doesn't load the plugin, it doesn't mess with the plugin loading ways at all. Its a clean hook.

    Then you'll need the event hooking inside onEnable(): (Important)
    Code:
            getServer().getPluginManager().registerEvent(Type.PLUGIN_ENABLE, SListener, Priority.Low, this);

    Now just as a safety net for the plugins that load last (oh god) there is a solution, in onEnable just check to see if the plugin exists like above, or make a method for it to keep your file clean like so:
    Code:
        public void onEnable() {
            setupPluginToHookInto();
        }
    
        public void setupPluginToHookInto() {
            Plugin plugin = this.getServer().getPluginManager().getPlugin("PluginToHookInto");
    
            if (this.pluginToHookInto == null) {
                if (plugin != null) {
                    this.pluginToHookInto = (PluginToHookInto)plugin;
                    System.out.println("[MyPlugin] hooked into PluginToHookInto.");
                }
            }
        }

    That way there is no possible way of the hook not working, for those who want Permissions and lets say an alternative here is a useful way to check:
    Code:
        public boolean hasPermissions(CommandSender sender, String s) {
            if (sender instanceof Player) {
                if (Permissions != null) {
                    return Permissions.getHandler().permission((Player)sender, s);
                } else {
                    return ((Player)sender).isOp();
                }
            }
            return true;
        }
    That way you just use plugin.hasPermissions(sender, "node"); and it will default to the ops.txt or true (if console) if permissions doesn't exist.
     
  2. Offline

    andret

    Thank you very much, this helps a lot.
     
  3. Offline

    8e8

    Interesting, but I've found it easier to setup that connection when a command needs permissions. One thing I was reminded of seeing the plugin events was checking to see if Permissions get's disabled so the plugin using it doesn't throw an error when a command is run. Something I'll have to write down to remind myself to do later.
     
  4. Offline

    xZise

    Nice tutorial. There are two things I want to say. First of all I created a pull request which fires the plugin enabled event, for each previously enabled plugin to inform the plugin that another plugin is already enabled. So the “ safety” part could be removed. But is isn't integrated yet.

    Second you should mention that the plugin developer should react on disabling the other plugin. So that the plugin doesn't request the other plugin.

    Fabian
     
  5. Offline

    Raphfrk

    If we are discussing pull requests :), I did one that adds dependency checking. Plugins loading is delayed, if any dependencies haven't already been loaded.

    It also means that a plugin can extend/use any class from any plugin that has already been loaded.

    The previous version of the pull caused some issues though.
     
  6. Tnx, but permissions is still not working for me. xd
     
  7. Offline

    xZise

    But is your dependency optional?

    Fabian
     
  8. Offline

    Raphfrk

    Hasn't been pulled yet, but yeah, it is optional. You create a list in the yml of plugins that you are dependent on.

    If an attempt is made to load your plugin, it throws an UnknownDependencyException if a dependency is missing.

    If this is thrown while the plugins are loaded, your plugin will be tried again at the end. It keep trying all remaining plugins until they all fail to load.[/quote]
     
  9. Offline

    Nijikokun

    Never enable the plugin inside yours wait for it to be enabled itself.
     
  10. Offline

    darknesschaos

    I will just stay ignorant and do it my way :D
     
  11. Offline

    Afforess

    What happens, out of curiosity, if plugin X depends on plugin Y, plugin Y depends on plugin Z, and plugin Z depends on plugin X?
     
  12. Offline

    Raphfrk

    Well, it is "basic" dependencies :).

    In that case, all 3 would fail.

    It attempts to load all remaining plugins one after another. If a plugin loads successfully (or gives an exception other than a dependency exception), then it is removed from the list.

    If the list isn't empty at the end, then it tries again, but with the smaller list.

    If all remaining plugins gave a dependency failure, then it does a final pass and doesn't catch the dependency exception.
     
  13. Offline

    Drakia

    @Nijikokun You say that enabling a plugin inside your own plugin causes data corruption, etc. But that doesn't seem like it could be true with how Bukkit manages plugins. By the time my plugins onEnable() is called, every other plugin is already "initialized" but may not be "enabled".
    The next step after initialization is to go through every plugin and call the onEnable() function on them. So if I call pm.enablePlugin(permission) in my plugins onEnable() function, your plugin has already been initialized, the constructor has already been called, and everything is already setup. There is no harm in me doing this, since as far as your plugin knows it's just being enabled as every other plugin would be.

    Could you please explain your logic behind saying it's bad practice for me to enable a plugin (iConomy, Permissions, etc) myself? What exactly would cause data corruption? Why would it be any different than the plugin manager enabling your plugin?
     
  14. Offline

    Nijikokun

    It enables twice thus the data could become flawed on the second one as it has already been enabled. Lets say with iConomy the plugin is enabled, then someone else comes along and enables it again. The accounts are loaded twice now you have duplicated data in a hashmap and it saves them all twice, lets say three plugins do this, now you have three plugins force it to save three times as much data as before.

    The difference is that the plugin manager enables it once, and never touches it again. Your plugin could be reloaded multiple times (There are plugin reloaders out there as in Essentials that I have been made aware of) and this could cause tons of data issues if you are enabling plugins willy nilly. It is not your place to do so, only the plugin managers.

    It is a very bad practice and should not be done.


    Updated after speaking with Dinnerbone with a few changes, naming, removing static, to get the information pass along (this) in your listeners to grab the plugin to do plugin.hasPermission, etc.
     
  15. Offline

    Drakia

    You obviously haven't looked over the Bukkit code, as this is enablePlugin function:
    Code:
        public void enablePlugin(final Plugin plugin) {
            if (!plugin.isEnabled()) {
                plugin.getPluginLoader().enablePlugin(plugin);
            }
        }
    As you can see, it's impossible to enable a plugin twice, and therefor your reasons for plugins not doing something to this effect are invalid:
    Code:
    if (perm != null) {
        if (!perm.isEnabled()) {
            pm.enablePlugin(perm);
        }
        permissions = (Permissions)perm;
    }
    If you ask me it's a perfectly fine practice, and should be used instead of the method you use, as it's more straightforward, and leads to less problems.
     
  16. Offline

    clash

    What about plugin managers that provide options to disable plugins? Once everything is up and running, someone disables the Permissions plugin (okay, not sure why they would, but still!) -- then your plugin just re-enables it. Isn't that odd?

    I tend to agree with Nijikokun. You can check if a plugin is enabled, but you should not enable it yourself.
    Of course plugin managers violate this rule. :confused:

    Another thing that is not exactly related, but has concerned me: is there a way to unregister events? I think most plugin authors register events in their onEnable method, but should they also unregister those events in in the onDisable method? Or should the listener onXXX methods check to see if the plugin is enabled?

    And finally, thanks to Nijikokun and Drakia for developing plugins that I use regularly. [​IMG]
     
  17. Offline

    Drakia

    My plugins enable their dependent plugins in onEnable only, not in any other method, so if somebody were to disable Permissions later, it would just completely kill everything (As well as 90% of other plugins). Disabling a plugin like Permissions is risky, because once it's enabled, most plugins don't actually check to see if it's still enabled before they use it.
    Niji has an extra class, and two extra functions, that's three points of failure and three places where whoever is implementing it can screw up. If you just check for the plugins existance, enable it, make a reference to it, that's ~5 lines of code not counting braces. And it accomplishes the exact same thing. There is no difference in our code other than the fact Niji waits for the plugin to be enabled in a list, and I enable it myself.

    I'm pretty sure a while back they made it so events are only called on enabled plugins, so the event is still "registered" but it won't be called if the plugin is disabled.
     
  18. Offline

    xZise

    There is already a Pull request.

    @Drakia: Then the plugin developers should handle problems like this! For example register a listener for disabling events (and react if your permission plugin has been disabled). Also it is possible that only your plugin got enabled and not the corresponding permission plugin. So it is a bad practice if you enable the permission plugin then (maybe there is a reason, why only your plugin got enabled).

    Fabian
     
  19. Offline

    clash

    Nice! Thanks. :)

    Oh wow. Why didn't I think of that?
    I listen for enable events, so I can just listen for disable events.


    Is there a smiley for a Homer Simpson "Doh!" moment?
     
  20. Offline

    Drakia

    I see nothing wrong with enabling a plugin that my plugin depends on, and as such all of my plugins will continue to do so. Nobody has given me a valid reason to change how I handle plugin dependencies.
     
  21. Offline

    xZise

    Does the reason “the admin want to define which plugin he want to enable” not count?

    Fabian
     
  22. Offline

    Drakia

    It's pretty obvious, my plugin depends on Permissions, therefor if you enable my plugin, and HAVE permissions, permissions will be enabled.
    If you don't want permissions enabled, then remove it, don't just disable it.
     
  23. Offline

    RyanTheLeach

    As a server admin, I'd greatly appreciate a plugin that isn't a plugin manager NOT messing with my enable/disable settings, as while debugging to see if a certain plugin is causing problems or not... it just overly complicates the issue.

    Either log an error, or make it known that your plugin is also disabled/limited.
     
  24. Offline

    Raphfrk

    I added a pull request for an onLoad() method. This method is called before any of the onEnable() methods are called.

    That would allow utility plugins to perform initialisation before any other plugins are enabled.
     
  25. Offline

    bekvon

    Just so you guys know, there is a PLUGIN_DISABLE event that you can hook into. If your plugin has a dependency and that gets disabled, you could simple catch the event it and disable your plugin as well. I also do think that you should at least inform the server admin if your plugin is going to enable another plugin.
     
  26. Offline

    Nijikokun

    I'm not just joking don't enable plugins inside of yours. It's seriously a security issue with users data, and it could lead to malformed data loading.

    Read my huge long post about how data loads and how you could be one of the leading causes of data being corrupted or loading incorrectly or multiple times because you ignorantly think that you are not doing anything wrong when you have been told multiple times that you shouldn't.

    As opposed to your other post, there is a major difference between what you do and what I do. I wait until the plugin is loaded or previously loaded where as you load it, then lets say someone else loads it, and then bukkit loads it. It's loaded 3 times, 3 * the data, which would be multipled by the database calls, the file calls, you're just increasing the load that the server has to do by incorrectly hooking into a plugin.

    Stop being ignorant like the topic says.

    public PluginNameHere() {
    // Load data it's the initializer
    }
     
  27. Offline

    Drakia

    And I'm asking you to tell me exactly how it's either a security issue OR can lead to data loss. I've shown how it doesn't do either of those. Now prove me wrong and I may change how I do things.
     
  28. Offline

    Nijikokun

    Please read my post, it's all laid out in there.

    Also yours is incorrect, it would still be enabled because that is called *after* it happens. therefore you are doing nothing to prevent it.
     
  29. Offline

    Drakia

    And you need to go reread post 15 in this topic where I showed that what you say is impossible. Go look over the bukkit code before trying to order people around.
     
  30. Offline

    Nijikokun

    .... it is very much possible, and it already happens.

    I have read the coding, I even told dinnerbone *about* them loading before and he changed it to *after* so it would work with the way I'm talking about because thats the ideal that *he* had and I'm just posting about it to make you aware of it.
     
Thread Status:
Not open for further replies.

Share This Page