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

"get_config" abstraction #57

Closed Raruto closed 4 years ago

Raruto commented 4 years ago

In writing this piece of code, I stumbled upon this point:

$dependency = $wpdi->get_config( $slug );
$sources    = $dependency['sources']; //  <-- I don't like it very much...

foreach ( $dependency['sources'] as $source ) {
  // do things here.
}

It would make sense to modify the get_config function to allow us to easily make the following calls?

$wpdi = WP_Dependency_Installer::instance();

$wpdi ->get_config("github-updater/github-updater.php", "sources" );
$wpdi ->get_config("github-updater/github-updater.php", "name" );
$wpdi ->get_config("github-updater/github-updater.php", "token" );

// etc.

Adds optional $key parameter:

/**
 * Get the configuration.
 *
 * @since 1.4.11
 *
 * @param string $slug Plugin slug.
 * @param string $key Dependency key.
 *
 * @return mixed|array The configuration.
 */
public function get_config( $slug = '', $key = '' ) {
  if ( isset( $this->config[ $slug ] ) ) {
    return empty( $key ) ? $this->config[ $slug ] : $this->config[ $slug ][ $key ];
  } else {
    return $this->config;
  }
}
afragen commented 4 years ago

Is your code example in your own code? I don't see it here.

Is this to be used somewhere in the framework or outside of it?

Raruto commented 4 years ago

Is this to be used somewhere in the framework or outside of it?

That function is already public, but I think it could be fine for both purposes

// Current code.
$this->config[ $slug ]["sources"];

// Proposal code.
$this->get_config( $slug, "sources" );

Is your code example in your own code? I don't see it here.

https://github.com/afragen/wp-dependency-installer/pull/55#issue-378023713

Here's how it would become my code:

/**
 * Get plugin notices.
 *
 * @param string $slug Plugin slug.
 *
 * @return array Admin notices.
 */
function wpdi_notices( $notices, $slug ) {
    $wpdi = WP_Dependency_Installer::instance();

    if ( ! $wpdi->is_active( $slug ) ) {
        return $notices;
    }

    // $dependency = $wpdi->get_config( $slug );

    // foreach ( $dependency['sources'] as $source ) {

    foreach ( $wpdi->get_config( $slug, 'sources' ) as $source ) {

        // Check if we were trying to deactivate a manadatory plugin.
        if ( isset( $_REQUEST['wpdi_required'] ) && $slug === $_REQUEST['wpdi_required'] ) {
            array_unshift(
                $notices, [
                    'status'  => 'error',
                    /* translators: %s: Plugin name */
                    'message' => sprintf( esc_html__( 'The %s plugin is a mandatory plugin.' ), $dependency['name'] ),
                    'source'  => $source,
                ]
            );
        }
    }

    return $notices;
}
afragen commented 4 years ago

OK, now I understand. Looks simpler.

Raruto commented 4 years ago

OK, now I understand. Looks simpler.

Ok, I do in the coming days.