#51742 closed defect (bug) (fixed)
Core auto-updates UI: disable checkbox when filter or constant takes precedent on the option
Reported by: | audrasjb | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 5.6 | Priority: | normal |
Severity: | normal | Version: | 5.6 |
Component: | Upgrade/Install | Keywords: | has-patch has-screenshots commit |
Focuses: | ui | Cc: |
Attachments (31)
Change History (90)
#1
@
4 years ago
- Keywords has-patch has-screenshots needs-copy-review needs-design-feedback dev-feedback 2nd-opinion added; needs-patch removed
51742.diff
disables the checkbox and append a small information about this in the label, in italics. The goal of this patch is to make sure constants and filters will take precedent on user settings.
Just adding some thoughts:
- We could also hide the whole auto-updates section when filters/constants are used.
- Of course, the disabled state of an input element can always be bypassed, but I think it doesn't matter as the constant/filter will always take precedent.
#2
@
4 years ago
Patch 51742.patch removes extra space from the disabled
tag and checked function.
When auto-update set to false using define('WP_AUTO_UPDATE_CORE', false)
then an empty p tag generated that also removed in a patch.
Feel feel to update patch if anything wrong
#3
@
4 years ago
Hmm, maybe the text
This setting has been disabled using a constant or a filter.
would be better as
This setting was overridden programmatically. (or something similar)
Thinking majority of users wouldn't know what constant
and filter
are. Perhaps better to avoid mentioning these in the UI.
#4
@
4 years ago
Thanks @azaozz I agree with you, that's better 👍
Will refresh the patch accordingly.
@mukesh27 I was going to address the <p> issue in another ticket, but let's address this here. I'll update your patch though, as there is a WPCS issue in it :)
#5
@
4 years ago
@audrasjb Patch 51742.2.diff address src/js/_enqueues/vendor/tinymce/tinymce.js
related updates
#6
@
4 years ago
- Owner set to audrasjb
- Status changed from new to accepted
Ah thanks, forgot to merge master changes ;)
Patch updated.
@
4 years ago
Upgrade/Install: Disable major auto-updates checkbox and display a message when WP_AUTO_UPDATE_CORE
constant and/or allow_major_auto_core_updates filter
are defined
This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.
4 years ago
#9
follow-ups:
↓ 11
↓ 13
@
4 years ago
- Keywords 2nd-opinion removed
As per today’s weekly core-auto-updates team meeting, this patch implements the following parts of the new scope for major versions auto-updates system:
- Include all previous changes concerning filters and constants
- Move the auto-updates UI to the top and replace form/checkbox with action links
@karmatosed @helen I'll post few screenshots. I think the technical part is ready and we can iterate on design/copy.
Pinging @whyisjake for a quick security review as action links are using GET variables.
@
4 years ago
Running trunk: auto-updates are enabled so we have a link to disable them (PS: I just clicked on the "enable" action link so we also have the notice bar on the top of the screen)
@
4 years ago
Auto-updates settings are already managed using a constant or filter: the option is not available and there is a sentence to explain that.
This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.
4 years ago
#11
in reply to:
↑ 9
@
4 years ago
Replying to audrasjb:
Looking at 51742.3.diff, see couple of problems and have few (nitpick) suggestions :)
core_auto_updates_settings()
should use a nonce when saving the options. As it's a link now thecore-auto-updates-major
query arg and the nonce will be in the$_GET
which is not ideal (if the user bookmarks the page while?core-auto-updates-major=...
is in the URL, it will try to set the option every time they use that bookmark). To fix that it should usewp_nonce_url()
and then immediately redirect after the option is updated (for example see how "action links" in list tables work).
- The filter
allow_major_auto_core_updates
runs two times in a row. Perhaps that's a typo, it's pretty confusing and not sure it is necessary.
#12
@
4 years ago
- Keywords needs-refresh added
I'll add the nonce and immediately redirect just like other action links do 👍
For allow_major_auto_core_updates
, that's indeed a typo, sorry about that…
Thanks @azaozz for your review!
#13
in reply to:
↑ 9
;
follow-up:
↓ 18
@
4 years ago
- Keywords needs-refresh removed
Replying to audrasjb:
Suggestions and nitpicks:
- The
$major_optin_disabled = false;
var is a "double negative". Perhaps would be easier to read/understand if it was a "positive", i.e.$major_optin_enabled = true;
? Also the name could be a bit more descriptive, perhaps, like$upgrade_major_optin_enabled
or similar.
- The
$is_major_optin_disabled
doesn't seem needed? If there should be a specific filter for disabling major core updates that runs in addition to$upgrade_major = apply_filters( 'allow_major_auto_core_updates', ...
, it can perhaps be checked right in the conditional (and expect only booleantrue
).
- Perhaps
do_action( 'after_core_auto_updates_settings_fields', $auto_update_settings );
can be onlyafter_core_auto_updates_settings
as there aren't any "fields" there.
- The
auto_update_core_major
and similar site options would perhaps be better as explicitly set toenabled
ordisabled
strings. It's more readable and, (as PHP 8.0 explicitly requires) it would be better to save and then expect a string, not a boolean that gets saved in the DB as a number, etc. Also, these new options should be pre-populated when updating to WP 5.6.
- The
WordPress auto-update settings updated.
notice could perhaps be more specific and "tell" whether major core auto-updates were enabled or disabled.
- Looking at the UI, maybe it would be better to have a "link as a button" for the action link? The rest of the "actions" are all buttons.
#14
follow-up:
↓ 16
@
4 years ago
@azaozz ah, about running the filter two times in a row, that's because it serves two different goals:
$upgrade_major = apply_filters( 'allow_major_auto_core_updates', $upgrade_major );
The idea is to get the value or the filter or to apply $upgrade_major
value. This is for the final value of the option.
$is_major_optin_disabled = apply_filters( 'allow_major_auto_core_updates', null );
The idea is to test if the filter should disable the UI. If it returns null
, we know that the filter is not applied.
But that's coming from the previous way this section was managed. We can probably adapt that and get rid of this double call.
#15
follow-up:
↓ 17
@
4 years ago
Just saw your last comment. All your points totally make sense to me, thanks 🙏
New patch coming soon :)
Ah. Except item 6: the idea is also to decrease the amount of buttons on this page, as this is something asked by the design team :)
#16
in reply to:
↑ 14
@
4 years ago
Replying to audrasjb:
@azaozz ah, about running the filter two times in a row, that's because it serves two different goals
...
The idea is to test if the filter should disable the UI
Ah, I see. Yeah, it is quite confusing (for plugins) now, as the same filter runs twice and filters different values. Perhaps the second run can be another, specific filter? Lets discuss/figure it out on Slack :)
#17
in reply to:
↑ 15
@
4 years ago
Replying to audrasjb:
Except item 6: the idea is also to decrease the amount of buttons on this page, as this is something asked by the design team :)
Right, it's just a nitpick. An inline link is less noticeable/less "important" than a button. Best to implement the design team's decision :)
#19
@
4 years ago
- Keywords needs-refresh added
Uh, didn't mean to remove needs-refresh
, posting too fast, sorry :)
This ticket was mentioned in Slack in #core by azaozz. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-auto-updates by azaozz. View the logs.
4 years ago
#22
@
4 years ago
This is already looking a lot better I think from a position and format view. Thanks for the rapid work. I think it's also really important to see this as a step to iterate from, over being set in stone for coming up versions.
The big thing right now for me to work on for this release is copy. There's an opportunity here to avoid the confusion and sea of content that we are adding to with this. I would like therefore some consideration before strings go in to a few areas:
- Concise, exact language: how space can we be, but still clear?
- Clarity for translation: let's be aware this is going to be translated and consider that for understanding.
- Warmth: how can we ease any feeling around this feature in the copy?
- Balance: whilst we want to encourage this feature, we want it to be natural to use and trusted, not feel a big deal. Copy can help there with tone.
By reducing the interface to a link, the weight has been taken from it, so now we can turn to the text being delivered itself.
A few specific things I think need changing are:
- Length of linked text: I see in one screenshot a link that is over 8 words long, let's try and avoid that.
- Consider opening words like inviting, is this too formal? It might not be, but let's consider the tone of the messaging here.
- Potentially use more spacing between messages and group them. I am not sure when we have 4 lines of text in row we can't also just have some extra white space. This could give breathing room in a sea of text as per the mockups around this feature, I'd love to see that come to this a little.
Perhaps as it's important, writing out each potential message strings is useful to then iterate collectively in this ticket?
#23
@
4 years ago
- Keywords needs-testing added; needs-refresh removed
In 51742.5.diff:
- Building on 51742.4.diff.
- Use
action
to follow the existing pattern. - Use
has_filter()
when determining if the UI has been overridden by a plugin/theme. - Some cleanup, etc.
This ticket was mentioned in Slack in #core-auto-updates by knutsp. View the logs.
4 years ago
#25
follow-up:
↓ 26
@
4 years ago
- Keywords needs-design-feedback removed
Thanks @azaozz! Just added 51742.6.diff
which updates your patch to also give precedence to AUTOMATIC_UPDATER_DISABLED
, as noted by @knutsp :)
Also, I wanted to mention that @karmatosed and @hedgefield worked on the UI copy. I'll update the patch with the decided strings later.
https://docs.google.com/document/d/1mxplJ4BfWLxtbxpFhPTSH1x3FpbIiBfOfzNECh37Mm8/edit?usp=sharing
#26
in reply to:
↑ 25
@
4 years ago
Replying to audrasjb:
Just added
51742.6.diff
which updates your patch to also give precedence toAUTOMATIC_UPDATER_DISABLED
Thanks, looks good :)
There's some more: we changed the values saved in the DB for the new options (auto_update_core_dev
, auto_update_core_minor
, and auto_update_core_major
) to avoid PHP type juggling, but the filtered values for allow_dev_auto_core_updates
, allow_minor_auto_core_updates
, and allow_major_auto_core_updates
filters would still need to be boolean as these filters have existed since 2013. Patch coming up.
Also all new options have to be added to populate_options()
to avoid making extra calls to the DB for non-existing options. Will update the patch on #51743.
Still todo: test in multisite/network in both existing and new installs.
#27
@
4 years ago
In 51742.7.diff:
- Update expected option values in
Core_Upgrader::should_update_to_version()
. - Use boolean values for the (pre-existing)
allow_dev_auto_core_updates
,allow_minor_auto_core_updates
, andallow_major_auto_core_updates
filters. - Fix couple of typos in comments.
#28
@
4 years ago
Alright, out of the doc suggestions these seem the most agreed upon ones we can use for beta 4 this afternoon, with some slight edits to make the phrasings match as much as possible:
Use case 1 - Auto-updates were enabled by clicking the action link and no constant or filter are used:
Automatic updates to new versions of WordPress are enabled. You can <a>turn this off</a>.
Use case 2 - Auto-updates for major versions are disabled and no constant or filter are used:
If you want you can <a>automatically update your WordPress site</a>.</a>
Use case 3 - Auto-updates were enabled via a filter or constant:
Your WordPress site will be kept updated automatically.
Use case 4 - Auto-updates were disabled via a filter or constant:
Automatic WordPress updates have been disabled for this site.
Use case 5 - Core auto-updates are completely disabled:
Automatic WordPress updates are not available for this site.
Admin notices after settings changes, displayed on page reload:
WordPress will auto-update to new major versions. WordPress will no longer auto-update to new major versions.
#29
@
4 years ago
- Keywords needs-copy-review dev-feedback needs-testing removed
Thanks for the update @azaozz!
Thanks for the strings @hedgefield!
51742.8.diff
implements those string changes 🙂
I believe we are good to go, nice work all!
#30
@
4 years ago
- Keywords commit added
I checked again 51742.8.diff
and it looks good to me. Adding commit
keyword.
#31
@
4 years ago
Some general notes from me on strings here (which I can also update a patch for):
- I would like for strings to refer to "this site" intsead of "your site". This mirrors some other string changes that have been made to reflect that many admins have multiple sites and that a given site might not be "theirs".
- I do not think that it makes sense to refer to "major" or "feature" releases. Even I'm confused about this means in the context of the WordPress admin - it only makes sense to me as a core developer.
- For the purposes of these strings, there are 3 states: all core autoupdates on, minor autoupdates on, no autoupdates on. Filter/constant usage means removing the action link.
- I think there are actually two places where a string can show - at the top where it has the check again link under the heading, and within the update available section below. For the upper one, I would like this to be a statement about what is enabled and with a clear action link. For the lower one, I think this could be more of a conversational "would you like to change this to X" type of string.
- Per the above, these are my suggestions which were in the doc but I think overlooked because there was a lot going on:
Top string, all updates on:
This site is automatically kept up-to-date with each new version of WordPress. <a>Switch to automatic updates for maintenance and security release only</a>
Top string, minor updates on:
This site is automatically kept up-to-date with maintenance and security releases of WordPress. <a>Enable automatic updates for all new versions of WordPress</a>
Top string, no updates on (this can only be done in code so no action link)
This site will not receive automatic updates for new versions of WordPress.
Admin notice after turning on major updates:
Automatic updates for all WordPress versions have been enabled. Thank you!
Admin notice after turning off major updates:
WordPress will only receive automatic security and maintenance releases from now on.
For the lower update section, I think we would only want to show an action link to enable automatic updates going forward as an additional prompt. This could read:
You can also <a>enable automatic updates for all version of WordPress</a> from now on.
This ticket was mentioned in Slack in #core-auto-updates by helen. View the logs.
4 years ago
#33
follow-up:
↓ 36
@
4 years ago
@helen I like those iterations. Good point about taking it away from personal to site specfic, alinging it with other strings is important.
My only ponder is the long sentence link, but it's not a deal breaker for me:
Switch to automatic updates for maintenance and security release only
Potentially I'd iteration to:
This site is automatically kept up-to-date with each new version of WordPress. <a>Switch to automatic updates</a> for maintenance and security release only.
This site is automatically kept up-to-date with maintenance and security releases of WordPress. <a>Enable automatic updates</a> for all new versions of WordPress.
I would +1 those string changes as we can get them in for beta then.
This ticket was mentioned in Slack in #core by helen. View the logs.
4 years ago
#35
@
4 years ago
As said on Slack and just in case you didn't see the conversation @karmatosed, I'm afraid we can't use "Switch to automatic updates for maintenance and security release only" as disabling major auto-udpates doesn't mean that security updates will be enabled. I know that sounds weird, but technically, minor updates can be disabled while majors are enabled 😅
That's possible because of filters bellow: They were introduced in WP 3.7 and are totally independant each others:
add_filter( 'allow_dev_auto_core_updates', '__return_true' ); // Enable development updates add_filter( 'allow_minor_auto_core_updates', '__return_true' ); // Enable minor updates add_filter( 'allow_major_auto_core_updates', '__return_true' ); // Enable major updates
#36
in reply to:
↑ 33
@
4 years ago
Replying to karmatosed:
I of course would like to see even something that happens automagically use active verbs.
How about:
This site stays up to date automatically with every new version of WordPress ...
for majors,
and
This site stays up to date automatically with maintenance and security releases ...
PS -- we only hyphenate 'up to date' when it directly modifies another word, becoming an adjective or adverb phrase. When it's the object of a verb that's acting as a standin for the verb 'to be', we don't hyphenate.
So, 'It's up to date!' But, 'I have the up-to-date strings.'
@helen I like those iterations. Good point about taking it away from personal to site specfic, alinging it with other strings is important.
My only ponder is the long sentence link, but it's not a deal breaker for me:
Switch to automatic updates for maintenance and security release onlyPotentially I'd iteration to:
This site is automatically kept up-to-date with each new version of WordPress. <a>Switch to automatic updates</a> for maintenance and security release only.This site is automatically kept up-to-date with maintenance and security releases of WordPress. <a>Enable automatic updates</a> for all new versions of WordPress.I would +1 those string changes as we can get them in for beta then.
#38
follow-up:
↓ 43
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
[49587] gives us something like https://share.getcloudapp.com/v1uxXNpm (note that there are a ton of possible UIs for this screen that need screenshot documentation). Reopening because I would like to do the following before RC:
- Add an additional prompt link in the
An updated version of WordPress is available.
section. Note that I've changed the string there fromYou can update ... automatically
string to sayYou can update ... manually
to reflect that you are clicking the button, not doing something with automatic updates. Per my other comment, I think this could say something likeYou can also <a>enable automatic updates for all versions of WordPress</a> from now on.
. - Potentially abstract the logic around getting the autoupdate settings into its own function, as the above will also need that information, and this whole file is very messy with functions and procedural PHP so it's a bit hard to trace what's going on.
- Consider adding a string for when major auto-updates are on but minor ones are off - IMO this is a strange and non-ideal thing to do, but is technically possible so the informational part should probably reflect that. I really don't like referring to these as "major" or "feature" updates in user-facing strings but we probably can't avoid that entirely here. Because it is only possible to turn off minor updates via code and I consider this to be a non-ideal state, we do not need an action link.
For the last item, this technically boils down to changing if ( $upgrade_major )
to if ( $upgrade_major && $upgrade_minor )
and then adding an additional elseif ( $upgrade_major )
with whatever we think the right string is for only updating to new major/feature versions.
In 5.7, I would like to tackle further UI updates to the updates screen in #51540 (in particular condensing the button overload) and take a look at also adding another way to manage these settings in general or similar. There are some other potential 5.7 items on https://make.wordpress.org/core/2020/11/10/wp5-6-auto-update-implementation-change/
This ticket was mentioned in Slack in #core-auto-updates by helen. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by markparnell. View the logs.
4 years ago
#41
@
4 years ago
add_filter('automatic_updater_disabled', '__return_true');
should result in the message This site will not receive automatic updates for new versions of WordPress.
, however it currently has no impact on the UI. Setting the corresponding constant to false
does result in the UI updating correctly.
#42
follow-ups:
↓ 44
↓ 47
@
4 years ago
- Keywords commit removed
@markparnell thanks for spotting this during the release party.
51742.10.diff
fixes this issue.
It was not a blocker for beta 4 but that's definitely something we want to fix before RC1 :)
#43
in reply to:
↑ 38
@
4 years ago
Replying to helen:
Consider adding a string for when major auto-updates are on but minor ones are off - IMO this is a strange and non-ideal thing to do, but is technically possible...
Yeah, this doesn't make sense even though it's been technically possible. Logically, when turning major updates on, it should override minor updates to always "on".
This is the same case as when beta
or rc
are set in the WP_AUTO_UPDATE_CORE
constant, see: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-core-upgrader.php#L287. So instead of adding a string, lets "fix" the logic there. True, this is a change (a logic enhancement?) to how the various "global" auto-update filters and constants work, but thinking now is the time to fix it.
Plugins can block any update depending on the "offered version" with the auto_update_[core]
filter: https://core.trac.wordpress.org/browser/tags/5.5.3/src/wp-admin/includes/class-wp-automatic-updater.php#L178, see the $item
param. It is possible to fully control which updates are automatically applied without even looking at the "global settings" in options, constants, and other filters.
#44
in reply to:
↑ 42
@
4 years ago
Replying to audrasjb:
@markparnell thanks for spotting this during the release party.
51742.10.diff
fixes this issue.
51742.10.diff looks good.
Yeah, it's a pretty "big mess" how the various auto-update constants and filters work. That also affects the UI...
Would be great to add a (big) docblock explaining all of these and how they interact, just have to find the right place for it :)
As of WP 5.6:
- There are three options that (may) control core auto-updates:
auto_update_core_dev
,auto_update_core_minor
, andauto_update_core_major
. There is UI only for the last one, and that UI is on the Dashboard -> Updates screen. There are various constants and filters that override these options.
- There is the
WP_AUTO_UPDATE_CORE
constant that can be used to enable or disable core auto-updates for beta, rc, minor and major versions.- This constant can be overridden by the
allow_dev_auto_core_updates
,allow_minor_auto_core_updates
, andallow_major_auto_core_updates
filters. - This constant and the filters that override it can be overridden by the
auto_update_[core]
filter inWP_Automatic_Updater::should_update()
.
- This constant can be overridden by the
- Then there is also the
AUTOMATIC_UPDATER_DISABLED
constant that overrides theWP_AUTO_UPDATE_CORE
constant and all the filters that override it (including the last one).- The
AUTOMATIC_UPDATER_DISABLED
constant also has a filterautomatic_updater_disabled
to override it. - In addition
AUTOMATIC_UPDATER_DISABLED
and the corresponding filter can be overridden for themes and plugins by theplugins_auto_update_enabled
andthemes_auto_update_enabled
filters inwp_is_auto_update_enabled_for_type()
. (Currently this function only supports plugins and themes. Perhaps would be good to add support for core too?)
- The
Did I miss something? :)
Please edit/correct the above and lets add it in a docblock, probably best in update-core.php
or for the abstracted function @helen mentioned above.
#47
in reply to:
↑ 42
@
4 years ago
Replying to audrasjb:
51742.10.diff
fixes this issue.
Just a quick note that in [49592] I changed the value passed to the filter from null
to false
, for consistency with the corresponding Site Health check and the documented type of 'bool'.
I know we pass null
to the auto_update_{$type}
filter to detect whether nothing has hooked into it, as discussed in #50848, but in this case it looks like we just need to detect whether the updater is disabled or not, so passing false
seems appropriate.
@
4 years ago
Add "only" to Core auto-updates status string when sites are set to auto-update to minors versions only
#48
@
4 years ago
As seen with @azaozz and @karmatosed in Core auto-updates Slack channel today, let's add "only" to the auto-updates status string when updates are set to minor versions only.
Before patch:
This site is only automatically kept up to date with maintenance and security releases of WordPress. <a>Enable automatic updates for all new versions of WordPress.</a>
After patch
This site is automatically kept up to date with maintenance and security releases of WordPress. <a>Enable automatic updates for all new versions of WordPress.</a>
This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.
4 years ago
@
4 years ago
Add "only" to Core auto-updates status string when sites are set to auto-update to minors versions only and adding a heading in case we are running a development version of WordPress
@
4 years ago
Add "only" to Core auto-updates status string when sites are set to auto-update to minors versions only and adding a heading in case we are running a development version of WordPress. Also adds a class to the enable|disable link for better CSS/JS targeting
#50
@
4 years ago
- Keywords commit added
In 51742.13.diff
:
- Add "only" to Core auto-updates status string when sites are set to auto-update to minors versions only
- Adding a specific heading in case we are running a development version of WordPress
- Add a class to the enable|disable link for better CSS/JS targeting
Adding commit
keyword as all of this was discussed and reviewed in core-auto-updates slack channel.
This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-auto-updates by hellofromtonya. View the logs.
4 years ago
#53
@
4 years ago
51742.14.diff incorporates 51742.13.diff (with one minor change to the new "only" string as suggested on slack) and adds an additional check for whether the site is under VCS and displays a separate "auto-updates disabled" for that case.
This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.
4 years ago
This ticket was mentioned in PR #751 on WordPress/wordpress-develop by helen.
4 years ago
#55
Trac ticket: https://core.trac.wordpress.org/ticket/51742
@
4 years ago
After 15.diff, VCS message, on latest development, showing that you can still install the nightly which seems fine
#56
@
4 years ago
51742.15.diff results in the 4 screenshots above when under VCS with various faked version strings (outdated develop, up to date develop, outdated stable, up to date stable). It's an iteration on 51742.14.diff that cleans up some logic in core_upgrade_preamble()
so that only one h2
is shown at a time (falling back to You have the latest version of WordPress.
if something odd happens). It also ensures that the core-major-auto-updates-saved
query arg is stripped from the URL because it's only for a dismissible notice.
The one kind of funny thing here is that if you are on the latest dev version it will actually still give you the button to install the latest nightly but it doesn't necessarily say that's an update, which I think is fine because of the nature of nightlies and how they're built. You will also typically see the update available message on SVN checkouts because the nightly version number is bumped more often than it is in SVN. I kind of wonder if we should be hiding that section entirely for VCS checkouts but that's not for 5.6, and it's going to make working on that section a lot more annoying later as contributors :)
Upgrade/Install: Disable major auto-updates checkbox when
WP_AUTO_UPDATE_CORE
constant and/orallow_major_auto_core_updates filter
are defined