Bugzilla – Bug 1602
plugins that fail initPlugin should NOT be disabled permanently
Last modified: 2008-09-15 14:37:04 UTC
currently if initPlugin fails, this seems to cause the plugin to be added to the disabledplugins pref. In the case of itunes, this means that the itunes settings page is gone, when it was supposed to be there in all cases. depite a bug report asking for it to be removed, it is in fact needed in many cases in order to point to the xml file for the many occasions where it is not found automatically ( of course, one decision could be to no longer support that, but it should be a decision and not just happen in absence of anyone paying attention). In cases of other plugins, like musicmagic, this bug means that if starting the server without musicmagic running, the plugin is then disabled and if you want to run later, you have to specifically re-enable. Far easier, and in fact used to behave this way, was to only disable for that instance of the server. Thus, if on the next run, the plugin is valid, it will just work without having to be re-enabled.
But how do we keep track of its disabled state (as it's actually disabled due to a missing parameter)? As this is imho very iTunes specific wouldn't it be better to do do some change in the plugin itself? I think it used to create the settings stuff itself in enabled() -> initPlugin() -> addGroups() independantly of the disabled state. I'll have a look at this.
as i explained, it is also relevant to musicmagic. It used to be specifically turned off from setup.pm without adding to the pref (thus temporary). this latest change also annoys the hell out of me when doing development work, as any error then requires specifically re-enabling the plugin, when this was specifically not required before (i wrote the code such that it was this way, the same code you have often called a mess). I've noticed this a while back, and eventually plan to fix when I'm less annoyed about it and feel like someone is once again at the helm to actually direct things.
It's not a missing initialisation, but it being disabled again: when addSetupGroup is called, it will call disablePlugin(). But if we really need that setting, why don't we just remove the code which is removing it from the settings page? We never need to do this, if we really want to keep the setting, do we? I think you were right when you said that commenting out line 233 should work. It works for me: I never had iTunes enabled, but the page is always there.
just have to make sure that the setting tab is there, but that the Use iTunes setting at the bottom of server settings should only be there if it should be there. I am not sure if that should be present if the initPlugin fails or if the plugin is disabled. again, that design decision really comes down to Dean. The exact appearance of iTunes stuff is because of his instructions.
committed the commented line at change 3290 in order to let Roy have a test. I tried to fake a condition where the library was not found, but I was still losing the settings, despite all logic saying the tab should have remained.
as for not disabling permanently, I forgot to mention the file types settings. those may 'fail' when there is no binary found, yet they are not 'disabled' unless specifically done by a user. The previous plugin method wasn't quite the same (it would disable if you changed another plugin setting) but the intent was along those lines. Perhaps in the case of plugins, they should be removed from the list when they fail for some reason, yet not added to the disabled plugins pref. this way, when they work again they return to their previous setting state. eventually, I'll look at this one myself.
There different ways a plugin can fail: it may be broken (eg. not 6.x compatible) or enabled() may return. In the first case it's added to a %brokenplugins hash, to the disabledplugins list and _not_ checked any more until the nest server start. If enabled() returns 0, a plugin is added to the disabledplugins list. Kevin, what you want is no automatic addition to the disabledplugins list for plugins which do not return enabled()? Something like a tempory (runtime only) list of disabled plugins? We'd then only have plugins in the disabledplugins pref which were disabled by the user.
that sounds right. however, I'm not sure that the broken case doesn't also affect the disabled plugins prefs. while working on the random plugin, if I had a typo I expected it to fail loading, but after fixing the typo, I would still have to go back and re-enable. If this is by design, then I'll have to get used to that. My main issue is with plugins (like musicmagic) that do tend to not always be fully connected, but should always be enabled until the users says to disable. In this case, it is returning enabled 0 due to initPlugin. Fixing this as you have described would be fine. if change 3290 works for Roy, then I'll call this one fixed and let the issue drop :)
Broken plugins are currently added to the disabled list. If it's by design, I don't know. But it's like that right now. But how could you replace a plugin without restarting the server? Once it's require'd it won't load again, will it?
well, there is the load on the fly...but I'm not talking about that. I was restarting the server. Start server without musicmagic running, and it seems to be automatically disabled. restart later with musicmagic running and you have to re-enable. develop a plugin, enable it, restart after a change and if it fails to require, its disabled (rightfully so) and taken from the list. fix the error and restart, have to remember to go into settings to re-enable. My gut feeling is that it shoudl behave just as if you had added a new plugin and be enabled. I think it will bite us in the behind to have plugins become part of the disabled prefs automatically. for instance, users upgrade and have old plugins. they are then disabled as part of being invalid. They then download the new version and its still disabled. They then complain that it still doesnt work becuase they wouldnt' think to have to enable it again, since they never disabled it. of course, you and I are always around to tell people to re-enable, so its fine either way.
Created attachment 524 [details] don't automatically add disabledplugins pref for broken plugins I think removing Buttons::Plugins.pm lines 146-148 will do what you want. As long as you don't touch the plugins settings a broken plugin won't be disabled. That pref should only be written after a change to the settings page. Is this ok? I successfully tested this with your scenario: have a plugin enabled, update to a broken version, fix and restart -> it's still enabled.
Created attachment 526 [details] add itunes tab always, but use option only if fully init that looks right. thanks, michael. i've made the attached changes to itunes so that the setup tab shows always, but the Use iTunes option is only present if the plugin is fully initialised.
With disabled iTunes plugin I don't have anything. After enabling it I get the iTunes tab. After fixing the path I should get the "use iTunes" option? This is very ok with me - not using iTunes I don't have any iTunes options :-).
what say we merge both of these and see what the itunes users think of it? ;) committed the itunes patch at change 3300
Committed Plugins.pm patch with change 3301.
There are 536 bugs in the database with targets of '---' that were fixed prior to new year 2006. I am setting them to targets of 6.2.1 to keep them from showing up in my queries.