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

Plugin proposition #275

Merged
merged 26 commits into from
Nov 8, 2015
Merged

Plugin proposition #275

merged 26 commits into from
Nov 8, 2015

Conversation

ArthurHoaro
Copy link
Member

What is this

I retrieved @nodiscc plugin system proposed in #164 and changed it to create PHP plugin system. It relies on hooks triggered by certain actions (only template rendering for now).

It's only a proposition, let me know what you think of it.

Why

  • I think that an "only template" plugin system might be too restrictive, and doesn't allow a lot of extension.
  • I raised concerns in [archived] General discussion #44 and don't blend too well with user made templates.
  • @nodiscc lacks of time to finish this.
  • I'd like to see 0.9beta release one day. :)

How it works

Hooks

PluginManager class includes enabled plugin PHP files at loading and register their hook function.

When we want to trigger a hook, PluginManager::executeHooks() is called, eventually with rendering data. It will call every plugin function registered under the standardized name:

hook_<plugin_name>_<hook_name>

Rendering data can be altered and/or completed.

Rendering

This is exactly what @nodiscc did.

Templates contain plugin display at specific location, which are populated by the plugin functions.

Implemented

Here is what's has been done:

  • hook_render_linklist: when linklist is rendered, all links data passed to plugins. It allows plugins to alter link rendering (such as Markdown parsing). They can also add a link_plugin icon for every link (QRCode, etc.)
  • hook_render_header: every page builder triggers this hook. Plugins can add specific data to header if the current page is concerned (toolbar).
  • hook_render_footer: : every page builder triggers this hook. Plugins can add specific data to header if the current page is concerned (JS file).
  • hook_render_includes: : every page builder triggers this hook. Plugins can add specific data to header if the current page is concerned (CSS file).

Extension

We can easily add hooks to whatever is pertinent (link add, picwal rendering, etc.).

TODO

  • Strong documentation, especially for plugin developers.
  • Unit tests for PluginManager and Router.
  • add a hook on link deletion.
  • make a POC for Isso.
  • Test this heavily.

Later:

  • finish Markdown plugin.
  • Add a plugin page in administration.

@nodiscc
Copy link
Member

nodiscc commented Jul 12, 2015

slowclap.gif
This looks really good.

Could you write a plugin for #147 and push it as a separate commit? This would help understanding how to write a simple one.

The commit history probably needs to be squashed/reordered and some commits need to be split, but we can do that at a later point.

@ArthurHoaro
Copy link
Member Author

Thank you! Ok I'll do that. It will be like the playvideos plugin.

ArthurHoaro added a commit that referenced this pull request Jul 13, 2015
@ArthurHoaro
Copy link
Member Author

So, I've rebased to edit the way plugins config are handled according to you message. I've also added loggedin status information to plugins.

Second commit contains a plugin which add a field in linklist.

The commit history probably needs to be squashed/reordered and some commits need to be split, but we can do that at a later point.

Yeah... won't be fun.

ArthurHoaro added a commit that referenced this pull request Jul 13, 2015
@virtualtam
Copy link
Member

Kudos for continuing work on the plugin system :D

Why not consider the current plugin-proposition branch as a dev sandbox, to identify:

  • which Shaarli components need to be altered
  • which features need to be added

in order to provide a functional plugin system, and then implement them one by one in separate PRs?

In Agile projects, it corresponds to the Epic notion: a long-term goal that needs to be split in several smaller, more localized tasks.

@ArthurHoaro
Copy link
Member Author

Sooooo... I have reset all commits, and split the whole thing in 2 commits:

  • core: plugin system
  • templates.

Then I've pushed all plugins into separate PR.

I hope that's OK cause it was a pain in the ass to do.

ArthurHoaro added a commit that referenced this pull request Jul 15, 2015
@ArthurHoaro
Copy link
Member Author

Updated: lot of new hooks and template placeholders.

EDIT: started documentation: https://github.com/shaarli/Shaarli/wiki/Plugin-System

@virtualtam
Copy link
Member

split the whole thing in 2 commits

Thanks!
I'll take time to test, review & comment this PR at the end of the week :)

I hope that's OK cause it was a pain in the ass to do.

And it'll be worth every bit of it!
Splitting huge changes in smaller localized commits really helps:

  • for reviewers: easier to read and understand, detect typos / bugs
  • for the submitter: it will truly avoid enduring a great pain in the aforementioned location when rebasing and making amends to take reviews into account :p

ArthurHoaro added a commit that referenced this pull request Jul 16, 2015
@ArthurHoaro
Copy link
Member Author

Unit tests added and documentation started.

@@ -0,0 +1,28 @@
https://github.com/shaarli/Shaarli/issues/181 - Add Disqus or Isso comments box on a permalink page
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • good starting point for plugin discussions :)
  • a README pointing to Github and/or local documentation, as well as the plugin label, could be useful for users/developers

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually @nodiscc file I just moved. But we can try to make a POC for Isso. It would be a good use case to test the plugin system pertinence.

Add an icon in linklist to display links QRCode
Display a button in buttons toolbar which allows to play all videos found.
Display an archive.org icon in linklist, foreach links.
Add a Wallabag icon in linklist for each link.
Add an icon for each link (linklist) for ReadItYourself
Add a field in linklist page to paste a new link.

Displayed in fields toolbar.
+ removed exit error if the config is not found
+ coding style
This plugin try to cover Shaarli's plugin API entirely.
Can be used by plugin developper to make their own.
ArthurHoaro added a commit that referenced this pull request Nov 8, 2015
@ArthurHoaro ArthurHoaro merged commit fd006c6 into master Nov 8, 2015
@ArthurHoaro ArthurHoaro mentioned this pull request Nov 13, 2015
@kalvn
Copy link

kalvn commented Nov 19, 2015

Hey guys,

Sorry for not having followed the development of the plugin system.
I just updated my theme to implement this great new way of improving Shaarli and I just want to share my feedback, as a theme developer:

  • If we want to facilitate the life of theme developers, we should give strong guidelines to plugin developers in terms of what they should do and not do. For example, I had trouble with the addlink_toolbar plugin: it embeds too much design rules which fits well with the default theme but not at all with mine. Same with the qrcode plugin. I think we should encourage plugin developer to put as little style as possible in their html elements so that theme developers can make sure the display fits the global theme style.
  • In the same idea, pictures embed with plugins need to be as neutral and simple as possible in order to integrates well with different themes. It would be great to offer the possibility to display different styles of picture, or text instead of pictures, but I suppose it's up to the plugin developer to propose that.

That's all I have in mind for now. But in any case, the plugin system works great and offers a tons of possibilities ! I just hope plugin developer will be aware of the fact that there are many different themes out there (could be useful to remind it in the documentation for plugins, by the way).

Thanks for the awesome work, guys !

@ArthurHoaro
Copy link
Member Author

Thanks for your feedback @kalvn!

I'll see what I can do to reduce coupling between plugin's code and design rules in the next versions.

@ArthurHoaro ArthurHoaro deleted the plugin-proposition branch January 12, 2016 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants