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

Add a autoloader for plugins #6714

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

plain-solutions-gmbh
Copy link
Contributor

@plain-solutions-gmbh plain-solutions-gmbh commented Oct 1, 2024

Description

Managing the plugin definition can be very time-consuming.

If you want to create (for example) a snippet, you also have to declare it in the index.php file.

Kirby::plugin('your/plugin', [
    'snippets' => [
        'header' => __DIR__ . '/snippets/header.php'
    ]
]);

This PR allows you, to leave the definition blank. It gets everything from the folders:

Kirby::plugin('your/plugin')

Summary of changes

Adding 3 new methods to Kirby\Cms\Plugin:

loadTranslations: Generate definition of translation folder (I18n). Encoding json or/and adding prefix to keys
loadFiles: Generates the definition of a specific folder.
autoload: Generates the definition of a specific folder (or folder array).

Reasoning

Many developers have already developed their own workaraound for this. It is time to implement something standardised.

Additional context

NOTICE!
Kirby::plugin('your/plugin'); gets the registered plugin and null if not registered before.
Now it's sets the extends. Please check for possible incompatibility.

Changelog

Fixes

Breaking changes

Docs

This example loads following items (based of the existing folders from the current plugin folder):

  • File implementation for snippets, templates, blueprint
  • Including the config folder and import the files (To outsource the plugin definition)
  • Load the translations from the I18n folder. Converts JSON files (if needed) and adding prefix to the keys based by the plugin name
Kirby::plugin("your/plugin", Plugin::autoload());

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@afbora
Copy link
Member

afbora commented Oct 1, 2024

First of all, thank you for your contribution 💛

Don't get me wrong please. IMHO I think this makes plugin registration more complicated than it makes it easier. For this, I would recommend Bruno’s plugin for these cases:

https://plugins.getkirby.com/bnomei/autoloader

@plain-solutions-gmbh
Copy link
Contributor Author

Don't get me wrong. What's easier than leaving the definition empty?
For me it is more complicated to install an additional plugin and distribute it to everyone who wants to use my plugin.
Every additional plugin installed makes the system more unstable.
Or what do the core team thing?

@afbora
Copy link
Member

afbora commented Oct 1, 2024

Honestly I have no problem with the idea, I can even say that I like it. I even have a plugin about autoloading. But I think implementing it in the Kirby core adds complexity to the current structure.

If I were to implement it anyway, I would do it like following instead of registering with Plugin::autoload():

// default
Kirby::plugin("your/plugin", autoload: true);

// with custom arguments
Kirby::plugin("your/plugin", autoload: []);

Maybe another idea to autoload would be like passing true:

Kirby::plugin('your/plugin', [
    'snippets'   => true,
    'blueprints' => true
]);

Also I would move the autoload helper methods to separate class something like that: PluginAutoloader. This makes increase the code readability.

@plain-solutions-gmbh
Copy link
Contributor Author

This seems to be a good solution. It does not interfere with normal behaviour and is there for those who want to use it.

@distantnative
Copy link
Member

distantnative commented Oct 2, 2024

Thanks for this suggestion, @plain-solutions-gmbh.

I don't think this should be the default behavior to be honest. I think this would create significant performance implications.

I do agree that offering an option in the core might be good, but I think the implementation still is crucial. To me, something in between makes most sense: on the one hand to land more automatically, reduce unnecessary boilerplate code; but still be deliberate and performant.

I was thinking whether instead an array of snippets, blueprints etc. we could allow passing a path to the folder. And when that is passed, Kirby would automatically load all files from that folder as snippets, blueprints.

Kirby::plugin('your/plugin', [
    'snippets'   => __DIR__ . '/snippets',
    'blueprints' => __DIR__ . '/blueprints'
]);

With that, we would keep a certain amount of control and deliberateness, but not overly tax performance but also stay very close to the current syntax and not create another option or whole new class/method (or one could still offer a PluginAutoload class as alternative option).

@plain-solutions-gmbh
Copy link
Contributor Author

Another thought I've had is if we had something like kirbyup for PHP too? 🤔

Or you could implement the autoload feature, but keep it out of the docs. 🤫 Basically, the point is that this is not used in every little plugin.

I understand what you mean. I'm also always very careful about doing resource intensive things in plugins. Especially when I see that they are loading on every request.

Why not saving the folder structure in the memcache. As soon as the last modify value changes, the cache will be reloaded. All the file management runs through the F:: & Dir::methods. Just an idea that I'm sure you've also considered. This topic has certainly been discussed several times at Kirby CMS. But I don't want to revisit these chapters here. 🤐

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

Successfully merging this pull request may close these issues.

3 participants