Feature/config only

[sebastianfeldmann]

This is a draft for the issue #1

This way a Mediator implementation could look like this.

<?php declare(strict_types=1);

namespace PharIo\Mediator;

class Plugin extends PluginBase
{
    /**
     * Path to your mediator.json config file         
     */
    protected $configPath  = '';

    /**
     * If you are using a config file you have to remove the following lines otherwise settings
     * defined here will overwrite settings defined in the configuration file.
     */       
    protected $config = [
        // replace this with the your phar package name
        'packageName' => 'my-namespace/my-phar-package',
        // replace this with the path to your key directory
        'keyDirectory' => __DIR__ . '/../keys/',
        // replace this with the name of the binary that you want to use within the folder vendor/bin/
        'pharName' => 'my-binary',
        // replace this with the path to the phar file.
        // replacements are described in the [README.md](https://)
        'pharUrl' => 'https://github.com/heiglandreas/JUnitDiff/releases/download/%version%/my-binary.phar',
        // replace this with the path to the signature-file for the phar
        // replacements are described in the [README.md](https://)
        'pharSignatureUrl' => 'https://github.com/heiglandreas/JUnitDiff/releases/download/%version%/junitdiff.phar.asc'
    ];
}

[sebastianfeldmann]

Or if you have a mediator.json configuration file it would look something like this.

<?php declare(strict_types=1);

namespace MyPharTool\ComposerPlugin;

class Plugin extends PluginBase
{
    protected $configPath  = __DIR__ . '/../mediator.json';
}

mediator.json

{
    "packageName": "MyNameSpace/MyPharTool",
    "keyDirectory": "keys",
    "pharName": "my-phar",
    "pharUrl": "https://github.com/...",
    "pharSignatureUrl": "https://github.com/..."
}

[sebastianfeldmann]

I think I would actually go that far that we force a mediator.json configuration and skip the code configuration stuff completely.

I think you meat something like

class ConfiguredMediator extends PluginBase
{
    public abstract function getMediatorConfig(): string;

and then

class MyPlugin extends ConfiguredMediator 
{
    public function getMediatorConfig(): string
    {
        return __DIR__ . '/../mediator.json';
    }
}

[heiglandreas]

I was more thinking of completely removing all code from the plugin if we go config only. The Plugin.php from the base project (composer-distributor) then retrieves the plugin name and folder from composer and checks that plugins folder for the mediator.json. Hopefully no need for a Plugin.php in the actual plugin folder...

[theseer]

I was more thinking of completely removing all code from the plugin if we go config only. The Plugin.php then retrieves the plugin name and folder from composer and checks that plugins folder for the mediator.json. Hopefully no need for a Plugin.php...

Wouldn't that be in the mediator? There has to be code somewhere and I believed that would be the distributor?

[sebastianfeldmann]

I was more thinking of completely removing all code from the plugin if we go config only. The Plugin.php from the base project (composer-distributor) then retrieves the plugin name and folder from composer and checks that plugins folder for the mediator.json. Hopefully no need for a Plugin.php in the actual plugin folder...

That would mean "the user" would have to register the vendor/ComposerDistributor class as a plugin. So far I haven't found a way to retrieve the path of the installed package to look for the mediator.json without any code on the plugin side, @heiglandreas do you know of something?

[sebastianfeldmann]

I have not tried to further eliminate the whole plugin code I will investigate that further but for now this update has incorporated most of your feedback so far :)

• config value objects
• leave abstract class untouched
• remove hacky code configuration completely
• add type hints ;)

TODO: clean json config file handling

Thats how a plugin would look like.

<?php

declare(strict_types=1);

namespace MyNamespace\MyTool;

use PharIo\ComposerDistributor\ConfiguredMediator;

class MyPlugin extends ConfiguredMediator 
{
    public function getMediatorConfig(): string
    {
        return __DIR__ . '/../mediator.json';
    }
}

mediator.json

{
  "packageName": "foo/bar",
  "keyDirectory": "keys",
  "phars": [
    {
      "name": "foo",
      "file": "...",
      "signature": "..."
    }
  ]
}

[theseer]

Looks good :) - except: Is there a reason for the method getMediatorConfig() to be public? Protected should be good enough, shouldn't it? :)

[sebastianfeldmann]

I added the missing json config validation and changed the json error handling because PHP7.2 :)

[theseer]

Looks pretty awesome so far! :+1:

Two questions popped up into my mind, will add them as review comments...

[theseer]

Question two has to go here anyhow ;)

We now have very good looking parsing code to deal with the json file. I'm not convinced though it belongs to the class it currently is in but should be extracted into its own class.

Thoughts?

[sebastianfeldmann]

Question two has to go here anyhow ;)

We now have very good looking parsing code to deal with the json file. I'm not convinced though it belongs to the class it currently is in but should be extracted into its own class.

Thoughts?

Thanks the question itself helps a lot, I thought so myself but wan't sure either. I will extract the json load and validation stuff into a Config\Reader

[heiglandreas]

I was more thinking of completely removing all code from the plugin if we go config only. The Plugin.php from the base project (composer-distributor) then retrieves the plugin name and folder from composer and checks that plugins folder for the mediator.json. Hopefully no need for a Plugin.php in the actual plugin folder...

That would mean "the user" would have to register the vendor/ComposerDistributor class as a plugin. So far I haven't found a way to retrieve the path of the installed package to look for the mediator.json without any code on the plugin side, @heiglandreas do you know of something?

Nope. But to be honest: I haven't looked so far ;-)

But so far the current setup is pretty "low-code" already. So from my side I'm happy with that as a starting point!

[sebastianfeldmann]

I moved the json parsing stuff to Config\Factory. Another thing I'm unsure about is if we should use json_decode to stdClass or to array ?

I think array fits better because passing a stdClass to Factory::createConfig feels weird. array would have the charm of in place configuration.

$factory = new Config\Factory();
$config  = $factory->createConfig(["packageName": "phar-io/phive",......]);

What do you think?

[heiglandreas]

I moved the json parsing stuff to Config\Factory. Another thing I'm unsure about is if we should use json_decode to stdClass or to array ?

I think array fits better because passing a stdClass to Factory::createConfig feels weird. array would have the charm of in place configuration.

$factory = new Config\Factory();
$config  = $factory->createConfig(["packageName": "phar-io/phive",......]);

What do you think?

I stumbled over that as well and it felt weird. TypeHinting stdClass or array is both kind of not saying anything, so from that point it is irrelevant. But the "in place config" would verify using array instead of stdClass.

[theseer]

Given that keys in json can contain chars not allowed of PHP classes or properties, the Array format is what I usually prefer. If you have bizzare key names, the PHP way to access them anyhow is $json->{"foo something"} which is really ugly ;)

Let's just settle with array.

[sebastianfeldmann]

Switched json_decode to ASSOC and setup is handled with arrays now

[sebastianfeldmann]

I updated comments, file headers and type hints to a format I found in phive .

[theseer]

Side note observation: While it's a lot of fun doing this, how can we possibly spent so much time tweaking such a small amount of code? ;-)

[sebastianfeldmann]

Side note observation: While it's a lot of fun doing this, how can we possibly spent so much time tweaking such a small amount of code? ;-)

Cause we want to do it right? :) But I think it is fine now (hopefully)

[heiglandreas]

If we go mediator.json only we might be able to modify the installer instantiation so that it can use the configuration objects instead of strings. something like

public function installOrUpdateFunction(PackageEvent $event): void
{
    $config    = Config\Loader::loadFile($this->getMediatorConfig());
    $installer = self::fromConfig(Config $config);
    $installer->install();
}

[sebastianfeldmann]

• Config return value changed from string to SplFileInfo
• Added ConfiguredMediator::createInstallerFromConfig
• File changed so signature is now optional.
• Installer changed so KeyDirectory is now optional