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

[POC] Objectified module management #271

Closed
wants to merge 1 commit into from
Closed

Conversation

pupi1985
Copy link
Contributor

The idea of this POC is to replace the module management system with an object oriented approach. This eases understanding on how plugins and the core interact and removes some code from the huge qa-base.php bag. The code is also encapsulated, information is hidden as much as possible, many calls such as method_exists are removed, etc.

Sadly, this is everywhere in the core, so the minimal atomic change to make would impact everywhere and leave non-working sections. That's the reason of the huge commit. So get a comfortable chair and some popcorn. Needless to say, don't expect documented code, as this is just a POC. However, you can enjoy some diagrams :)

These are some random notes I've taken while making the changes:

  • Share class names and filenames to avoid needing to hardcode both
  • All modules depend on a plugin (eg: system modules didn't use to depend on a plugin)
  • Modules with same name in different plugins will not collide
  • Added a new module type (Setting) which is what used to be the admin_form and to hold default values for options
  • The global variable $pluginManager should be a member of a singleton application instance but that is part of a different pull request
  • The widgets table might need to have the "title" field turned into a "moduleid" field and maybe some extra characters
  • A DB upgrade might be needed to migrate the current modules IDs (widgets, captcha, search, editor modules) to the new ones in the "pluginId-moduleId" format
  • Removed the db_connected hook as it can never be called if the plugins need to access the database. E.G.: in order to check if a plugin needs to run initialization it needs to access the DB. So the call to check the tables would be called even before the Process module is even instantiated. I think this scenario might be similar to hooking a process module to a db_connected call and add an init_queries method (I haven't tested this though)
  • Some changes needed to be done in qa-index. The plugins need to have a valid urlToRoot set. It is needed to initialize some global variables to do so ($request, $relativeroot). Problem is the Q2A_* classes are CURRENTLY being created before the variables are set. It is needed to change that, making sure the ajax calls still work the same way. I added an approach to fix that but I'm not quite happy with it

These are the steps involved in plugin initialization:

Instantiate plugin
If plugin has invalid metadata
    return
If plugin requires database initialization
    return
Initialize plugin
Post initialization (after all plugins have been initialized)

The post initialization is actually a one level dependency management approach. If plugin B depends on A, then B can use A during post initialization. The thing is that if a plugin C depends on B there is no post post initialization. That could be solved making each plugin explicitly mention their dependents and running the initialization in steps, being the amount of steps the longer dependency chain. I was just a bit lazy to work on that and wasn't so critical.

Class diagram:
classdiagram

Gliffy Source: https://gist.github.com/pupi1985/82fb76a90640cce3c1f9#file-classdiagram-gliffy

And a short flow diagram about plugin initialization: http://yuml.me/edit/42e2816e

I guess something similar can be done to overrides and phrases but this seemed to be the first testable checkpoint. Layers are being discussed in a different pull request.

@pupi1985 pupi1985 changed the title Objectified module management [POC] Objectified module management Jul 13, 2015
@pupi1985 pupi1985 force-pushed the patch-63 branch 2 times, most recently from 08ead37 to cfb7fe3 Compare July 18, 2015 04:58
@svivian svivian added rfc Request for comments and removed refactor Issue requires a large code change labels Aug 8, 2015
@svivian
Copy link
Collaborator

svivian commented Aug 8, 2015

Interesting stuff here. Only had a quick look through the code, but are the "install" parts for a full plugin manager/installer? I mean like uploading plugins through the admin interface? Or is it just for the DB initialisation?

I do worry about you writing so much code that may not get used... ;) It would be nice if we could implement some of these ideas gradually while keeping backwards compatibility. If we take the example of plugins extending a base class: we can add a base class that matches the current "API" from the docs, then authors can update their code to extend that class, then in a later version we can "require" that they extend that base class (i.e. remove the method_exists calls).

A lot of this stuff will be best left until we drop 5.2 support and can use namespaces, but if you have ideas for small changes we can discuss them (before you spend hours/days implementing them :))

@pupi1985
Copy link
Contributor Author

pupi1985 commented Aug 9, 2015

The install parts are called like that because they are related to the Q2A install procedure, e.g., appear in the install.php file. I think before having the ability to install plugins it would be better to have a component that can handle that and focus on all plugin-related operations. That's one of the reasons I thought on adding the PluginManager.

I do worry about you writing so much code that may not get used... ;)

Just don't. It is much better to see if something is possible or not by making it. If I have the time to do it, I'll do it.

It would be nice if we could implement some of these ideas gradually while keeping backwards compatibility

Absolutely. However, this POC, as all of them, focus on v2.0. In order to plan what changes should be implemented to reach an ideal release you first need to define what ideal release would be. Then apply minor changes progressively. But if you don't know where you're heading to, you might take small steps into a wrong direction.

Also bear in mind that, being strict in what release numbers represent, before 2.0 you only have 1.8 and 1.9. There are not many releases left, so the minor changes (which might not be so minor) will have to be thought carefully.

Even in the worst case scenario in which there is a backwards incompatibility, take into account there aren't thousands of plugins around so maybe it might be better to fork the repos and perform minimal (and usually mechanical) changes on the plugins rather than keeping an old-style core.

@svivian
Copy link
Collaborator

svivian commented Aug 9, 2015

Also bear in mind that, being strict in what release numbers represent, before 2.0 you only have 1.8 and 1.9. There are not many releases left, so the minor changes (which might not be so minor) will have to be thought carefully.

Actually if we're following SemVer we can do 1.10, 1.11 and so on before 2.0. I don't think it will be too confusing. Or we can roll on to 2.* anyway and put any BC-breaks into 3.0. There's an infinite supply of numbers :)

@svivian
Copy link
Collaborator

svivian commented Apr 4, 2017

Closing this for now but will certainly pick up the idea some time in the future.

@svivian svivian closed this Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants