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

Add auto-update-enabled and auto-update-disabled views on the plugins screen #18

Merged
merged 4 commits into from Mar 5, 2020

Conversation

@pbiron
Copy link
Collaborator

pbiron commented Feb 27, 2020

Fixes #17

@pbiron

This comment has been minimized.

Copy link
Collaborator Author

pbiron commented Feb 27, 2020

There are inline comments and notes in the DocBlocks of the 2 functions this PR adds that describe where changes will be needed if/when this is merged into core.

Adding the views is straightforward, with core's views_plugins filter.

Limiting the plugins displayed in the list table when one of those views is current is a bit of a hack, but since the readme says

with potentially some hacks when needed

I guess it's OK :-)

The hack uses core's pre_current_active_plugins action to override what happens in WP_Plugins_List_Table::prepare_items() when one of the links in the views is clicked on.

@pbiron

This comment has been minimized.

Copy link
Collaborator Author

pbiron commented Feb 27, 2020

Also, the hack will not be necessary once the auto-update-enabled and auto-update-disabled "status" are incorporated into the code in WP_Plugins_List_Table::prepare_items().

Copy link
Collaborator Author

pbiron left a comment

The change on line 31 was not intentional....there must be a whitespace difference between that line in master and in the plugin in the .org repo.

@audrasjb audrasjb self-requested a review Feb 27, 2020
@audrasjb audrasjb self-assigned this Feb 27, 2020
@audrasjb

This comment has been minimized.

Copy link
Owner

audrasjb commented Feb 27, 2020

Thanks @pbiron !
Yeah, I know for the hack, no worries.
Also, worth noting the feature is already implemented in the Trac ticket: https://core.trac.wordpress.org/attachment/ticket/48850/48850.11.diff

Worth also adding it to the Feature Plugin, thanks!
Will test the patch later today,

Cheers 🍻
Jb

@pbiron

This comment has been minimized.

Copy link
Collaborator Author

pbiron commented Feb 27, 2020

Also, worth noting the feature is already implemented in the Trac ticket: https://core.trac.wordpress.org/attachment/ticket/48850/48850.11.diff

Cool, didn't realize that.

Worth also adding it to the Feature Plugin, thanks!
Will test the patch later today,

Would it help if I updated the PR to more closely match what is in the diff on Trac?

@pbiron

This comment has been minimized.

Copy link
Collaborator Author

pbiron commented Feb 27, 2020

And shame on me for using the right hook to add the views in multisite and for adding them on the non-network plugins screen in multisite :-) Me, of all people, should have known better :-)

I'll update the PR accordingly, but will wait to hear back from you about whether I should also make things more closely match what's in the diff on Trac.

@audrasjb

This comment has been minimized.

Copy link
Owner

audrasjb commented Feb 27, 2020

That's not mandatory, but it would be a nice to have indeed :-)

@pbiron

This comment has been minimized.

Copy link
Collaborator Author

pbiron commented Feb 27, 2020

PR updated:

  1. uses the same strings and array indexes for the views as in https://core.trac.wordpress.org/attachment/ticket/48850/48850.11.diff
  2. in multisite, only adds the views on the plugins-network screen
  3. adds the wp-autoupdate domain to the _n() calls for the strings
@audrasjb

This comment has been minimized.

Copy link
Owner

audrasjb commented Feb 27, 2020

I made some fixes (in a PR in your branch):

  • use auto-update-enabled and auto-update-disabled custom statuses everywhere otherwise it wont work
  • fix typo on $wp_auto_update_plguins variable :)
  • to get the number in $enabled_count, we have to use only plugins that exists in the WP installation. Indeed some of them could be deleted and still be in wp_auto_update_plugins site option :D
@audrasjb audrasjb added this to the 0.2.0 milestone Feb 28, 2020
….trac.wordpress.org/attachment/ticket/48850/48850.11.diff; 2) fix typo in $wp_autoupdate_plugins variable name; 3) when computing the counts of plugins in each view, remove entries from the wp_auto_update_plugins site option that are no longer installed.
@pbiron

This comment has been minimized.

Copy link
Collaborator Author

pbiron commented Feb 28, 2020

PR updated.

  • this uses status_links array indices autoupdate_enabled and autoupdate_disabled throughout, as those are what is used in the patch on Trac. My previous commit forgot to change that in one place
  • corrects the typo in $wp_autoupdates_plugin variable name
  • uses array_intersect() to strip from the value stored in the wp_auto_update_plugins site option plugins that are no longer installed (rather than iterating over the array and incrementing the count as you suggested).
@audrasjb

This comment has been minimized.

Copy link
Owner

audrasjb commented Mar 3, 2020

Thanks for the update Paul!

@pbiron

This comment has been minimized.

Copy link
Collaborator Author

pbiron commented Mar 5, 2020

2 things before you merge this:

  1. after #32 is merged, then removing "deleted" plugins from the counts for the views isn't strictly necessary...but I think it's still a good idea to leave that code in for now
  2. if the count for a view is 0, should that view not be added? Generally speaking, that's how most of the core list tables work. For instance, if all posts have the publish status, the views do not include Pending (0), Draft (0). The one exception to that I know of is the Privacy Tools (see https://core.trac.wordpress.org/ticket/47495)
@audrasjb audrasjb merged commit ab8973f into audrasjb:master Mar 5, 2020
@audrasjb

This comment has been minimized.

Copy link
Owner

audrasjb commented Mar 5, 2020

Thanks Paul.
I'm fine with (1) for the moment and let's handle (2) in a separated issue :)

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

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.