(14:07:15) mudrd8mz@jabber.cz/glum: Hi Tim
(14:07:25) Tim: Hi
(14:07:47) mudrd8mz@jabber.cz/glum: I just described proposed solution for https://tracker.moodle.org/browse/MDL-38230 in the tracker.
(14:08:28) mudrd8mz@jabber.cz/glum: There is a patchset in the linked issue. I would appreciate your peer-review that you kindly offered the other day.
(14:10:54) Tim: So, that new file would need to eb stored in mod/quiz/adminlib.php?
(14:11:29) mudrd8mz@jabber.cz/glum: Yes. As mentioned there, Petr started with that convention.
(14:11:51) mudrd8mz@jabber.cz/glum: The alternative would be to put it into lib/pluginlib.php of course
(14:12:12) mudrd8mz@jabber.cz/glum: adminlib.php can be seen more as a solution for non-standard mods
(14:12:25) mudrd8mz@jabber.cz/glum: Both locations are ok for me
(14:12:25) Tim: No. Informatoin about subplugins should only be ni teh plugin, even fro standard plugins.
(14:12:36) Tim: Where is the list of standard quiz access rule plugins?
(14:12:38) mudrd8mz@jabber.cz/glum: Yes, I agree.
(14:13:00) mudrd8mz@jabber.cz/glum: in lib/pluginlib.php at the moment, we do not have callback yet
(14:13:15) Tim: Ok. That should be in that class in mod/quiz/adminlib.php
(14:13:20) mudrd8mz@jabber.cz/glum: (although for this purpose, I think it is acceptable to have the static list in the core)
(14:13:36) Tim: I suppose, sinc ehte list is of standard things.
(14:13:41) mudrd8mz@jabber.cz/glum: it's still whitelist
(14:13:42) mudrd8mz@jabber.cz/glum: yeah
(14:14:19) Tim: I mean, what do we thinks should happen for forumng sub-plugins? Some forumng sub-plugins are a standard part of forumng
(14:15:14) mudrd8mz@jabber.cz/glum: Well, from core's point of view, add-on can't have something like "standard" subplugin. For core, it's all add-on
(14:15:23) mudrd8mz@jabber.cz/glum: But I see your point
(14:15:36) Tim: Well, moodle will report forumng as an add-on.
(14:15:42) Tim: https://github.com/mudrd8mz/moodle/commit/a24c3da761e84b03f2e11acef3fe622a5e8f563e commit comment too long.
(14:17:15) mudrd8mz@jabber.cz/glum: Yeah, my ViM highlighted it as well while I was writing the message. I must admit I violate it pretty often
(14:18:15) Tim: "Uninstall before settings on plugins overview screen"
(14:19:02) mudrd8mz@jabber.cz/glum: lol, I had to think for a while what you mean
(14:21:15) Tim: I don't like your API design for public function get_uninstall_url() {. You seem to be mixing up two different thigns: 1. getting the particular URL that you would use to uninstall this plugin. 2. Whether the user should see that link, becuase other things depend on this plugin. Subclasses should override 1, but not 2, so puttin ghtem in the same functoin is bad.
(14:22:30) Tim: I think you should ahve a bool function can_plugin_be_uninstalled, or is_plugin_uninstallable, and then a get_uninstall_url that always returns a URL.
(14:23:14) mudrd8mz@jabber.cz/glum: No. 2 should be overridable as well - that's what we do for quiz access rules, for example.
(14:24:02) Tim: I don't udnerstand why access rules are different from other plugins regarding 2.
(14:24:35) mudrd8mz@jabber.cz/glum: they are not
(14:25:13) mudrd8mz@jabber.cz/glum: you said: "whether the user should see that link should not be overridden by subclasses
(14:25:51) Tim: Then you said we should do that for access rules, then you said that access rules were the same as anything else. I am confused.
(14:26:12) mudrd8mz@jabber.cz/glum: I mean
(14:27:34) mudrd8mz@jabber.cz/glum: the default (safe) behaviour for plugins that do not provide own uninstall UI is that only add-ons can be uninstalled. but plugins can decide to use the new common uninstall via admin/plugins.php explicitly, too - like I demonstrated for quiz access rules.
(14:28:10) Tim: I cannot see a good reason to allow that inconsistency!
(14:28:20) mudrd8mz@jabber.cz/glum: I can.
(14:30:24) mudrd8mz@jabber.cz/glum: My main motivation for this was to provide a tool that plays well with the new add-ons installer. Uninstalling installed add-ons is reasonable requests. But uninstalling core plugin (and whether to allow it) must be well analysed by the maintainer of the subsystem - and explicitly declared.
(14:31:01) Tim: In a modular system, why should we prevent the removal of any plugin?
(14:31:30) mudrd8mz@jabber.cz/glum: because moodle is not clean modular system and you know it. core relies on some plugins being installed.
(14:31:53) Tim: A better way to handle that might be a whitelist of those plugins.
(14:32:21) Tim: Actally, in main version.php, shoould we have a ->dependencies array, just like any other component?
(14:32:47) mudrd8mz@jabber.cz/glum: could be. for now, I consider this as a safe to go solution.
(14:34:01) mudrd8mz@jabber.cz/glum: the primary use case was really that add-ons could not be uninstalled, I did not consider uninstalling core plugins much.
(14:34:24) Tim: Well, anyway, you have not commented on my proposed alternative API.
(14:34:30) mudrd8mz@jabber.cz/glum: or better, I do not want to encourage admins to do so
(14:35:21) mudrd8mz@jabber.cz/glum: Honestly, I think it adds extra complexity for not good enough reason. The caller must not forget to call both methods.
(14:35:42) mudrd8mz@jabber.cz/glum: and this is BC, too
(14:36:04) mudrd8mz@jabber.cz/glum: (although BC with a design that you do not like)
(14:36:13) Tim: Well, you could make a convenience method like get_unistall_url_if_possible.
(14:36:35) Tim: Oh. BC.
(14:37:18) mudrd8mz@jabber.cz/glum: Also
(14:37:55) mudrd8mz@jabber.cz/glum: I really do not like that get_uninstall_url() would return something even if the same class knows that that URL must not be used.
(14:38:22) mudrd8mz@jabber.cz/glum: it's like here is my credit card number but you must not look at it
(14:38:38) mudrd8mz@jabber.cz/glum: better to not return it imho
(14:39:50) Tim: OK, so public get_uninstall_url like now. INternally does if (can_uninstall) return $this->uninstall_url() else return null; uninstall_url is protected, and is the one to override. can_uninstall shoudl probably be public.
(14:39:52) mudrd8mz@jabber.cz/glum: I hope you don't mind if I copy & paste thich chat log into the tracker
(14:40:00) Tim: Of cousre.
(14:40:06) Tim: Probably good ot edit it a lot.
(14:41:02) mudrd8mz@jabber.cz/glum: no, they won't read it anyway :-p (Dear integrators, if you read this, please let me know. Thanks)
(14:42:04) Tim: I am now wondering which plugins Moodle core really cannot work  without.
(14:42:09) Tim: Other than forum.
(14:42:32) Tim: Which is weird and historical.
(14:43:03) mudrd8mz@jabber.cz/glum: well, it can work without them, but requires more careful removal procedure (like converting auth methods, migrating enrolments etc.)
(14:43:28) mudrd8mz@jabber.cz/glum: actually, it would be a good experiment to remove all polugins
(14:43:30) Tim: Well, that is idfferent. ONce a plugin is in use, we should not allow it to be deleted.
(14:43:55) mudrd8mz@jabber.cz/glum: imho the reason is that the $SITE itself it a course, which needs some format at least
(14:44:41) Tim: Except taht I don't think we acutally use the course format with site. I wonder which course format it is set to?
(14:45:10) mudrd8mz@jabber.cz/glum: topics, IIRC. marina marked it explicitly as non-uninstallable iirc
(14:45:52) mudrd8mz@jabber.cz/glum: if ($this->name !== get_config('moodlecourse', 'format') && $this->name !== 'site') {
(14:46:35) mudrd8mz@jabber.cz/glum: hmmm
(14:46:42) Tim: Except that the $site has  ->format = 'site', and format_site does not exist!
(14:47:17) mudrd8mz@jabber.cz/glum: ok, now I think I found a contra-argument that supports your API proposal
(14:47:21) Tim: Also, it would be crazy to uninstall block_settings unless you had already installed an equivalent block_mycustomsettings.
(14:48:28) mudrd8mz@jabber.cz/glum: with the current API, the core has no way to stop displaying "Uninstall" for activity modules that are required by other plugins. so we need a stopper that can't be overridden
(14:49:18) Tim: Right.
(14:49:24) mudrd8mz@jabber.cz/glum: so yeah, splitting into two separate methods would solve it.
(14:49:41) Tim: Also, you should probably count the number of instances, and prevent uninstall if there is an instance.
(14:49:58) Tim: Taht is what we do with qtypes. If you want to uninsatll a qtype, you need to find all quesiotns of that type and delete them first.
(14:50:01) mudrd8mz@jabber.cz/glum: yup, instances are not supported yet by pluginlib (there is a todo in the code)
(14:50:06) Tim: Good.
(14:51:00) Tim: Also, then, you can call teh same can_uninstall check in plugin.php, rather than duplciating the if statements.
(14:51:07) Tim: Except that plugins.php needs to display nice error messages.
(14:52:15) mudrd8mz@jabber.cz/glum: So we need to ask the base class (core) if the plugin can be uninstalled and it must have a veto right (typically because of dependencies). If it does not veto it, we ask the subclass.
(14:52:50) mudrd8mz@jabber.cz/glum: And the default implementation of subclass would be to allow it for add-ons only.
(14:53:05) Tim: Right, there should be a hook for the subclass. Do you trust the subclass to call parent::can_uninstall, or not?
(14:53:13) mudrd8mz@jabber.cz/glum: no
(14:53:40) Tim: OK. I had three other points.
(14:53:47) Tim: 2. Why does admin/plugins.php have both uninstall and delete options, and code paths, with duplicated code?
(14:54:28) mudrd8mz@jabber.cz/glum: uninstall removes DB, delete deletes files
(14:54:41) Tim: Why shoudl those be separate?
(14:55:32) mudrd8mz@jabber.cz/glum: because I want to ask the admin on removing PHP only after successful uninstall
(14:55:56) Tim: I see. that workflow makes sense, but the UI workflow was not very clear from the code.
(14:56:14) Tim: Is it easy to add a comment to make it clear?
(14:56:39) mudrd8mz@jabber.cz/glum: inline appended to optional_param() lines?
(14:57:02) mudrd8mz@jabber.cz/glum: or at the top of the code blocks
(14:57:04) Tim: Or perhpas, in the file comment at the top of the file, list all the parameters that the file can accept.
(14:57:10) mudrd8mz@jabber.cz/glum: yup
(14:57:35) Tim: In a way, a PHP scvript with optional/requried params is a bit like a PHP funciton with @params, so you should list the available ones in the file comment.
(14:57:43) mudrd8mz@jabber.cz/glum: yes
(14:58:17) Tim: 3. Should we have code like
        $pluginname = $pluginman->plugin_name($pluginfo->component);

        $this->page->set_title($pluginname);
        $this->page->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); 
in a renderer? I don't think so.

(14:58:48) mudrd8mz@jabber.cz/glum: well
(14:59:22) mudrd8mz@jabber.cz/glum: what particularly you do not like?
(14:59:42) Tim: A renderer should just produce the HTML for something.
(15:00:00) mudrd8mz@jabber.cz/glum: but this is HTML for the whole page
(15:00:18) Tim: Yes. Producing the HTML for a page != setting up the page.
(15:00:29) mudrd8mz@jabber.cz/glum: so it is navbar and title you disagree with?
(15:01:01) Tim: Also, gettting the pluginname. Strictly, that should passed in.
(15:01:20) Tim: It would be OK to say $plugin->get_name() or somethign liek that.
(15:01:42) mudrd8mz@jabber.cz/glum: right
(15:02:04) mudrd8mz@jabber.cz/glum: that's the API thing of plugininfo_* classes, it can be improved
(15:02:20) mudrd8mz@jabber.cz/glum: and ok with setting the page in the caller
(15:02:26) mudrd8mz@jabber.cz/glum: makes sens
(15:02:28) Tim: I can see that for ther parts of the code, it is nice to be able to make one call to say "In this case, setup and display this particular page", but that should eb a separate helper function that calls the renderere method. It should not be the renderer method itself.
(15:03:01) Tim: I do that with questions. Normally you call the helper $question_usage->render_question(1), rather than directly callting the questoin renderer object.
(15:04:42) mudrd8mz@jabber.cz/glum: I'll have to leave now
(15:04:53) Tim: I will just paste my last point:
(15:04:55) Tim: 4. I don't really like the API
    public function uninstall(array &$messages) {
        return true;
    }
I would prefer
    public function uninstall(progress_notifier $notifier) {
        return true;
    }
Where progress_notifier has one method $notifier->add_message("..."). But that might be over-engineering.

Or, you could do
return array($success, $messages);
to return two things.

(15:05:01) Tim: You can read that later.
(15:05:15) mudrd8mz@jabber.cz/glum: Yeah
(15:05:24) Tim: Bye.
(15:05:33) mudrd8mz@jabber.cz/glum: I alway miss a common/standard way for inter-classes communication
(15:05:39) mudrd8mz@jabber.cz/glum: I solve it over and over again
(15:07:11) mudrd8mz@jabber.cz/glum: I never know if i like returning array more than passing the collector by reference. And yes, progress logger would be nice but as I say, I would like to have something common rather than implementing it over and over again everywhere
(15:07:40) Tim: I think there is something in weblib.php already.
(15:08:15) Tim: abstract class progress_trace {
(15:08:29) Tim: That is ancient, and may be crap.
(15:08:31) mudrd8mz@jabber.cz/glum: aha, will look. You are right that we should do it nicely since day 0 as it will become part of the API 
(15:08:34) Tim: (I wrote it originally)
(15:08:56) mudrd8mz@jabber.cz/glum: for subclasses that may be out of core
(15:09:23) mudrd8mz@jabber.cz/glum: Thanks a lot for your time.
(15:09:35) Tim: No worries.
(15:09:39) Tim: Thanks for coding this.