Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving sub plugins design #178

Open
dmitriim opened this issue May 9, 2023 · 8 comments
Open

Improving sub plugins design #178

dmitriim opened this issue May 9, 2023 · 8 comments

Comments

@dmitriim
Copy link

dmitriim commented May 9, 2023

Rather than having sub-plugins (trigger, steps) committed to the plugin, I'd like to propose changing the design of managing subplugins so other plugins can add triggers, steps without actually code being committed to this plugin.

This can be implemented by using callbacks or relying on a class names. So the main plugin becomes more like API for others to extend it. This doesn't limit to have bunch of useful "subplugins" inside the main plugin and follow the same design. E.g. see that example https://github.com/catalyst/moodle-tool_dataflows/blob/MOODLE_35_STABLE/classes/manager.php#L36

Basically nothing should be changed for the end users and existing functionality should work exactly as it does now.

Pros:

  • this change will add a new level for developers for extending this plugin in some specific cases that couldn't be applied to the plugin sue to being very specific or a client no willing to open source the change.

Cons:

  • some cool features may potentially stay outside of the plugin as a developer won't willing to push changes upstream.
@NinaHerrmann
Copy link
Contributor

I don´t really see the improvement, triggers and steps can have subplugins - if you want to implement it.
What do you want to implement? 🤔

@dmitriim
Copy link
Author

dmitriim commented May 9, 2023

sorry @NinaHerrmann I should be more specific.

Currently triggers and steps must be in the folders defined in db/subplugins.json https://github.com/learnweb/moodle-tool_lifecycle/blob/master/db/subplugins.json#L3 This is ok and this is how Moodle designed suplugins. However, this approach got few disadvantages. In particular when you want to have a functionality very specific for being part of that plugin (will never be accepted as a pull request). If that happens developers have to fork lifecycle plugin, implement their changes in the fork and then support forked version rather than spend the same effort on supporting main lifecycle plugin.

There is a way to avoid this. It's getting commonly used recently across different plugins (a callback workaround is in the initial description).

So the improvement here would be changing a way how tool_lifecycle works with subplugins. In particular how subplugins are getting discoverer. After the change, there will be no need for suplugins to be in the paths described in db/subplugins.json, but any plugin will be able to implement a trigger or a step functionality by implementing a callback and returning a list of expected classes.

That will help keeping very specific (not suitable for merging) functionality outside of the plugin.

The initial intention for me proposing this change is trying to avoid having multiple plugins doing the same thing. Currently we have few big clients looking at the archiving functionality for their fat moodle instances. We could start a dev work from scratch and implement a nice new plugin that would do everything that tool_lifecycle does. But we strongly believe that a better approach would be investing in the existing tool_lifecycle that does 80% of what's we require, rather than have multiple clones of the same functionality. However, there is a blocker in the current design of tool_lifecycle that could slow us down, so we are proposing this change.

Hope that all make sense.

@dmitriim
Copy link
Author

dmitriim commented May 9, 2023

What do you want to implement?

Unfortunately, I don't have all requirements at the moment as it's early stages of scoping. But from the experience I can tell that 100% there will be something that will never be accepted as PR due to being very specific. Also you may not want to have all that new stuff in the plugin as you will need to support all that code.

@NinaHerrmann
Copy link
Contributor

Hey @dmitriim,

I guess you can not say what specific logic you want to implement but you have to be more precise to explain why this functionality can not be expressed as a step or trigger. Is it some recursive call? Is it some if/else logic? If you do not want to make that public we can make a short call.

@dmitriim
Copy link
Author

I think to continue with this we actually should draft a PR to demonstrate what we mean with this change. Will see if we could get one done in the next few weeks.

@NinaHerrmann
Copy link
Contributor

Yes and no, we want to go into the use cases before we merge pull requests.

@tuanngocnguyen
Copy link

Hi @dmitriim @NinaHerrmann ,

I have created a PR for this issue: https://github.com/learnweb/moodle-tool_lifecycle/pull/189/files
Please let me know your opinion on the patch.

@tuanngocnguyen
Copy link

Hi @NinaHerrmann

An use case I can think of:

Client has an external system (beside Moodle), that determines when a course should be backed up/deleted. This can be solved by creating new trigger plugin. However, the implementation is specific to that client, and they don't want to publish the plugin.
We can add this plugin in subplugin folder ('trigger'), but since we are using 'git' to manage our code, we cannot add new trigger as a 'git submodule' without changing lifecycle code base. With the patch, we can do so, hence easier to maintain.

Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants