kirby

kirby copied to clipboard

This PR …

Adds the option to specify the plugin version in your plugin definition outside of the composer.json. You can still just use the composer.json. This PR just adds an additional way to specifiy the plugin version.

<?php

use Kirby\Cms\App;

@include_once __DIR__ . '/helpers.php';

App::plugin('lukaskleinschmidt/snippet-controller', [
    'version' => '2.1.0', // <- This one here
    'options' => [
        'name' => fn (string $name) => $name . '.controller',
    ],
    'components' => [
        'snippet' => function (App $kirby, $name, array $data = [], bool $slots = false) {
            $data = snippet_controller($name, $data);

            return $kirby->nativeComponent('snippet')(
                $kirby,
                $name,
                $data,
                $slots,
            );
        }
    ],
]);

I usually create my composer plugins without adding the version key.

The version detection from Kirby works as long as my plugins are installed via composer. But adding it as a git submodule or simply copy pasting it does not work.

The reason why I avoid using the version key in my composer packages: https://getcomposer.org/doc/04-schema.md#version

TLDR: Version of the docs:

Note Specifying the version yourself will most likely end up creating problems at some point due to human error.

Not a must have and I also understand arguments that prefer using solely the composer.json for such things but I think the solution might fit the bill to make both worlds happy.

Fixes

https://github.com/lukaskleinschmidt/kirby-snippet-controller/issues/2

Breaking changes

Ready?

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

For review team

• [ ] Add changes to release notes draft in Notion
• [ ] Add to website docs release checklist (if needed)

Two thoughts maybe:

• I'd let the version in index.php take priority over the one in composer.json?
• why do you think human error is less a thing for you with index.php compared to composer.json?

Mainly messing up the version in the index.php does not mess up anything in composer. A version mismatch from a release tag and the version specified in the composer.json often is a mess (at least for me).

So changing the version later on in the index.php fixes the issue for the "manual" installs (Where it would acutally matter. That is also the reason for the priority of composer vs index) but keeps integrity with the release tags composer is aware of.

I think there was also the idea in discord somehwere to maybe have a Kirby specific plugin file just for holding any metadata. Maybe a description text if you install plugins via the kirby CLI one day or whatnot.

Not sure if something likes this would be necesseray at all. Just wanted to throw this in as well. https://discord.com/channels/525634039965679616/525638772591951882/1155793991611191326

I can try to add some tests for this. Something around these lines?

1. Test if the fallback is available when no composer.version
2. Test if the fallback is overwritten by composer.version

What's about instead of putting it in the extension array, it's rather a new argument in the plugin call?

Not really convinced about that TBH. We need to keep it optional, so with positional arguments, the only place to put it would be at the end where it looks weird after the $extends array. I quite like putting it into $extends as it's optional and looks clean.

I thought about that as well. And agree with @lukasbestle that having it directly in the $extends looks and feels much cleaner. But not limiting the config to just the version as a third parameter could open up the option for additional configs down the road. But can't tell if something like this is worth it.

App::plugin('lukaskleinschmidt/snippet-controller', [
    'options' => [
        'name' => fn (string $name) => $name . '.controller',
    ],
    'components' => [
        'snippet' => function (App $kirby, $name, array $data = [], bool $slots = false) {
            $data = snippet_controller($name, $data);

            return $kirby->nativeComponent('snippet')(
                $kirby,
                $name,
                $data,
                $slots,
            );
        }
    ],
], [
    'version'     => '2.1.0',
    'description' => 'A plugin to use controllers with snippets.',
]);

I think there was also the idea in discord somehwere to maybe have a Kirby specific plugin file just for holding any metadata. Maybe a description text if you install plugins via the kirby CLI one day or whatnot.

At the moment, all metadata we need has a direct match to keys of composer.json. If we ever need more, we could use the extra key.

I can try to add some tests for this.

Thanks 💛

We already have separate tests for version:

https://github.com/getkirby/kirby/blob/8bb22df36cd6ef9728153a9081c63b875fce1ffc/tests/Cms/Plugins/PluginTest.php#L645

But not limiting the config to just the version as a third parameter could open up the option for additional configs down the road. But can't tell if something like this is worth it.

Hm. I'm always a fan of planning ahead, but TBH it looks a bit too over-engineered to me because it is so far beyond what we currently need.

Then something like above would just mimic the composer file. So probably not worth it. And the version key is a pretty special edge case in the end.

Edit: If the need arises one day you could still easily migrate to a third parameter option down the road and could easily have fallbacks for "old" plugin definitions without breaking anything.

I was thinking less of one third parameter with additional metadata but making use of named arguments. That way we would still add it as third parameter for backwards compatibility but in all the docs etc could make it look like this:

Kirby::plugin(
  name: 'my/plugin',
  version: '1.0.0'
  extends: [...],
  license: 'paid'
);

I like that a lot because it moves us into safer, more future-proof code and we can still keep it backwards compatible

Should I give this a try or is this something you would like to implement separately?

I think it makes sense to already implement it like that. Of course only the version argument so far.

My take on "version as new property": https://github.com/getkirby/kirby/compare/v4/develop...v4/enhancement/plugin-version

Still not fully sure that composer should overrule the version number that explicitly gets passed as argument to the plugin.

Still not fully sure that composer should overrule the version number that explicitly gets passed as argument to the plugin.

Does it? I think the current order is composer.json, index.php, installed Composer version.

Yes but that's what I mean: shouldn't the version that we explicitly pass to Kirby's method to register a plugin be the truth above all?

There are three ways now the version gets resolved.

Right now:

1. Check the version in composer.json of the plugin
2. Check the version from the Plugin defintion
3. Check the version in the installed.php from composer (when installed via composer)

In my opinion if a version is added to the composer.json this would indicate that I want to handle versions using the composer.json. If I dont I have the option to set the version using the new Plugin definition. I don't think someone should do both.

In the end the version shown should reflect the one that composer intended to install I guess. So the order should actually be:

1. Check the version in composer.json of the plugin
2. Check the version in the installed.php from composer (when installed via composer)
3. Check the version from the Plugin defintion

public function version(): string|null
{
	$composerName = $this->info()['name'] ?? null;
	$version      = $this->info()['version'] ?? null;

	try {
		// if plugin doesn't have version key in composer.json file
		// try to get version from "vendor/composer/installed.php"
		$version ??= InstalledVersions::getPrettyVersion($composerName);
	} catch (Throwable) {
		return null;
	}

	$version ??= $this->version;

	// …
}

Or even check the installed.php first if you messed up the version in your composer.yml? 🙈

I honestly disagree. If I tell Kirby the version of the plugin by passing it explicitly to the register method, that one should have the authority to- not some other version Kirby can find elsewhere. Those are great as fallbacks if I don't tell Kirby the version. But if I explicitly do, it's weird to ignore it and also hard to understand and debug for developers.

I think this priority idea will cause confusion. I have another idea. IMHO metadata data should be retrieved from either composer.json or index.php. If a plugin does not have composer.json, it should look at the third parameter in index.php for metadata data (App::plugin('plugin', [], ['version' => 1.0]). Otherwise it should get it from composer.json. So everything is clear and simple.

But why is an arbitrary file like composer.json seen as more important than what the developer explicitly registers the plugin as. We don't do that for other things like the name, do we?

I think composer.json is not an arbitrary file, it's a standard. Almost all plugins are in git. I don't think it makes much sense to look at both file (composer.json and index.php). I'm completely open to the idea of including metadata data only from the plugin registration method. Then we can do it like this;

K4: New registry

Plugin::register(
  name: 'my/plugin',
  version: '1.0.0'
  extends: [...],
  license: 'paid'
);

Even with class (that's would awesome)

class MyPlugin implements Plugin
{
    public function name(): string
    {
        return 'My plugin';
    }
    
    public function version(): string|float
    {
	return 1.0;
    }
    
    public function description(): string
    {
	return 'My description';
    }
    
    public function extends(): array
    {
	return [];
    }
}

App::plugin() still works.

K5: deprecate App::plugin()

K6: remove App::plugin()

So In K6, reading metadata from composer.json is completely removed.

As a note from the outside: please keep this simple and don't apply a cascade of rules to extract the version number. Noone will understand that. One place of truth would be very helpful, including this in the Kirby plugin registry sounds like a very good idea especially as Composer discourages the usage of the version tag in their file specifically.

Plugin developers will always make mistakes and forget to update their version numbers in some places. But having only one place to look into will make things easier.

I still think that VSC tagged version should have precendence over a potential wrong (because forgot to update) version in any file.

If the version in the index.php would have precendece and you forgot to update it accordingly then it would be necessary to update your plugin and create a new tagged version so composer installs will show the correct version (which already exists anyway in the installed.php)

In a perfect world the version specified in index.php or composer.json should reflect the tagged version used by packagist. But.

Specifying the version yourself will most likely end up creating problems at some point due to human error.

So having that kind of order would allow for composer installs to still show the correct (tagged) version. And custom installs via git or copy paste can simply download the latest version from git (when I almost certainly will forget to update that version in the index.php file again).

I agree with Nils. This discussion clearly shows that we are adding quite a bit of complexity here.

I like Ahmet's idea to migrate to a new system entirely. Maybe like:

• Dynamic lookup via installed.php always takes priority (if it is available, it is always correct)
• If not available (= not installed via Composer), use the one in index.php. We need to have a way to manually define the version for ZIP downloads and this one is the simplest that doesn't conflict with Composer's recommendations.
• Deprecate reading from composer.json entirely. Would still work until v5, but plugin devs should migrate to index.php.

Am I correct that Composer reads the current version number from Git tags? So there would be two places that matter: the plugin version definition and the Git tags. Maybe you could add a Kirby CLI command to update both at the same time? Might reduce manual oversights on the developer side.

Could also be a validator: "Your version number and Git tags do not match" …

I think so too.

About adding a command to the CLI: The CLI would actually have a much easier time to update composer.json compared to index.php.

Not being able to keep the version in the composer.json in sync manully (sometimes forget to update) and not wanting any automation or custom deploy script is one of the reasons this PR exists in the first place 😉

Otherwise sticking with the composer.json would have been the way to go.

Bringing this back to life:

I think the way that @lukasbestle proposed https://github.com/getkirby/kirby/pull/5764#issuecomment-1757331617 makes a lot of sense.

Yes, there could still be two sources of truth (git tag that ends up in installed and the plugin's index.php) but I don't think we will get around this in a sensible way: There is on the one side a strong preference of some to not store a version info at all in any file and just rely on the git tag which composer will use. That works if you use composer, but completely fails for manual downloads. However, we need a version number for the update check etc. - so no version number when not installed via composer is not a valid option. On the other side, index.php can't be the only source of truth either as git tags will still be required and we won't be able to force composer to read our index.php

So even if not ideal, I think it's the most sensible way forward. If we don't want it inside composer.json. Then again, we are not winning much more than shifting the "problem" from composer.json to index.php.

An alternative could be to have a simple text file with the version number (like .version) at the root of the plugin directory. This file could then be easily updated by automated scripts and the CLI.

The order in my comment above would still apply: First the Composer-installed version if available, otherwise the static version file. The version in composer.json would still be deprecated.

I don't have any preference whether in index.php or a .version file. I think we should just wrap this up finally. What's the most reliable way to read the version from installed.php?