afragen / wp-dependency-installer

A lightweight class to add to WordPress plugins/themes to automatically install plugin dependencies.
MIT License
205 stars 32 forks source link

Register config #44

Closed Raruto closed 4 years ago

Raruto commented 4 years ago

Related to https://github.com/afragen/wp-dependency-installer/issues/43, adds an optional second parameter within the run() function to allow us to abstract from its contents.

Example of usage:

/**
 * Group Plugin Installer
 *
 * @author  Andy Fragen
 * @license MIT
 * @link    https://github.com/afragen/group-plugin-installer
 * @package group-plugin-installer
 */

$config = json_decode(<<<JSON
[
  {
    "name": "GitHub Updater",
    "host": "github",
    "slug": "github-updater/github-updater.php",
    "uri": "afragen/github-updater",
    "branch": "master",
    "optional": false,
    "token": null
  }
]
JSON
, true ); // <-- NB the comma here is only needed for a faster HEREDOC assignment

include_once( __DIR__ . '/vendor/autoload.php' );

// Install and active dependencies.
// OLD:
// WP_Dependency_Installer::instance()->source = basename( __DIR__ ); <-- 😠 bad code
// WP_Dependency_Installer::instance()->load_hooks();
// WP_Dependency_Installer::instance()->register( $config );

// NEW:
WP_Dependency_Installer::instance()->run( __DIR__, $config );
Raruto commented 4 years ago

BTW, maybe the following would be a better solution?

WP_Dependency_Installer::instance()->register( $config );
WP_Dependency_Installer::instance()->run( __DIR__ );
afragen commented 4 years ago

I don't like passing the JSON into the variable only to decode it. Using this framework implies some amount of capability. Creating an associative array doesn't seem like a huge thing. I think the solution is for better documentation.

I also think moving the load_hook() from run() to register() makes more sense. It would also make the extra call to both, as above, unnecessary.

So if using a JSON file then run( __DIR__ ) and if using an array register( array )

Raruto commented 4 years ago

Creating an associative array doesn't seem like a huge thing. I think the solution is for better documentation.

It's just for simplicity, JSON configs are just more practical...

For example, in my other plugin, I had created a bulk action for an easy export / generation of this json file (directly from a live wordpress instance):

68747470733a2f2f72617275746f2e6769746875622e696f2f696d672f77702d6465766f70732d62756c6b2d616374696f6e732e706e67

and they can also be more error-proof in the case of a manual conversion / generation ...

afragen commented 4 years ago

I think the purpose being able to load a config programmatically is really just for those developers who don't want to create a JSON config file for whatever reason.

In that regard it's probably better to use an optional parameter and continue using the same function as you have in this PR.

afragen commented 4 years ago

So something like.

include_once( __DIR__ . '/vendor/autoload.php' );

// Current workflow:
// WP_Dependency_Installer::instance()->run( __DIR__ );

$config = [
    [
    'name'    => 'Query Monitor',
    'host'    => 'wordpress',
    'slug'    => 'query-monitor/query-monitor.php',
    'uri'     => 'https://wordpress.org/plugins/query-monitor/',
    'require' => true,
    ],
    [
    'name'     => 'Debug Bar',
    'host'     => 'wordpress',
    'slug'     => 'debug-bar/debug-bar.php',
    'uri'      => 'https://wordpress.org/plugins/debug-bar/',
    'optional' => true,
    ],
];

// Alternative workflow?
WP_Dependency_Installer::instance()->run( __DIR__, $config );
afragen commented 4 years ago

Though I guess it really doesn't matter as the user could create the $config variable in either manner.

Raruto commented 4 years ago

Maybe I found a better solution, could you reopen this pull?


// Next major release.
WP_Dependency_Installer::instance( __DIR__ )->register( $config );
WP_Dependency_Installer::instance( __DIR__ )->run();

// Backwards compatibility releases.
WP_Dependency_Installer::instance()->register( $config, __DIR__ );
WP_Dependency_Installer::instance()->run( __DIR__ );
afragen commented 4 years ago

I reverted the merge but not sure that re-opens the PR