Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#46599 closed defect (bug) (fixed)

Replace compatibility tests with new functions

Reported by: afragen Owned by: SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Site Health Keywords: servehappy has-patch needs-testing
Focuses: Cc:

Description

r44978 provides for new compatibility functions wp_is_wp_compatible() and wp_is_php_compatible().

Let's use those functions in all the places we needed them before they were in core.

Attachments (5)

46599.diff (4.5 KB) - added by afragen 3 years ago.
46599.2.diff (4.3 KB) - added by SergeyBiryukov 3 years ago.
46599.3.diff (6.5 KB) - added by afragen 3 years ago.
add isset() test independent of and before calling is_wp_compatible() or is_php_compatible()
46599.4.diff (6.7 KB) - added by afragen 3 years ago.
simplify $tested_wp assignment
46599.5.diff (7.0 KB) - added by afragen 3 years ago.
function renaming to is_wp_version_compatible() and is_php_version_compatible()

Download all attachments as: .zip

Change History (20)

@afragen
3 years ago

#1 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.2
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#2 @SergeyBiryukov
3 years ago

We should keep the empty() checks, as plugin API requests can be overridden, and the responses may not contain the requires or requires_php field, triggering PHP notices. See #31694, #33330, or #44582 for an example of similar notices.

46599.2.diff is a patch with empty() checks. However, at this point, the new functions just become an alias for version_compare() and don't offer any significant benefit or code reduction.

The naming of wp_is_wp_compatible() and wp_is_php_compatible() (as suggested in comment:41:ticket:43992) also sounds a bit weird.

I'd think we should pull them for now, and discuss better naming and whether we actually need them.

#3 @SergeyBiryukov
3 years ago

In 44981:

Plugins: Remove wp_is_wp_compatible() and wp_is_php_compatible() functions added in [44978] for now, to discuss use cases and better naming.

See #46599, #43992.

#4 @afragen
3 years ago

If we wish to keep the empty checks I would propose adding an additional test to ensure the variable is set to something. The empty test in the original was to allow for a plugin that didn’t have a header set to be valid.

It appears that this commit will stop any plugin that doesn’t have one of the readme.txt headers defined to fail. I’m not at my computer today so I can’t test. I would much rather see this reverted and figure out a better name in beta.

Version 0, edited 3 years ago by afragen (next)

#5 @afragen
3 years ago

I initially wanted to use is_wp_compatible() and is_php_compatible().

Similar code exists in most plugins that are designed to fail gracefully. Having this in core means not having to fiddle with the subtleties of version_compare for many plugin authors.

Specifically having this test in a single location in core means that when having values in the readme.txt headers is mandatory it will be easier to implement. Currently the empty( $requires ) || version_compare… allows for this less stringent test.

I think we could add a single line of code to ensure that whatever variable we pass to this function doesn’t generate a PHP notice for potentially being undefined.

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

@afragen
3 years ago

add isset() test independent of and before calling is_wp_compatible() or is_php_compatible()

#6 @afragen
3 years ago

I still think it's beneficial to have a core function that tests for WordPress and PHP version compatibility by passing it a variable. It saves on determining the subtleties of correctly using version_compare. Additionally it allows for a single location to allow for passing through empty variables as passing or not. Eventually core will strictly enforce the requirement of including the Require as least and Requires PHP headers in a readme.txt.

46599.3.diff tests for the existence of the passed parameter before passing it instead of having to include the empty( $plugin['requires_php'] ) || version_compare...

The use case for the function in core is it's utility in any plugin that is designed and tests to fail gracefully. I realize this shouldn't be an issue for plugins in the directory but it could tremendously assist those plugins that are hosted elsewhere.

If we agree the functions should exist, we can certainly discuss the naming.

See #43992

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

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


3 years ago

@afragen
3 years ago

simplify $tested_wp assignment

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


3 years ago

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


3 years ago

#10 @JeffPaul
3 years ago

Input from @swissspidy via today's bugscrub is that the functions need better names, at least having "version" in the function name would be an improvement.

#11 @afragen
3 years ago

How about is_wp_version_compatible() and is_php_version_compatible()?

@afragen
3 years ago

function renaming to is_wp_version_compatible() and is_php_version_compatible()

#12 @afragen
3 years ago

Renamed functions as above with input from @swissspidy

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


3 years ago

#14 @SergeyBiryukov
3 years ago

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

In 45185:

Plugins: Introduce is_wp_version_compatible() and is_php_version_compatible() for checking compatibility with the current WordPress or PHP version.

Props afragen.
Fixes #46599.

#15 @spacedmonkey
3 years ago

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