#41191 closed enhancement (fixed)
Create browse happy type notice for PHP versions
Reported by: | joostdevalk | Owned by: | flixos90 |
---|---|---|---|
Milestone: | 5.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Site Health | Keywords: | has-patch servehappy has-dev-note |
Focuses: | accessibility | Cc: |
Description (last modified by )
For tons of reasons we want people to upgrade their PHP versions, preferably to PHP7. While historically we've never bothered users with this, the Yoast WHIP project has proven that doing so can actually change the statistics in a meaningful way, both because users act and because hosts become more active when pushed harder.
There's prior art for notices about PHP, both in Joomla and Drupal:
https://www.drupal.org/node/2670966
https://image.ibb.co/gFa8MQ/wwww.png
I would argue we should take a more positive approach than they do and focus on the speed benefits of PHP7.
To do this, we would:
- Introduce notices that would tell people to upgrade their PHP.
- Link those to explanatory page that tell people what PHP is and why they should care.
- Link to pages that show them how to upgrade (preferably host specific if we can).
- Give them email examples of emails they can send their host if it didn't work out.
- Send them to the WordPress hosting page if all else fails.
Let's start by fleshing out # 1, I've already got my content team working on # 2 as well. @hedgefield already had some ideas, I'll ask him to add them here :)
Attachments (28)
Change History (151)
This ticket was mentioned in Slack in #core-php by joostdevalk. View the logs.
7 years ago
@
7 years ago
Here's another (smaller) take where most background information is deferred to the page with the instructions. We should also make a page that explains exactly what PHP is and why it is important, for people who have no idea what we mean. I added a link to that in the copy of this mockup too.
This ticket was mentioned in Slack in #core-php by swissspidy. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-php by desrosj. View the logs.
7 years ago
#5
@
7 years ago
I'm very keen on informing and pushing users to upgrade (or get their hosts to upgrade) their PHP versions. A few thoughts on this
- How would we go about creating host specific links. Are we able to determine what host a WordPress site is installed on?
- The notifications @hedgefield has posted look good, but I wonder if they will clutter up the admin notices, perhaps just the 'Your site could run faster' message with link that opens the content below it?
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
7 years ago
#7
@
7 years ago
The following came out of the discussion of this ticket in the #core-php Slack channel
- We'd like to show a notice to recommend jump to PHP >= 5.6 (even better >= 7.0).
- We'd like to show that notice first for only 5.2 users, but then iterate over time to show it, up until we reach 5.5.
- Once number of 5.2 users is low enough, we bump WP core's minimum requirement to 5.3. Then over time, we do that further until we reach 5.6.
#8
follow-up:
↓ 9
@
7 years ago
Once number of 5.2 users is low enough
I'm curious whether there's any actual number behind "low enough"... :)
#9
in reply to:
↑ 8
@
7 years ago
Replying to chesio:
Once number of 5.2 users is low enough
I'm curious whether there's any actual number behind "low enough"... :)
No. That will likely be discussed at a future meeting in the #core-php channel in Slack. (Meetings each Monday at 18:00 UTC.)
#10
@
7 years ago
It seems currently 4.6% of WP installs is still using 5.2, it would indeed be nice to set a target % when to tackle the higher version(s).
I must say to be very surprised that only 52.5% is on PHP 5.6 or higher. I would have expect that one to be much higher seeing the performance gains.
Following closely!
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
7 years ago
#12
@
7 years ago
It seems currently 4.6% of WP installs is still using 5.2
Coincidentally, ~4.3% of WP installs run WordPress 3.6 or older (ie. 4+ years old and without automatic background updates option that has been introduced in 3.7 as far as I know).
The 4.6% is still too big number to ignore, but I think more reasonable way to make decisions based on these stats is to apply them only to WordPress installs that are "actively maintained". We can argue what "actively maintained" means, but I don't believe that someone suddenly decides to hit the "Update WordPress" button after not doing so in 4+ years...
#13
@
7 years ago
While I certainly like this idea, I think showing a very prominent notice to users should be the last step though on our way to making users upgrade their PHP version.
For example I consider #40934 more important as it is a more organic way to push users to a higher version. When they see a plugin won't work any longer, they know for fact they have a problem and are more likely to make efforts. Performance and security are both rather abstract arguments to users.
This notice considered here will likely make more people upgrade, but it will also be possibly annoying, not give a user a reason they can fully grasp. I think less aggressive solutions should be the first step.
#14
@
7 years ago
I'll mention here that the Yoast notice about PHP being "out of date" was poorly done on several levels:
- Far too large a notice (15 or so lines!) - should have been a 1 or 2 line banner with a link to read more about the issue.
- It was undismissable, which was frankly appalling. If it must re-appear at some point, have it appear once a month or when the Yoast plugin is updated again.
- The text made some highly dubious statements like "your version of PHP no longer receives security updates" (not true - CentOS backports security updates and that's what we are running) and "they [hosters] don't dare to do that [upgrade] because they're afraid they'll break your site" (we went through a PHP 5.3.3 -> 5.6.31 upgrade recently and it completely broke a site in such a manner we had to move that site back to an earlier PHP - it was literally unfixable with 5.6.31. So it's not being "afraid" that's the issue - we jumped right in there - it's the fact that sites *do* break on PHP upgrades).
I think my main concern here is that no-one's discussing LTS releases here like RHEL/CentOS that backport PHP fixes from later releases, but keep the base version the same. They'll get hit with messages telling them their 5.3.3 (RHEL/Centos 6) PHP is out of date when in fact it isn't. Yes, they can use the IUS repo (maybe WordPress should have docs on how to upgrade PHP on various platforms if they're going to push PHP upgrade notices? At least the notices can then link to the appropriate docs page for the platform), but the IUS repo is what broke one of our sites, so it's not a guarantee at all that things will work after an upgrade.
This ticket was mentioned in Slack in #core-php by nerrad. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-php by schlessera. View the logs.
7 years ago
#17
@
7 years ago
I think this is a great idea. I agree to most in #comment:14 regarding that we need to consider that on RedHat 6 the old PHP 5.3 version still does get security updates, and that the notification should be dismissable.
There has not been any activity on this for 4 months. Why hasn't the idea been blessed already? Is there a patch / git branch somewhere?
This is something we would like to contribute to, but the issue has no owner and no concrete channel where we could focus our energy to get progress. I hope some core devs could facilitate here a bit. Greetings from the WC Athens Contributor day.
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
7 years ago
#20
@
7 years ago
- Owner set to schlessera
- Status changed from new to assigned
I take on ownership for this ticket, as I intend to push for making progress in the coming weeks.
#21
@
7 years ago
@ottok This is being discussed in the #core-php channel. We meet once per week to move the "servehappy" project forward, of which this is one part. You can find summaries of our meetings here: https://make.wordpress.org/core/tag/core-php/
#22
@
7 years ago
@schlessera I attended the meeting on Dec 11th, but I don't have time to attend it weekly. Nice to see progress on this one!
#23
@
7 years ago
At the request of @schlessera I adjusted the backend notification mockup to have a single CTA that goes to a landing page somewhere in the wordpress ecosystem with the information and instructions (for which we also made a quick mockup). Since the draft of the copy for that page is quite long, we divided it up into expandable sections, kinda like the Gutenberg sidebar. But feel free to run with the layout of that page, this is just an idea. We made a few more sketches if needed.
This ticket was mentioned in Slack in #core-php by mte90. View the logs.
7 years ago
#25
@
7 years ago
Once we have a basic version of the admin notice working, we should discuss whether and how to backport it to earlier versions of WordPress and distribute through auto-updates. This way, we could reach a bigger audience.
#26
@
7 years ago
distribute through auto-updates. This way, we could reach a bigger audience.
Doing it past the current-stable (4.9) wouldn't gain much audience - most older autoupdates rarely have administrators viewing dashboards IMHO (as evidenced by them ignoring the WordPress update banners)
#27
@
7 years ago
- Milestone changed from Awaiting Review to Future Release
Per yesterday's PHP chat, the information page is gonna live at wordpress.org/support/upgrade-php/
. See https://meta.trac.wordpress.org/ticket/2996 for more information.
I started work on the core notice in https://github.com/wp-core-php/wordpress-develop/pull/1. For the first iteration I wanted to stick with UI patterns that core already includes, so the notice looks minimally different from the mockups here (I based it on notice WP backend.png). But we can always consider changing that, as necessary.
Let's continue the discussion and work on the core notice on that pull request, before we get back to SVN and Trac for an eventual merge.
#28
follow-up:
↓ 29
@
7 years ago
@dd32:
most older autoupdates rarely have administrators viewing dashboards IMHO (as evidenced by them ignoring the WordPress update banners)
What about those site owners that were burned by a major update once and have avoided them since? WordPress updates always contain lots of feature changes, and sites that were built in a fragile way are very easy to break on update.
Do we have any data available that helps us make an educated guess about how worthwhile this would be?
@flixos90:
Let's continue the discussion and work on the core notice on that pull request, before we get back to SVN and Trac for an eventual merge.
Sounds like a plan!
#29
in reply to:
↑ 28
@
7 years ago
Replying to schlessera:
@dd32:
most older autoupdates rarely have administrators viewing dashboards IMHO (as evidenced by them ignoring the WordPress update banners)
What about those site owners that were burned by a major update once and have avoided them since? WordPress updates always contain lots of feature changes, and sites that were built in a fragile way are very easy to break on update.
Do we have any data available that helps us make an educated guess about how worthwhile this would be?
We don't really have any data that could be used to infer that someone is directly avoiding updating, however, I can say that 99.9% of sites which updated to 4.8.5 did so automatically, leaving 0.1% who updated by clicking the button. The number is so small that it's entirely possible it's an automated tools which show up as manual updates in our stats.
I don't think that number says that they're avoiding the major version update, but it could be.
#30
@
7 years ago
99.9% of sites which updated to 4.8.5 did so automatically, leaving 0.1% who updated by clicking the button
Oh, interesting data point (and interesting approach to finding an answer)!
I guess we could just deploy to what we think would make sense within reasonable effort, and then keep an eye on the PHP version x WordPress version statistics. This would tell us whether the portion with unsupported versions that we're not reaching is significant or not. If it isn't, we know we don't need to bother with the backporting. If it is, we can relaunch the backporting discussion.
#31
@
7 years ago
The Servehappy page draft is now available via public preview at https://wordpress.org/support/?page_id=9948338&preview=1&_ppp=6866f27cbf. Please do another review of the content so that we can publish it.
On the core end of things, 41191.diff implements the admin notice in core which links to the above support page. The patch was created from [this pull request on GitHub](https://github.com/wp-core-php/wordpress-develop/pull/1), please have a look for further information and reasoning on the patch.
This ticket was mentioned in Slack in #core by flixos90. View the logs.
7 years ago
#34
@
7 years ago
Just making note of some of the feedback made in slack when this was announced there. Click through the link to read.
Whatever final implementation we land with, discoverability is a key need here to support the goals of this initiative. So we cannot avoid some form of notice
I'm just throwing out another option as well as an idea: the WordPress “About” splash page is shown after every update. What about a new tab on that page that is colored to draw attention and dedicated to healthcheck/php version info. It can be colored a warning color when the php version of the site is “old”?
#35
@
7 years ago
- Keywords has-patch has-unit-tests added
- Milestone changed from 4.9.5 to 5.0
- Owner changed from schlessera to flixos90
I'm not opposed to the alternative approach @nerrad suggests, but I think an admin notice is the most straightforward and on-point solution here. Per the discussion on Slack, let's rather aim for 5.0 instead of a minor release, as seeing the notice after a minor release may indeed feel weird. 41191.2.diff simply adjusts the version numbers accordingly.
Per the suggestion by @clorith, we should also consider using a warning color for the notice. While each of us probably considers a site running PHP 5.2 "broken", we have to be honest to ourselves: It is not broken. It is insecure and all sorts of other things, but I think a warning actually fits better.
Again, please continue further discussion [on GitHub](https://github.com/wp-core-php/wordpress-develop/pull/1).
This ticket was mentioned in Slack in #hosting-community by flixos90. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
7 years ago
#38
@
7 years ago
- Keywords 2nd-opinion added
In today's PHP chat, the idea of implementing the notice as a dashboard widget came up, as there is prior art for that with the Browsehappy notice.
I implemented an initial take on this alternative approach with 41191.3.diff (so it does not iterate on the previous patches here!). The original source of the patch is https://github.com/wp-core-php/wordpress-develop/pull/2, where further discussion can take place.
We should compare the two approaches and come to a decision next week which one to go with.
#39
@
7 years ago
As of last Thursday, the Serve Happy API is live on wordpress.org! See https://api.wordpress.org/core/serve-happy/1.0/?php_version=7.0 and https://meta.trac.wordpress.org/ticket/3474
41191.4.diff is an updated patch for the dashboard widget (which is the approach we'll be going for, per last week's PHP meeting), calling the new .org API to determine the status of the PHP version. It furthermore introduces a filter to allow tweaking the direct PHP update URL (which is optional, and an addition to the information page URL). This allows hosts to link to their own specific upgrade instructions.
With the API and the above patch in place, the core notice is almost ready to be merged. Remaining discussions:
- The API on wordpress.org supports an
ip_address
parameter which has to be a truncated IP address where the last segment is always 0 (for privacy reasons, example: 123.456.789.0). This can be used in the future to possibly automatically detect the host and thus return a properupdate_url
(which at this point is a supported return value already, but always an empty string). It would be easy to make core future-proof in that regard: The actual notice already takes that return value into account, now we only have to decide whether we should actually send the (truncated) IP address or not. - Should the color of the notice remain red (typically for errors) or be changed to orange (typically for warnings)?
Also still missing in the implementation is the change of setting the dashboard widget to appear at a high priority (no discussion needed, just implementation).
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
7 years ago
#41
@
7 years ago
Yesterday, the new API structure was committed (see https://meta.trac.wordpress.org/changeset/6806). 41191.5.diff takes that change into account.
#42
@
7 years ago
41191.6.diff changes the widget to be of a high
priority, as discussed.
Both the API and the patch are now in a solid state for some final testing. If no objections, I plan to commit this next week.
#44
@
7 years ago
- Keywords needs-design ui-feedback added
A couple of code notes:
dashboard.php:41
, remove the!
from the ternary operator and swap the strings.- For the
$update_url
if
statement, please don't introduce new instances ofif
/endif
notation.
The text in the widget is going to need rewriting to fit the usual voice in Core. It'd be worth having this reviewed by the Design and Marketing teams.
Also, the double button behaviour when $update_url
needs a review from the Design team. This is presumably so hosts can add a mu-plugin
that directs to their upgrade routines? Rather than confusing the issue with two buttons, it seems like this should just be a single button that sends you to the host's upgrade page. At the very least, I suspect the strings being a split sentence will cause translation issues.
#45
@
7 years ago
Minor: I'm pretty sure core doesn't use '
anywhere, I'd suggest for consistency to use a plain single quote or ’
.
Design / a11y:
If possible, the widget title should be a real title that indicates what the widget is about, not the text of a notice. This title is also used for the toggle button with feedback about the expanded / collapsed state and sounds a bit weird as it is now:
The big blue button looks like a very big button but it's actually a link, if possible it would be preferable to make it look like a link :)
#46
@
7 years ago
One bit of the patch is this:
<a class="notice-upgrade-button button button-primary button-hero" href="<?php echo esc_url( $information_url ); ?>"><?php _e( 'Learn more about upgrading PHP' ); ?></a> <a href="<?php echo esc_url( $update_url ); ?>"><?php _e( 'or upgrade right away' ); ?></a>
That's two consecutive links, which according to WCAG 1.0 at least, "Older screen readers read lists of consecutive links as one link."
This could be mitigated by making the "or" not linked, which would then also make the links more distinct for visual users as well.
And as mentioned, this split sentence will make translating less friendly.
This ticket was mentioned in Slack in #core-php by schlessera. View the logs.
7 years ago
This ticket was mentioned in Slack in #design by karmatosed. View the logs.
7 years ago
#49
@
7 years ago
We discussed this ticket in the design team tonight. We definitely agree on encouraging people to upgrade and we prefer the wording in WHIP core3. However, some questions arise, that we haven't got a final answer to. So maybe we can consider the following:
- The message can be intimidating for non-tech users (even after explaining)
- Maybe add a text that if they don't understand what this means, they contact their hosting or web developer (which leads to the following question):
- What if they don't have a web developer?
- Is it possible to show this notification to admins only (assuming that, in general, they may have more technical knowledge)
Love to hear how others feel about this.
#51
@
7 years ago
I love this project!
However, the tone of the third section is a bit stark and stern sounding. Specifically the line:
"If you follow the instructions we've provided to the letter, upgrading shouldn't take more than a few minutes, and is generally very safe to do."
I would suggest rewording this as:
"Upgrading shouldn't take more than a few minutes, and is generally very safe to do if you carefully follow our provided instructions."
#52
@
7 years ago
Hi Core,
Felix reached out to the Marketing Team.
I agree with @mcdwayne that the phrase "to the letter" has "a tone" as he puts it.
I'll be more bold and say it's condescending at best frustrating at worst. As a blogger first, not a Marketing Team Rep, I would have no idea what to do and even if I saw this, I would be frustrated.
I prefer the text in WHIP core2.png since PHP versioning is part server-side.
The admin notice is a good way for basic awareness. The user should ask their host. That communication from host customer to host should help with voluntary migration.
I love the landing page that it takes the user to and that text is informative and friendly.
Thanks for letting us take a look.
Kindly,
Bridget
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
7 years ago
#54
@
7 years ago
I disagree that the "tone" of the third section is condescending or harsh. The intent of the phrase "to the letter" to me is clear in articulating the need to follow instructions carefully. Since the phrase "to the letter" seems to be problematic, @mcdwayne 's suggestion could be used or you could simply replace "to the letter" with "carefully" so:
If you carefully follow the instructions we've provided...
#55
@
7 years ago
- Keywords needs-testing needs-design ui-feedback removed
41191.7.diff implements all remaining changes that were agreed on for the first iteration:
- Only a link to the education page is displayed. There is no longer a filter to add another environment-specific URL.
'
has been replaced with’
.- Ternary operator when creating the widget title has been tweaked accordingly.
- Markup and CSS have been slightly simplified.
- New capability has been added to capabilities test class.
With that, the patch fulfills the requirements for a first iteration. Possible improvements should be discussed in separate tickets.
#57
@
7 years ago
- Keywords needs-news-post added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening this for possible inclusion in a minor release before 5.0, and to flag that it requires a news entry.
All improvements, such as related to accessibility or wording, should however be handled through individual tickets.
#59
@
7 years ago
- Keywords has-patch has-unit-tests removed
The text in the widget is going to need rewriting to fit the usual voice in Core.
This hasn't been addressed. Please don't backport it until the copy has been rewritten.
#60
follow-up:
↓ 61
@
7 years ago
One question: what is the minimum php version accepted before show the notification?
recommended_version - php ?
is_supported - php ?
is_secure - php ?
is_acceptable - php ?
Thanks!
#61
in reply to:
↑ 60
;
follow-up:
↓ 62
@
7 years ago
Replying to Luciano Croce:
One question: what is the minimum php version accepted before show the notification?
See https://meta.trac.wordpress.org/ticket/3474 - specifically https://meta.trac.wordpress.org/browser/sites/trunk/api.wordpress.org/public_html/core/serve-happy/1.0/config.php
Currently it's 5.3
for ACCEPTABLE_PHP
, so this should only show for PHP 5.2 users. That value will change in the future.
#62
in reply to:
↑ 61
@
7 years ago
Replying to dd32:
Replying to Luciano Croce:
One question: what is the minimum php version accepted before show the notification?
See https://meta.trac.wordpress.org/ticket/3474 - specifically https://meta.trac.wordpress.org/browser/sites/trunk/api.wordpress.org/public_html/core/serve-happy/1.0/config.php
Currently it's5.3
forACCEPTABLE_PHP
, so this should only show for PHP 5.2 users. That value will change in the future.
Thanks!
This ticket was mentioned in Slack in #core by danieltj. View the logs.
7 years ago
#64
@
7 years ago
- Keywords needs-patch added; needs-news-post fixed-major removed
See comment:59. Ending the widget with "Good luck" is a bit harsh IMO.
This ticket was mentioned in Slack in #core-php by schlessera. View the logs.
7 years ago
#66
@
7 years ago
- Keywords has-patch added; needs-patch removed
I uploaded a patch (towards current trunk) that changes the copy to better fit the voice of WordPress and to modify the general feel of the widget.
We thought that it feels too much like a human interaction (written in the form of a letter, with a greeting and a complimentary clause). This might lead some users to the assumption that someone actively scanned their site and initiated this interaction. With the changes to the copy, this now feels more like a system check that has identified an issue.
This is what the patched version looks like (thanks to @xkon for providing the mockup):
A small note regarding the wording:
@charliespider and I couldn't agree on what form is better:
Upgrading usually takes only a few minutes
vs
Upgrading usually only takes a few minutes
I prefer the former but can quickly adapt the patch if more people prefer the latter.
#67
@
7 years ago
Quick note about headings: they shouldn't be used for visual purposes, just to have some bold "titles". Headings should identify section of content, and they should have some meaningful text for that purpose. They should be meaningful even when read out of context.
Of the three H3 used here, seems to me only the first one is used properly:
The other two ones:
"Okay, how do I update?"
and
"Thank you for taking the time to read this!"
don't identify what I'd call a "section of content". Most of all, they are meaningless when read out of context. Since it's a very quick change, I'd recommend to keep just the first H3 and use bold text for the other two sentences.
#68
@
7 years ago
@afercia :
The "Thank you for taking the time to read this!" was already removed with the last patch.
Regarding the second one - "Okay, how do I update?" - what if we rephrase it into "How can I upgrade my PHP version?" ? That gives it more meaning when read out of context.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-php by schlessera. View the logs.
7 years ago
#73
@
7 years ago
- Keywords needs-design added
The #core-php team thinks this is good to go in terms of matching the WP voice.
I'm flagging this with needs-design
to see whether the #design team can provide any additional input.
Note: in terms of design, it currently fits the rest of the dashboard, so the #design team is free to just check this off as "good enough for now".
#74
@
7 years ago
- Keywords ui-feedback ux-feedback added; needs-design removed
Oh, I think I used the wrong workflow label. We're actually looking for feedback here, so using the corresponding labels.
This ticket was mentioned in Slack in #design by joyously. View the logs.
7 years ago
#78
@
7 years ago
This is an improvement, but still has issues. I've asked the a8c editorial team to review the copy.
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
7 years ago
#82
@
7 years ago
- Keywords needs-testing added
In 41191.11.diff I've rewritten some of the wording in the dashboard widget. I feel like we don't need to over complicate the details here and is pretty well covered in the external page we link out to. I think it will make users happier if we get to the point and say; upgrade now (please).
Thoughts on this patch?
This ticket was mentioned in Slack in #design by boemedia. View the logs.
7 years ago
#84
@
7 years ago
In order to get you design feedback could we have a little video or screenshot of the latest patch please to allow all designers, even those that don't run patches to see? Also can we have screens of all the states, for example closed and open actions.
Once we have those, we can ensure you get feedback from the team. Thanks.
#85
@
7 years ago
In 41191.8.diff:
- Change the widget header to match the style of the other widgets.
- Reword the notice, props @benhuberman.
- Make the button open in a new tab.
I've also asked @benhuberman to edit the upgrade page.
@karmatosed: Here's a screenshot of the latest widget:
It doesn't have any special behaviour on smaller screens, so I've only included the one screenshot. 🙂
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
6 years ago
#88
@
6 years ago
I'm liking the choice of words a lot better. However I'm still uncomfortable with the last sentence:
The instructions we provide will guarantee a secure, painless update.
Are we really comfortable using the word "guarantee" here?
#89
@
6 years ago
Here's some suggestions for alternatives:
- The instructions we provide will help provide a secure, painless update.
- The instructions will help guide you through a secure, painless update.
- The instructions will help you with updating.
I personally prefer 3 because I think it doesnt' make any promises that might not match experience. Our instructions will help with updating plain and simple. We can't promise it will be a secure or (especially) painless update.
#90
@
6 years ago
I also approve of the changes in 41191.8.diff, particularly the removal of some redundant or less important parts for the site owner.
Regarding the suggestions @nerrad proposed above, I prefer option 1. I generally agree with him that we shouldn't use the word "guarantee" because we cannot guarantee a smooth update. But the upgrade page gives enough information about possible issues and how to deal with them, so I think replacing "guarantee" with "help" is accurate.
This ticket was mentioned in Slack in #design by karmatosed. View the logs.
6 years ago
#92
follow-up:
↓ 94
@
6 years ago
As has been asked, I am going to give a design review of this. I am coming a little into this without knowing the history (so please consider this).
- That is a lot of text and expecting people to read it is quite the ask.
- That button (blue primary one) should not be full width. Make it fit the text please.
- I am unsure this should be a red error, perhaps it's more of a warning?
#93
@
6 years ago
Regardless of the quality of the current version of the text, it would be great to think further about a much shorter copy for the PHP nag. Last week we briefly compared the nag to the Browsehappy nag and thought it may be worth considering a much shorter text (something like https://make.wordpress.org/core/2018/04/03/php-meeting-recap-april-2nd/ in the grey box). It's fast to read and immediately gets to the point and the problem. On the other hand, upgrading PHP (and even knowing what it is) is more involved than updating the browser, so it may need the additional content.
Concerns about the length of the nag were expressed in today's design triage meeting as well (as @karmatosed highlights above), so we should determine whether this is viable or whether we really need the additional text.
#94
in reply to:
↑ 92
@
6 years ago
Replying to karmatosed:
As has been asked, I am going to give a design review of this. I am coming a little into this without knowing the history (so please consider this).
- That is a lot of text and expecting people to read it is quite the ask. I think it should be collapsed by default or a lot less text.
- That button (blue primary one) should not be full width. Make it fit the text please.
- I am unsure this should be a red error, perhaps it's more of a warning?
Agree on these points here.
Button should be smaller (fit the text), the text reduced. I tried to reduce some of it in 41191.11.diff and the red should be changed to a orange (warning colour) as whilst it's not broken per se, PHP should be upgraded.
I am against having this collapsed by default though. If we have it collapsed be default it won't have the effect that we're intending it to have. It should be right in front of you to see it clearly.
#95
@
6 years ago
"the red should be changed to a orange (warning colour) as whilst it's not broken per se, PHP should be upgraded"
I would recommend adapting the colour to the PHP version:
- Red for old versions of PHP that are considered no longer acceptable, for example PHP 5.2.4+
- Orange for PHP versions considered acceptable, for example PHP 5.6+
#96
@
6 years ago
I agree with @Luciano Croce about the color and with @karmatosed that the UI should be collapsed by default.
@
6 years ago
Text reviewed by Automattic editorial team, removed word "guaranteed", replaced red bar with symbol
This ticket was mentioned in Slack in #core-php by nerrad. View the logs.
6 years ago
#98
@
6 years ago
- Adjusts the inline docs for
wp_check_php_version()
, as it can also returnfalse
on failure. - Handles that
phpversion()
can returnfalse
according to PHP docs. - Changes "opens in a new window" to "opens in a new tab" according to #43803.
- As 41191.9.diff ] adds
target="_blank"
, we shouldadd rel="noopener noreferrer"
according to #37941. - Adds a full-stop to the
// PHP Version
comment and the multiline comment inwp_check_php_version()
. - Adds strict 200 comparison in
wp_check_php_version()
.
There's this notice in wp_check_php_version()
:
/** * Response should be an array with: * 'recommended_version' - string - The PHP version recommended by WordPress * 'is_supported' - boolean - Whether the PHP version is actively supported * 'is_secure' - boolean - Whether the PHP version receives security updates * 'is_acceptable' - boolean - Whether the PHP version is still acceptable for WordPress */
there's an array check, but there are currently no array key checks. One way could be to validate the data before it's saved to the db.
That's something to consider, but 41191.10.diff further:
- Adds an isset check, before using
$response['is_acceptable']
inwp_dashboard_setup()
. - Adds an isset check, before using
$response['is_secure']
inwp_dashboard_php_nag()
.
ps: Sometimes I wished core would prefix it's own options data, like 'wp_php_check_' instead of 'php_check_', but that's not part of this patch though ;-)
#99
@
6 years ago
@birgire
Handles that phpversion() can return false according to PHP docs.
If I understand correctly, this only applies for querying extension version like phpversion( 'mysqli' );
.
#100
@
6 years ago
Return Values
If the optional extension parameter is specified,
phpversion()
returns the version of that extension,
or FALSE if there is no version information associated or the extension isn't enabled.
@schlessera aha you're probably correct, I read it in a way that "or FALSE if there is no version information associated
" would also apply without extension ;-)
#101
@
6 years ago
I do not prefer the most recent iterations of this feature.
The best one, IMO, was this one from 4 weeks ago:
https://core.trac.wordpress.org/attachment/ticket/41191/41191.rewording.2.png
Here are the problems I see with the current iteration. Sorry... here goes:
- The first sentence isn't actually true. Insecure software is not slow; secure software is not fast. Upgrading is not quick for most people. This is not an easy app update. This is a piece of system software on a server that most users do not have control over.
- PHP isn't the programming language we use to build or maintain WordPress; that's like saying English is the language we use to build and maintain books. "PHP is one of several programming languages WordPress uses to help 33% of the web share their thoughts online" or something like it, sounds uplifting without being inaccurate.
- Questions are bad when coupled with red warnings. "What is PHP?" and "How can I update?" sound like WordPress inserting a scary narrative into a user's mind. This is a time for confidence building, not for a quiz.
- The current verbiage reads, to me, like a spammy virus pop-up. I would think my WordPress had a virus, if I didn't know better.
- "Click the button below" is not good UX. Here's one link on the web explaining why, amongst the million: http://uxmovement.com/content/why-your-links-should-never-say-click-here/
- The left aligned ¾ width button looks misplaced. It has no home. Is that intentional?
- The button links to a page with 1924 words, and no clear steps to update PHP. We've built urgency with no immediate solution. We are guiding these users down a dead-end alleyway.
- The closing statement is not true. There is(was) a high likelihood of pain, which is why most folks are still running older versions. Not pain from WordPress itself, but plugins and other PHP-based applications running on the same server will bring breakage, and we are not preparing them for that here.
- That red bubble has to go. WordPress uses different red bubbles to communicate things like comments needing moderation, and plugins & themes needing updates. Notification bubbles are attached to trivial tasks that are easily completed. This red bubble isn't actually a bubble, and breaks the convention of meta-box titles only being words. Plugin authors will follow suit, resulting in abuse, losing its effectiveness.
- Using phases like "we use to build WordPress" and "the instructions we provide" are poor attempts to convey false trust, and break the 4th wall pretty hard. Who are "we" here? Users care about who "we" are as much as they care who their plumber is, especially when there's a crisis. And anyone that does know WordPress is open-source software developed by whomever shows up, knows this isn't a confidence builder; it's a warning.
Today, in Slack, folks mentioned this being "good enough" to ship, but (to me, it seems like) that's based on wanting to avoid more bike-shedding, and not based on the viability of (or actually liking) the current results. Not sure who to ping for a review/consult. I know @flixos90 is owning the ticket here, but the comments make it look like @pento and @karmatosed directed some changes from Automattic folks. Who's in charge of saying when this is done?
#102
@
6 years ago
Oh, just now noticed that my post following the upload was not actually uploaded.
Here goes again, just for future reference:
41191.9.diff follows on 41191.8.diff (which has the content approved by the Automattic editorial team), with the following changes:
- The red border is replaced with a red dashicon exclamation mark that adds "shape" to the color-only visual signal of the previous iteration. This also more closely aligns to the visual appearance of a widget, as the border turned it into a hybrid between a widget and a notice.
- The word "guaranteed" as been removed, and the text adapted accordingly.
- The button has been turned from full-width to just fit the text.
#103
@
6 years ago
Hacking do_meta_boxes()
to hardcode a dashboard_php_nag
check is proof this is the wrong approach.
There's no reason every meta-box call in WordPress needs to make that check.
#104
follow-up:
↓ 105
@
6 years ago
41191.11.2.diff takes the following approach:
- Strong and earnest opening statements - either they are on a version that is EOL, or on a version that's vulnerable
- Show the current and recommended PHP versions. This automatically triggers users to speculate how far behind they are. Also handy for communicating with hosts after clicking "Learn More".
- Prefer "Current" over "Newer" - guide users toward being up-to-date and current, without implying anything newer is better
- Close with confidence. Sell the benefits.
- Less-hugely button, because that big button looks awful
#105
in reply to:
↑ 104
@
6 years ago
Replying to johnjamesjacoby:
Less-hugely button, because that big button looks awful
"Learn more" doesn't make much sense when read out of context, e.g. by assistive technology tools. It doesn't seem any different from "click here" example in the article you've linked to:
You may have text around the link that explains what they’re clicking, but when users read the link itself they won’t have a clue. This means that users have to read the text all around the link to understand the context of the link. This impedes users from taking the quick and short route of clicking the link directly because they have to read the surrounding text first. If there’s a lot of text, this could slow users down a lot.
"Learn more about updating PHP" may look less pretty, but provides better context for accessibility.
#106
@
6 years ago
Given that requirement, none of the other buttons or links in core make any more sense than what I’m proposing. “Learn more” actually better matches the deeply ingrained general WordPress voice.
I also think we need to give screen-assisted users more credit, that they can infer what “Learn More” would be in context to, the same as they already must with “Trash”, “Save Post”, “Add New”, “Quick Draft” and others.
I’m not saying we should do bad because we’ve always done bad, but I do think we should stick to the style-guide here, and experiment elsewhere.
#107
@
6 years ago
@johnjamesjacoby
Given that requirement, none of the other buttons or links in core make any more sense than what I’m proposing. “Learn more” actually better matches the deeply ingrained general WordPress voice.
We can have the best out of both worlds by doing the following: __( 'Learn more<span class="screen-reader-text"> about updating PHP</span>' )
Hacking
do_meta_boxes()
to hardcode adashboard_php_nag
check is proof this is the wrong approach.
While it is terrible we need to hack to accomplish that, this indicates a lack of flexibility in the Meta Boxes API. The Browsehappy notice has been doing that as well. These two meta boxes are special, since they only appear conditionally to inform the user about significant issues with their setup - so I think it’s okay for them to have special handling.
I‘ll have more time to look at your suggestions and give feedback later.
#108
@
6 years ago
After recent discussions and the additional feedback and patch from @johnjamesjacoby, 41191.12.diff is an iteration on 41191.10.diff which takes a couple of suggestions (partly from 41191.11.2.diff) into account. Changes made:
- Fix incorrect statement "WordPress has detected that your site is running on an insecure version of PHP, which means that a faster website is just a quick update away." by removing the second part of that sentence. That furthermore makes it a more clear statement, and the aspect of speed is mentioned in the second paragraph anyway.
- Remove the whole last section that essentially just revolves around the user needing to click the button. It helps further shortening the copy, as requested by the design team, and was redundant.
- Left-align button and make it regular-size, as requested by the design team and in 41191.11.2.diff. @johnjamesjacoby We cannot shorten the button text itself, as "Learn more" makes it too unclear this link helps with updating PHP.
- If the PHP version is not insecure, but only out-of-date, show the icon with yellow color, as requested by the design team. This change is to be future-proof, but note that at this point, it will never be rendered this way because we show the notice only on 5.2 which is insecure.
#109
@
6 years ago
@flixos90 @SergeyBiryukov
We can have the best out of both worlds...
That's not a bad idea. I don't like having HTML in the string, but that's not new.
In retrospect, this shouldn't even be styled as a button; it doesn't submit anything, and it's an external link. What if we take inspiration from the Events and News meta-box and make it a link at the very bottom?
All of the other "Learn more about..." links in core are not buttons.
While it is terrible we need to hack to accomplish that, this indicates a lack of flexibility in the Meta Boxes API.
These are the kind of code compromises we make to support backwards compatibility, not introduce a new feature.
No. The meta-box API intentionally prevents unique boxes because they break the promise we make to users about how meta-boxes work. The reason BrowseHappy & ServeHappy need special casing is because they're ignoring that promise. If a plugin asked for special casing like this, we'd be trying to convince them why it's not necessary.
If core needs unique meta-box types, then we should build that before this. There are 3 right now in core alone: Welcome, BrowseHappy, and this one. That feature is literally a separate issue from this one.
#112
@
6 years ago
Stretch goals, or things left to do:
- Unhack
do_meta_boxes()
to remove the hard-coded check for this single meta-box. There's no reason for every unrelated meta-box call to need to perform this check. Remove the BrowseHappy hack also. - Reconsider not using button styling. It isn't submitting anything. It's an external link; it should look like a link.
- Reconsider use of dashicon in title. Plugins _are_ going to copy this.
#113
@
6 years ago
@johnjamesjacoby I like some of these ideas. :)
Can you create tickets for those items and refer to this one here as necessary? Let's proceed through individual iterations from now on, as this ticket has gotten way too big.
This ticket was mentioned in Slack in #hosting-community by mike. View the logs.
6 years ago
#115
@
6 years ago
- Milestone changed from 4.9.6 to 5.0
Moving back to the 5.0 milestone since this was not backported.
This ticket was mentioned in Slack in #core-php by flixos90. View the logs.
6 years ago
This ticket was mentioned in Slack in #core-php by sergey. View the logs.
6 years ago
#118
@
6 years ago
- Milestone changed from 5.0 to 5.1
- Resolution fixed deleted
- Status changed from closed to reopened
Version numbers need to be adjusted to 5.1.0.
#119
@
6 years ago
- Resolution set to fixed
- Status changed from reopened to closed
Further work (including version number change) will be handled in #45686.
Here's an idea for how we could present this notice. I've adapted the text from the Yoast WHIP notice for the Wordpress audience, and styled it like the Welcome widget on the WP dashboard. This could be the 'worst version' of the notice for people on 5.2, and it could get progressively more benign the closer to PHP7 someone is.