WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 12 months ago

Last modified 9 months 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 13 months ago.
46599.2.diff (4.3 KB) - added by SergeyBiryukov 13 months ago.
46599.3.diff (6.5 KB) - added by afragen 12 months 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 12 months ago.
simplify $tested_wp assignment
46599.5.diff (7.0 KB) - added by afragen 12 months ago.
function renaming to is_wp_version_compatible() and is_php_version_compatible()

Download all attachments as: .zip

Change History (20)

@afragen
13 months ago

#1 @SergeyBiryukov
13 months ago

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

#2 @SergeyBiryukov
13 months 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
13 months 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
13 months 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 be unable to activate. 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.

Last edited 13 months ago by afragen (previous) (diff)

#5 @afragen
13 months 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 13 months ago by afragen (previous) (diff)

@afragen
12 months ago

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

#6 @afragen
12 months 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 12 months ago by afragen (previous) (diff)

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


12 months ago

@afragen
12 months ago

simplify $tested_wp assignment

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


12 months ago

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


12 months ago

#10 @JeffPaul
12 months 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
12 months ago

How about is_wp_version_compatible() and is_php_version_compatible()?

@afragen
12 months ago

function renaming to is_wp_version_compatible() and is_php_version_compatible()

#12 @afragen
12 months ago

Renamed functions as above with input from @swissspidy

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


12 months ago

#14 @SergeyBiryukov
12 months 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
9 months ago

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