Plugin version outside of the composer.json

[lukaskleinschmidt]

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

lukaskleinschmidt/kirby-snippet-controller#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)

[distantnative]

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?

[lukaskleinschmidt]

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.

[lukasbestle]

Your thoughts and the resulting order make sense to me. I had thought about index.php as the final fallback if the dynamic Composer lookup also failed, but I came to the conclusion that using the explicitly defined versions first is easier to understand and debug. So I think your code is the right way to go. I had also thought about whether another option is worth it, but maybe we could actually recommend this in the future instead of adding the version to composer.json (which can indeed cause problems if there is a mismatch).

Only needs tests IMO.

[lukaskleinschmidt]

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

[distantnative]

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

[lukasbestle]

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.

[lukaskleinschmidt]

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.',
]);

[lukasbestle]

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:

kirby/tests/Cms/Plugins/PluginTest.php

Line 645 in 8bb22df

public function testVersion()

[lukasbestle]

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.

[lukaskleinschmidt]

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.

[distantnative]

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'
);

[bastianallgeier]

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

[lukaskleinschmidt]

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

[lukasbestle]

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

[distantnative]

My take on "version as new property": 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.

[lukasbestle]

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.

[distantnative]

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?

[lukaskleinschmidt]

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? 🙈

[distantnative]

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.

[afbora]

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.

[distantnative]

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?

[afbora]

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.

[nilshoerrmann]

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.

[lukaskleinschmidt]

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).

[lukasbestle]

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.

[nilshoerrmann]

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" …

[lukasbestle]

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.

[lukaskleinschmidt]

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.

[distantnative]

Bringing this back to life:

I think the way that @lukasbestle proposed #5764 (comment) 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.

[lukasbestle]

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.

[distantnative]

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?

[lukasbestle]

I think we should just wrap this up finally. What's the most reliable way to read the version from installed.php?

We already do:

kirby/src/Cms/Plugin.php

Line 304 in 2965c31

$version ??= InstalledVersions::getPrettyVersion($composerName);

The only thing that would change is the order and the source of secondary information.

I think we should just wrap this up finally.

Yes, but we shouldn't rush it either as we are introducing a new concept here that needs to work for everyone.

[lukaskleinschmidt]

I think the current PR might still tick all the boxes for fixing the issue for now.

We need to have a way to manually define the version for ZIP downloads … #5764 (comment)

I get that there are plans to overhaul the plugin system, but I think that could be a separate issue from the current one.

The PR is backwards compatible and non breaking if not used. The current plugin register flow will stay the same. And the change would not hinder any future changes to the plugin system down the road (at least in my opinion).

[lukasbestle]

The issue is that plugin devs will use the new feature, which will prevent us from removing it again later without a breaking change. And having multiple competing ways is not great either. So I think it's important to find a good solution.

[lukaskleinschmidt]

My point being that if you overhaul the whole plugin system at some point it will probably be a breaking change no matter what. So in my opinion no harm in removing this version feature again (from the extends) and put it somewhere else.

Probably depends more on the when then. I get that it does not make sense to introduce this feature and remove it again just a few months later.

[lukaskleinschmidt]

One additional thing that occurred to me today was that this "intermediate" way of adding the version could also easily be backported to Kirby v3.

[lukasbestle]

I'm not aware of plans to overhaul the plugin system, especially in this area.

[lukasbestle]

Trying to summarize a possible way forward:

• Dynamic lookup via Composer's InstalledVersions::getPrettyVersion() always takes priority (if it is available, it is always correct)
• If not available (= plugin was not installed via Composer), read another place we need to decide on. This could be:
  1. A new version key inside index.php as proposed by this PR. Advantage: Fits in well. Disadvantage: Cannot easily be updated by the CLI or other tools.
  2. A .version file in the root of the plugin folder that would just contain the installed version number, nothing else. Advantage: Being a separate file, it allows us to easily build commands into the CLI to update the plugin version etc. Disadvantage: The file name is opinionated. Is there maybe a standard for such version files we could piggyback onto?
  3. A custom key in composer.json like extra.kirby-plugin-version. Advantage: Moves the version out from the main version key that Composer doesn't like but is still "in one place" and easy to update with the CLI. Disadvantage: Looks weird and feels clunky.
• If nothing matches, the version from composer.json would be the fallback for now. A possible deprecation could work like this:
  • Deprecate the version in composer.json in 4.x.
  • In 5.0, throw a deprecation warning if a plugin does not have the new way we decide on.
  • In 6.0, no longer try to read the version from composer.json.

My current favorite is option ii (separate file in the plugin root), although I'm not sure about the name.

[bastianallgeier]

@lukasbestle do you think the regex for the version bump would be so problematic if the version is stored in the index.php? I'm not a huge fan of adding more config files. It's not like we don't have enough *.config,json .ignoreblabla etc. in modern web dev projects already :)

[lukasbestle]

You are right, simplicity-wise it would indeed be best to have the version in index.php. Matching and replacing it with a regex could work in many cases, but it's not something I would fully trust.

[bastianallgeier]

@lukasbestle I think we should no longer keep this PR unmerged though if the key is the easiest and most usable option for plugin devs who don't use composer. I'm sure we can find a solution for cli commands later to do this reliably.

[lukasbestle]

If it is, then I fully agree.

Maybe we should post this suggestion on Discord and get feedback from other plugin devs. Could avoid having to backtrack and find yet another solution after releasing this.

[bastianallgeier]

Hey @lukaskleinschmidt,

I just really wanted to thank you for your contribution and more importantly for your patience with us :) I think the discussion on Discord really led to a good result and we are taking over from here: #6337