#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)
Change History (20)
#1
@
5 years ago
- Milestone changed from Awaiting Review to 5.2
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#4
@
5 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 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.
#5
@
5 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
.
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.
@
5 years ago
add isset()
test independent of and before calling is_wp_compatible()
or is_php_compatible()
#6
@
5 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
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 by audrasjb. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
5 years ago
#10
@
5 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.
We should keep the
empty()
checks, as plugin API requests can be overridden, and the responses may not contain therequires
orrequires_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 forversion_compare()
and don't offer any significant benefit or code reduction.The naming of
wp_is_wp_compatible()
andwp_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.