Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support 'wporg_status' and 'wporg_last_updated' as optional wp plugin list fields #382

Merged
merged 21 commits into from Dec 12, 2023

Conversation

janw-me
Copy link
Member

@janw-me janw-me commented Nov 10, 2023

Fixes idea#36

Extends wp plugin list with the fields:

  • wp_org which will get the wp.org status (active, closed or empty)
  • wp_org_updated which will get the last svn commit date if it's a wp.org plugin.

These fields will take a look at the status of the plugin on wordpress.org and if it ever was a .org plugin it can display the last commit date.

Related wp-cli/wp-cli#5859

@janw-me janw-me requested a review from a team as a code owner November 10, 2023 17:15
@schlessera
Copy link
Member

@janw-me the API-specific stuff should be included in the WpOrgApi class found in wp-cli/wp-cli.

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start!

I'd like to standardize on wporg_status and wporg_last_updated as the key names.

src/Plugin_Command.php Outdated Show resolved Hide resolved
src/Plugin_Command.php Outdated Show resolved Hide resolved
protected function get_wporg_data( $plugin_name ) {
$data = [
'status' => 'no_wp_org',
'last_updated' => '-',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'last_updated' => '-',
'last_updated' => '',

We can leave this empty when it's not present.

*/
protected function get_wporg_data( $plugin_name ) {
$data = [
'status' => 'no_wp_org',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'status' => 'no_wp_org',
'status' => '',

We can leave this empty when it's not present.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not happy about the label itself.
But personally I prefer when a check like this tells "something". Empty can feel like an unknown failure.

Are there other places where we leave fields empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other places where we leave fields empty?

Yes:

CleanShot 2023-12-06 at 05 18 24@2x

// Just because the plugin is not in the api, does not mean it was never on .org.
}

$request = wp_remote_get( "https://plugins.trac.wordpress.org/log/{$plugin_name}/?limit=1&mode=stop_on_copy&format=rss" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we only make this request if we're checking for the date?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a better check in d7ec274
The answer is, not always,

When the call to api.wp is not succesfull, we need to check the SVN to distinguish between closed and never on .org.

@janw-me
Copy link
Member Author

janw-me commented Nov 11, 2023

I'd like to standardize on wporg_status and wporg_last_updated as the key names.

Relabeling done in: fd9a235

@janw-me
Copy link
Member Author

janw-me commented Nov 11, 2023

@janw-me the API-specific stuff should be included in the WpOrgApi class found in wp-cli/wp-cli.

Fixed in 71a1d23
Personally I don't think this is better, as this does the whole request instead of the HEAD.
But I also value consistency , so fine.

'auto_update' => false,
'author' => $item_data['Author'],
'wporg_status' => 'no_wp_org',
'wporg_last_updated' => '-',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per another comment, let's leave this empty per default instead

Suggested change
'wporg_last_updated' => '-',
'wporg_last_updated' => '',

@schlessera
Copy link
Member

Personally I don't think this is better, as this does the whole request instead of the HEAD.

@janw-me Note: The WpOrgApi class can be adapted to make sure it fulfills all the requirements, of course. It is not considered feature-complete at this point and was meant to be extended/improved as needed.

I'll see how we can adapt it so it does not unnecessarily do the full request.

@danielbachhuber
Copy link
Member

@janw-me Once your PR is ready, can you also update the PR description with details on how the new feature works? Thanks!

@danielbachhuber danielbachhuber added this to the 2.1.17 milestone Dec 12, 2023
@danielbachhuber danielbachhuber changed the title Check if Installed Plugins Are No Longer in the Plugin Directory Support 'wporg_status' and 'wporg_last_updated' as optional wp plugin list fields Dec 12, 2023
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this, @janw-me !

I made a couple of tweaks (c927a46, 9d97c95) and regenerated the README (1dc730e).

Also, as a FYI for the future, please create a feature branch for pull requests as it makes things a bit easier to work with.

@danielbachhuber danielbachhuber merged commit 12618c7 into wp-cli:main Dec 12, 2023
36 checks passed
@janw-me
Copy link
Member Author

janw-me commented Dec 13, 2023

I made a couple of tweaks (c927a46, 9d97c95) and regenerated the README (1dc730e).

Great additions.

Also, as a FYI for the future, please create a feature branch for pull requests as it makes things a bit easier to work with.

Will keep this in mind next time 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check if Installed Plugins Are No Longer in the Plugin Directory
4 participants