Skip to content

Commit

Permalink
Add a group of abstracted methods for often used functionality:
Browse files Browse the repository at this point in the history
- `get_tgmpa_status_url()` -> url to the tgmpa page for a certain view
- `tgmpa_complete()` -> do we have actions on the registered plugins or not ?
- `is_plugin_installed()`
- `is_plugin_active()` -> similar to WP native `is_plugin_active()`
- `can_plugin_update()` -> check whether the minimum WP requirement for a plugin update has been met
- `can_plugin_activate()` -> check whether a plugin complies with an optionally set minimum version of the plugin and is currently inactive
- `get_installed_version()` -> get the version nr of an installed plugin
- `does_plugin_require_update()` -> check whether a plugin complies with an optionally set minimum version of the plugin
- `does_plugin_have_update()` -> check whether there are updates available for a plugin, required or otherwise
- `get_upgrade_notice()` -> get the upgrade notice from the plugin readme if supplied by the plugin author
- `get_plugins()` -> wrapper for WP native `get_plugins()`
  • Loading branch information
jrfnl committed May 3, 2015
1 parent a9a136d commit 495034f
Showing 1 changed file with 202 additions and 0 deletions.
202 changes: 202 additions & 0 deletions class-tgm-plugin-activation.php
Original file line number Diff line number Diff line change
Expand Up @@ -1347,6 +1347,208 @@ public function get_tgmpa_url() {
return $url;
}

/**

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

"URL" please.

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

Changed in all comments where it was lowercase.

* Retrieve the url to the TGMPA Install page for a specific plugin status.
*
* @since 2.5.0
*

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

May not have found it yet during the review, but are the possible enumerations here captured in an array or returned in a method somewhere? If so, pointing to that may be better than listing them here, in case the list gets expanded.

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

Not yet, having some arrays with standard values would be useful in more places than just here though. Something for a future refactor.

* @param string $status Plugin status - either 'install', 'update' or 'activate'.
* @return string Properly encoded url (not escaped).
*/

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

Why is tgmpa mentioned in the function name?

What we're looking at here is the "action" or "intent" URL for a single plugin, so can it be named accordingly? I see that you've used the word "action" in the @return description for the next function below so that validates my thinking.

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

Sorry, but you misunderstand the purpose of the function.
There is a get_tgmpa_url() method (just above this one) which will return the URL to the TGMPA page - i.e. http://mysite.com/wp-admin/plugins.php?page=tgmpa-install-plugins
This function builds onto that to return the URL to a specific view for the TGMPA page, the original URL will go to the 'all' view, the URLs returned by this method will go to either the 'install', 'update' or 'activate' view, i.e. http://mysite.com/wp-admin/plugins.php?page=tgmpa-install-plugins&plugin_status=update

So this URL has nothing to do with single plugins, but rather with the newly introduced List_Table views.

As for the choice of the terminology 'status' in the function name and 'plugin_status' as URL argument - that was based on the WP native implementation of the same on the Core plugins page.

The next function checks something different altogether. I'd originally actually moved some functions to other places in the code flow where they would be more logically placed, but that was making things even more difficult to review/compare, so I moved them back and reserve the right to re-order the methods in a separate PR at some point 😎

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

That's all good then.

Could you add an example in the description of these functions (using example.com as the domain) for the typical URL expected back from both of these functions please? Let's clarify it in the DocBlock so there's no confusion from other zombified developers 😴

public function get_tgmpa_status_url( $status ) {
return add_query_arg(
array(
'plugin_status' => urlencode( $status ),
),
$this->get_tgmpa_url()
);
}

/**

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

Avoid "we".

Will this summary make sense if pulled out into a different context? (WP_Parser, phpDocumentor etc.)

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

Adjusted

* Determine whether we still have anything to do at all.
*
* @since 2.5.0
*
* @return bool True if complete, i.e. no outstanding actions. False otherwise.
*/

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

Since this returns a boolean, is should be named with a leading is_ to make it easier to read please.

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

Done

public function tgmpa_complete() {
$complete = true;
foreach ( $this->plugins as $slug => $plugin ) {
if ( ! $this->is_plugin_active( $slug ) || false !== $this->does_plugin_have_update( $slug ) ) {
$complete = false;
break;
}
}

return $complete;
}

/**

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

Is checking for MU plugins on your radar for 2.5.0, or leaving it for now?

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

Not for 2.5.0, quite apart from the potential detection issues what with that every mu plugin which lives in a directory needs a bootstrap in the mu-plugins root and that both bootstrap as well as the directory as used in mu might not be named as expected. There are no native WP functions to get a list of the MU subdirectories either, just of the files in the mu-plugins directory. A user might have one bootstrap loading all the subdirectory plugins or even not have subdirectories in mu, but point the mu-bootstrap to the normal plugins directory. In other words: a minefield.
Also see: https://core.trac.wordpress.org/ticket/24205

* Check if a plugin is installed. Does not take must-use plugins into account.
*
* @since 2.5.0
*
* @param string $slug Plugin slug.
* @return bool True if installed, false otherwise.
*/
public function is_plugin_installed( $slug ) {
$installed_plugins = $this->get_plugins(); // Retrieve a list of all installed plugins (WP cached)
return ( ! empty( $installed_plugins[ $this->plugins[ $slug ]['file_path'] ] ) );
}

/**
* Check if a plugin is active.
*
* @since 2.5.0
*
* @param string $slug Plugin slug.
* @return bool True if active, false otherwise.
*/
public function is_plugin_active( $slug ) {

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

This is the second time in a few lines you've had to access the file_path property of a particular plugin. Seems to be 23 instances in for this branch right now, which is over half of the instances of ->plugins[ $slug. Abstract to something like $this->get_plugin_file_path( $slug ) ?

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

There is already a function which does that _get_plugin_basename_from_slug() and using that function is a lot more expensive than using array access which is which is why that function is called from register() and the array is accessed everywhere else.

return ( is_plugin_active( $this->plugins[ $slug ]['file_path'] ) || ( ! empty( $plugin['is_callable'] ) && is_callable( $plugin['is_callable'] ) ) );
}

/**
* Check if a plugin can be updated, i.e. if we have information on the minimum WP version required
* available, check whether the current install meets them.
*
* @since 2.5.0
*
* @param string $slug Plugin slug.

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

"OK"

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

actually, the "is" should be "if" dyslectic

* @return bool True is ok to update, false otherwise.
*/
public function can_plugin_update( $slug ) {

// We currently can't get reliable info on non-WP-repo plugins - issue #380

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

There appears to be five instances where 'repo' is === or !== $this->plugin[ $slug ]['type']. Refactor out?

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

See my comment about 'file_path'. Similar situation.
Current code is in this regards already a lot better than before ,as before in each of those places the whole logic of determining what the plugin source type was was duplicated. Now that logic has been abstracted to a separate method which is called once from register()

if ( 'repo' !== $this->plugins[ $slug ]['type'] ) {
return true;
}

$api = $this->get_plugins_api( $slug );

if ( false !== $api && isset( $api->requires ) ) {
return version_compare( $GLOBALS['wp_version'], $api->requires, '>=' );
}

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

No need for this return to be wrapped in an else. If we've got this far, then the only thing left is to return.

if ( ... ) {
    return version_compare( ... );
}

// No usable ...
return true;

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

LOL, I knew you were going to say that ;-)

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

At least we're in agreement then!

else {
// No usable info received from the plugins API, presume we can update.
return true;
}
}

/**
* Check if a plugin can be activated, i.e. is not currently active and meets the minimum
* plugin version requirements set in TGMPA (if any).
*
* @since 2.5.0
*
* @param string $slug Plugin slug.

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

"OK".

* @return bool True is ok to activate, false otherwise.
*/
public function can_plugin_activate( $slug ) {
return ( ! $this->is_plugin_active( $slug ) && ! $this->does_plugin_require_update( $slug ) );
}

/**
* Retrieve the version number of an installed plugin.
*
* @since 2.5.0
*
* @param string $slug Plugin slug.
* @return string Version number as string or an empty string if the plugin is not installed
* or version unknown (plugins which don't comply with the plugin header standard).
*/
public function get_installed_version( $slug ) {

$installed_plugins = $this->get_plugins(); // Retrieve a list of all installed plugins (WP cached)

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

Would it make more sense to negate this logic and switch the returns?

"If this thing isn't empty, return it. Otherwise return an empty string."

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

Fine

if ( empty( $installed_plugins[ $this->plugins[ $slug ]['file_path'] ]['Version'] ) ) {
return '';

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

The final return doesn't need to be wrapped in the else.

} else {
return $installed_plugins[ $this->plugins[ $slug ]['file_path'] ]['Version'];
}
}

/**
* Check whether a plugin complies with the minimum version requirements.
*
* @since 2.5.0
*
* @param string $slug Plugin slug.
* @return bool True when a plugin needs to be updated, otherwise false.
*/
public function does_plugin_require_update( $slug ) {
$installed_version = $this->get_installed_version( $slug );
$minimum_version = $this->plugins[ $slug ]['version'];

return version_compare( $minimum_version, $installed_version, '>' );
}

/**
* Check whether there is an update available for a plugin.
*
* @since 2.5.0
*
* @param string $slug Plugin slug.

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

Change "nr" to "number" please.

* @return false|string Version nr string of the available update or false if no update available.
*/
public function does_plugin_have_update( $slug ) {

// Presume bundled and external plugins will provide at least the minimum required version
if ( 'repo' !== $this->plugins[ $slug ]['type'] ) {
if ( $this->does_plugin_require_update( $slug ) ) {
return $this->plugins[ $slug ]['version'];
}
return false;
}

$repo = get_site_transient( 'update_plugins' );

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

Should this be ! empty(), as per line 1531 below?

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

No.

The WP api for the plugin repo is largely undocumented, so I want to be a bit more careful here.
It could be returning '0' which empty would discard, but might be valid in some obscure situation.

if ( isset( $repo->response[ $this->plugins[ $slug ]['file_path'] ]->new_version ) ) {
return $repo->response[ $this->plugins[ $slug ]['file_path'] ]->new_version;
}

return false;
}

/**
* Retrieve potential upgrade notice for a plugin.
*
* @since 2.5.0
*
* @param string $slug Plugin slug.
* @return string The upgrade notice or an empty string if no message was available or provided.
*/
public function get_upgrade_notice( $slug ) {

// We currently can't get reliable info on non-WP-repo plugins - issue #380
if ( 'repo' !== $this->plugins[ $slug ]['type'] ) {
return '';
}

This comment has been minimized.

Copy link
@GaryJones

GaryJones May 3, 2015

Member

Line 1550 above uses $repo but this uses $repo_updates?

This comment has been minimized.

Copy link
@jrfnl

jrfnl May 3, 2015

Author Contributor

Changed to $repo_updates for all places where access the site transient

$repo_updates = get_site_transient( 'update_plugins' );

if ( ! empty( $repo_updates->response[ $this->plugins[ $slug ]['file_path'] ]->upgrade_notice ) ) {
return $repo_updates->response[ $this->plugins[ $slug ]['file_path'] ]->upgrade_notice;
}

return '';
}

/**
* Wrapper around the core WP get_plugins function, making sure it's actually available.
*
* @param string $plugin_folder Optional. Relative path to single plugin folder.
* @return array Array of installed plugins with plugin information.
*/
public function get_plugins( $plugin_folder = '' ) {
if ( ! function_exists( 'get_plugins' ) ) {
require_once ABSPATH . 'wp-admin/includes/plugin.php';
}

return get_plugins( $plugin_folder );
}

/**
* Delete dismissable nag option when theme is switched.
*
Expand Down

0 comments on commit 495034f

Please sign in to comment.