Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#44350 closed task (blessed) (fixed)

Block plugin updates if required PHP version is not supported - Updates screen

Reported by: flixos90's profile flixos90 Owned by: afragen's profile afragen
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Site Health Keywords: servehappy has-patch commit
Focuses: Cc:

Description

Note: This ticket is a subtask for the overarching #40934 ticket.

When a plugin states it requires a specific minimum PHP version through its "Requires PHP" header information and the server does not support this PHP version, WordPress should block any possibility to update the plugin. This way, plugins will stay at the latest release that still supports the server's PHP version.

This ticket's goal is to prevent plugin updates from the general "Updates" admin screen. With that, it complements #43987, which deals with preventing plugins from the "Plugins" admin screen.

Attachments (18)

44350.diff (1.7 KB) - added by afragen 6 years ago.
For update-core.php
screenshot_06.png (62.3 KB) - added by afragen 6 years ago.
Checkbox gone and notice listed.
44350.2.diff (2.6 KB) - added by afragen 6 years ago.
feedback applied
screenshot_04.png (56.3 KB) - added by afragen 6 years ago.
44350.3.diff (2.6 KB) - added by afragen 6 years ago.
changed messaging text from 'upgrading' to 'updating'
44350.4.diff (2.7 KB) - added by miyauchi 6 years ago.
Improve escaping
44350.5.diff (2.7 KB) - added by afragen 6 years ago.
more escaping improvements
44350.6.diff (2.7 KB) - added by afragen 6 years ago.
fixed some escaping for esc_html not esc_attr
44350.7.diff (2.6 KB) - added by afragen 5 years ago.
Updated patch for post 5.0 merge, fixed link to update-php
44350.8.diff (5.0 KB) - added by afragen 5 years ago.
fix version_compare so 5.1.10 is greater than 5.1.2
44350.9.diff (4.7 KB) - added by afragen 5 years ago.
update patch for new functions wp_get_update_php_url() and wp_update_php_annotation()
44350.10.diff (2.3 KB) - added by afragen 5 years ago.
updated patch. This will require #46044 to be committed first as it uses new function from that ticket
44350.11.diff (2.3 KB) - added by afragen 5 years ago.
44350.12.diff (2.3 KB) - added by afragen 5 years ago.
close then open p tag
44350.13.diff (1.8 KB) - added by desrosj 5 years ago.
44350.14.diff (2.3 KB) - added by afragen 5 years ago.
change to <br><em>....</em>
incompatible-update-filtered-links.png (319.9 KB) - added by desrosj 5 years ago.
incompatible-update-default.png (296.3 KB) - added by desrosj 5 years ago.

Download all attachments as: .zip

Change History (53)

This ticket was mentioned in Slack in #core-php by flixos90. View the logs.


6 years ago

#2 @flixos90
6 years ago

  • Owner set to afragen
  • Status changed from new to assigned

@afragen
6 years ago

For update-core.php

@afragen
6 years ago

Checkbox gone and notice listed.

#3 @afragen
6 years ago

44350.diff for removing ability to update from update-core.php page.

The following screenshot shows the checkbox missing and therefore not possible to update along with a notice regarding PHP updating.

Checkbox gone and notice listed.

#4 @afragen
6 years ago

  • Keywords dev-feedback has-patch added; needs-patch removed

Are we also looking to disable updates if the WP version is not supported?

#5 @afragen
6 years ago

The last discussion on Slack was to change the text to

This plugin doesn’t work with your version of PHP. [Learn more about updating PHP]

I’ll update the patch accordingly unless there are other suggestions.

@schlessera @flixos90 @SergeyBiryukov any other thoughts?

@afragen
6 years ago

feedback applied

@afragen
6 years ago

#6 @afragen
6 years ago

Updated patch 44350.2.diff with feedback incorporated.


Not much difference except for updating -> upgrading

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by afragen. View the logs.


6 years ago

@afragen
6 years ago

changed messaging text from 'upgrading' to 'updating'

#9 @afragen
6 years ago

These patches are dependent upon some changes in the patch for #43987 for the View Details window to display a Cannot Update button.

This ticket was mentioned in Slack in #core-php by sergey. View the logs.


6 years ago

@miyauchi
6 years ago

Improve escaping

#11 @afragen
6 years ago

Translatable strings in core don’t generally seem to be escaped. Just my observation as core translations are more thoroughly vetted.

#12 follow-up: @miyauchi
6 years ago

Thanks @afragen

I thought properties of the $plugin_data like $plugin_data->Name should be escaped. :)

#13 @afragen
6 years ago

Core seems to be a slightly different case and I don’t honestly know the answer. I’m certain at some point we’ll get some feedback from @flixos90 or @SergeyBiryukov

Though more likely it’s just something I missed and thanks for picking up on it.

Last edited 6 years ago by afragen (previous) (diff)

#14 in reply to: ↑ 12 @afragen
6 years ago

Replying to miyauchi:

Thanks @afragen

I thought properties of the $plugin_data like $plugin_data->Name should be escaped. :)

If you want to escape the properties, you might use something like esc_attr( $plugin_data )

@afragen
6 years ago

more escaping improvements

#15 @miyauchi
6 years ago

Hi @afragen

esc_attr() should be used to escape attributes for HTML.
In this case, I think it should esc_html().

But I agree to escape the property only as your patch. :)

Thanks. :)

@afragen
6 years ago

fixed some escaping for esc_html not esc_attr

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


6 years ago

#18 @afragen
6 years ago

@schlessera @flixos90 should this be milestone 4.9.9?

#19 @afragen
5 years ago

Probably need to milestone this for 5.1

#20 @flixos90
5 years ago

  • Milestone changed from 5.0 to 5.1

@afragen
5 years ago

Updated patch for post 5.0 merge, fixed link to update-php

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


5 years ago

@afragen
5 years ago

fix version_compare so 5.1.10 is greater than 5.1.2

#23 @afragen
5 years ago

There is clearly overlap with 43987 and this patch. Also, may need to lint the patch. Sorry. Not sure everyone wants to see yet another iteration.

#24 @afragen
5 years ago

We probably need to change milestone to 5.2

Last edited 5 years ago by afragen (previous) (diff)

@afragen
5 years ago

update patch for new functions wp_get_update_php_url() and wp_update_php_annotation()

#25 @afragen
5 years ago

@flixos90 I need to revise the wp_update_php_annotation() to wp_get_update_php_annotation() and update this patch too. I found that I really do need to return the annotation and not simply echo it for this patch to work.

I'll make a new ticket for the function rename. #46044

Last edited 5 years ago by afragen (previous) (diff)

#26 @flixos90
5 years ago

  • Milestone changed from 5.1 to 5.2

@afragen
5 years ago

updated patch. This will require #46044 to be committed first as it uses new function from that ticket

#27 follow-up: @TimothyBlynJacobs
5 years ago

Should this use the compatibility functions from #43992?

I don't see why escaping the plugin name is required here. It isn't done elsewhere in core. The Version also isn't escaped, see https://github.com/WordPress/wordpress-develop/blob/6d2f78d9bacf931fb9d4ba031e135c4eb5b17713/src/wp-admin/includes/class-wp-plugins-list-table.php#L802. And the new_version isn't either, see in that same file when building the view details link.

I think a committer needs to look at that.

If we do want escaping, then I believe, those should be esc_html. esc_attr is used elsewhere in that file because the final destination is in href tags.

#28 in reply to: ↑ 27 @afragen
5 years ago

I was planning on a new ticket/patch for utilizing the new is_wp_compatible() and is_php_compatible() later. I didn't want to include them here as I was concerned someone testing the patch would encounter an error that they couldn't easily explain.

As for escaping, I was simply trying to escape before output. If these don't need escaping; I agree that's something for a core-committer to decide.

Replying to TimothyBlynJacobs:

Should this use the compatibility functions from #43992?

I don't see why escaping the plugin name is required here. It isn't done elsewhere in core. The Version also isn't escaped, see https://github.com/WordPress/wordpress-develop/blob/6d2f78d9bacf931fb9d4ba031e135c4eb5b17713/src/wp-admin/includes/class-wp-plugins-list-table.php#L802. And the new_version isn't either, see in that same file when building the view details link.

I think a committer needs to look at that.

If we do want escaping, then I believe, those should be esc_html. esc_attr is used elsewhere in that file because the final destination is in href tags.

@afragen
5 years ago

#29 @desrosj
5 years ago

#46269 was marked as a duplicate.

@afragen
5 years ago

close then open p tag

@desrosj
5 years ago

#30 @desrosj
5 years ago

  • Keywords needs-testing added; dev-feedback removed

In 44350.13.diff:

  • It seems like the closing <p> is not needed when displaying the annotation here. Also, the .description class was causing light gray text on white, which was not good for a11y. Changing to a <span class="description"> fixes the mismatched HTML and the contrast issues.
  • I removed the added output escaping. The $plugin_data variable is populated by _get_plugin_data_markup_translate(), which passes each value through wp_kses() with a list of allowed tags for that attribute.

#31 @afragen
5 years ago

@desrosj I thought we were changing the class=“description” to an em?

@afragen
5 years ago

change to <br><em>....</em>

#32 @desrosj
5 years ago

  • Keywords commit added

Thanks, @afragen. I was tinkering with the markup a bit on that and included the wrong change.

I attached some screenshots to show what the notices look like now with the final patch.

Also, it looks like one of the escaping calls were re-added in 44350.14.diff (I can remove this before committing). But, otherwise it looks good.

#33 @desrosj
5 years ago

  • Keywords needs-unit-tests needs-testing removed

#34 @desrosj
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 44939:

Plugins: Block plugin updates on the update screen if required PHP version is not supported.

This brings the changes in [44937] to the WordPress Updates page in the admin. Now, when a site does not meet the minimum PHP version requirements for a plugin update, the user will not be able to update. Instead, they will be presented with educational information to guide them through the process of updating PHP.

Props afragen, miyauchi, TimothyBlynJacobs, desrosj.
Fixes #44350.

#35 @spacedmonkey
5 years ago

  • Component changed from Plugins to Site Health
Note: See TracTickets for help on using tickets.