-
Notifications
You must be signed in to change notification settings - Fork 18
Interoperability between existing and new filters and constants #95
Comments
I'll answer this one first, as I think doing so will make answering your other question easier. The way the feature plugin works is precisely by hooking into
If both of those checks are true, then it returns true from The 2nd check actually consults the I'm currently working on the patch for the merge into core and my current thinking is to do the above checks in directly in
In addition to the use above, the constants and new filters also control whether the UI appears at all. That is, if The reason the constants filters are check even when the UI isn't displayed is to avoid the case where:
Does that explanation help? |
I should note that I have not tested (not sure if others have) the case where the Updates API returns a |
Sorry, should have explained that better :) With the plugin enabled, add |
The question about Both the filter and the constant do the same thing, with the filter overriding the constant. What is the use case to warrant that? It seems either the filter or the constant would be enough (with strong preference for the filter as it is "the WP way of doing things"). |
Good catch :) Yeah, this should always update the plugin/theme but may be harder to implement in the plugin. Perhaps this can be added when merging to core. |
OK, now I get what you mean. That has been discussed although not in great detail and no decision has been made about it. For reference see audrasjb/wp-autoupdates#10 (comment) (which is a comment I made on PR that, unfortunately, didn't get migrated from the old repo to the new). Do you have any thoughts on the idea put foreword there? |
It's modeled directly on core's |
BTW, these are all great questions! Thanx for the careful consideration! |
his has been on my mind since this project started. The special thing with this featured plugin is that it builds upon functionality that is already in core, with constants and filters. So when another plugin sets some or all plugins/themes to auto update, it UI should accept that, reflect that, and not offer to enable it or disable it, just say it's enabled or disabled (by a plugin). That gives four variations in the auto updates column or box (simplified): The above should be in place at merge or before target (5.5) release. Details could be something for Site Health, if necessary to avoid confusion. This can wait. |
Right, and it adds another "layer" of constants and filters to the already existing. This makes managing the feature pretty complicated and hard to do. The main problem is that it adds UI where the user can decide to enable or disable auto-updates, but in some cases that UI doesn't reflect what actually happens. This can "misinform/mislead" the users and make the experience pretty bleak. At the same time, as mentioned by @pbiron and @seb86 on audrasjb/wp-autoupdates#10, it is not straightforward to fix. I don't see a good way to "check" the As far as I see there are couple of thing possible to improve this:
I understand this will make the UI not-as-clear/somewhat more complex, but is a lot better than giving the wrong information to the users. |
It might be easier to do that once this is merged into core. I'm close to being done with a first pass on the patch for the core merge. Once I'm pretty sure that patch is solid, I plan to experiment with some version of the approach I mentioned in that old PR to see how feasible it is. I'll be sure to report back here how that goes. |
I experimented with this a little bit over the weekend. Where the content of the
@azaozz I was wrong yesterday when I told you on Slack that some of the info that would be returned by the Using the above would still not 100% guarantee that what is displayed in the UI is what will happen in the future. For example, any plugin hooking into
I think (with the addition of the above code) that is a reasonable way to "market" this. |
Should we simply disable the display of the autoupdate column in the respective list table if Just trying to simplify some of the Slack discussion. |
Not sure that'd be the right thing to do, since those filters get passed info about a specific plugin/theme update. So, plugins that hook into them could (but are not required to) make the return value of their callback depend on the specific plugin/theme update. Of course, a plugin that wanted to "take over" the UI could certain do Which reminds me: given the slack discussion re: naming conventions, are the names of those filters clear enough that what they do is control whether the UI is displayed (and not whether auto-updates will happen)? |
The naming would indicate more that the autoupdates happen and don’t convey that it’s only about the display. |
That's what I think as well. The names of those filters should probably change to something like But as @azaozz said on Slack, naming is hard! I'm totally open to suggestions! As for the additional new filters we talked about on Slack during today's meeting, the intention of those was to control what gets displayed in the So, again, looking for more suggestions on what to name those additional new filters so that it properly conveys the intention. I now have a couple of questions:
For number 1, my initial thought is that passing slug, For number 2, my initial thought is that the return should be just the controlling plugin name |
Seems this has been discussed at different times/places but maybe a good idea to gather it in one place.
What are the expected uses of the combination of:
WP_DISABLE_PLUGINS_AUTO_UPDATE
constant, andwp_plugins_auto_update_enabled
filter, and the theme's equivalentWP_DISABLE_THEMES_AUTO_UPDATE
andwp_themes_auto_update_enabled
.Also, as far as I see the
auto_update_plugin
(old/existing) filter overrides bothWP_DISABLE_PLUGINS_AUTO_UPDATE
andwp_plugins_auto_update_enabled
making the UI "wrong". Same forauto_update_theme
.The text was updated successfully, but these errors were encountered: