Making WordPress.org

Opened 7 years ago

Closed 7 years ago

Last modified 14 months ago

#3474 closed task (blessed) (fixed)

Introduce `serve-happy` API endpoint

Reported by: flixos90's profile flixos90 Owned by: dd32's profile dd32
Milestone: Priority: normal
Component: API Keywords: has-patch has-unit-tests
Cc:

Description

Per discussion in [this week's PHP meeting](https://wordpress.slack.com/archives/C60K3MP2Q/p1519056088000267), we would like to implement a basic endpoint for PHP version information handling, particularly to be used by core to check whether to show the PHP upgrade nag (see https://core.trac.wordpress.org/ticket/41191).

The endpoint should live at api.wordpress.org/core/serve-happy/ and do the following:

  • Return true/false for whether the passed PHP version needs to be upgraded.
  • Return true/false for whether the passed PHP version is insecure (no longer receives security updates).
  • Display relevant PHP versions on which the above "decisions" are based, plus WordPress' recommended PHP version.
  • Maybe also the latest PHP version available?
  • Maybe also the URL to the Servehappy education page? This would allow us to tweak it more flexibly and independently from core updates, just like the version numbers.

Attachments (2)

3474.diff (23.4 KB) - added by flixos90 7 years ago.
3474.2.diff (5.8 KB) - added by flixos90 7 years ago.

Download all attachments as: .zip

Change History (16)

@flixos90
7 years ago

#1 @flixos90
7 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

3474.diff introduces the serve-happy API endpoint. Details:

  • The index.php file procedurally executes the code, but all implementation details are part of separate classes.
  • All changes related to regular maintenance will occur in the separate config.php file (it holds the PHP version numbers, the Servehappy page URL etc.).
  • The code requires at least PHP 7 to run (which is fine here, per Slack discussion).
  • The main class handling a request and serving the appropriate response is Serve_Happy\API. The other classes are simple utility classes representing request and response.
  • The whole implementation has been inspired by how the WP REST API works, with several simplifications where the original flexibility would be overkill here.
  • Tests are also part of the patch, providing solid coverage.

The main discussion points should probably be: Do we need all the configuration/response data that is returned by this implementation? Do we need more? Keep in mind that some of this may be useful in the future (such as the upgrade_url, which could lead to a host-specific upgrade URL), and we may still wanna include it already to have a predefined pattern that we could implement in core already today (in core we could check if that upgrade URL is not empty, and if so, display a link to it).

Last edited 7 years ago by flixos90 (previous) (diff)

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


7 years ago

#3 follow-up: @dd32
7 years ago

  • Owner set to dd32
  • Status changed from new to accepted

I'll take a deeper look into this tomorrow, however, as a few initial notes:

  • Returning latest_php probably isn't going to be useful to the implementation, nor actually be updated as often as PHP releases are made, so it's probably worth dropping that field entirely. BrowseHappy the website IIRC pulls the data from another source, which I don't think we'd want to implement in an API.
  • Can we cache the API endpoint responses based on the REQUEST_URI alone? (If possible, reliance upon $_POST and $_SERVER vars means caching isn't really viable) At present it appears as such.
  • How is it planned for the upgrade_url parameter passed to be host-specific? What data would be used?
  • Including the URL to https://wordpress.org/support/upgrade-php/ seems like a worthwhile idea at first, although having it hard-coded in core would allow for locales to translate it into their own languages - which is far more important than it being included in the API.

To clarify:

Return true/false for whether the passed PHP version needs to be upgraded.

The 'upgrade' flag isn't whether it needs upgrading, but if WordPress.org believes it's out-of-date. Calling it upgrade seems like a misnomer, It's not "Oh you're running 7.0.1, so you need to upgrade to 7.0.27".

Two options I thought of while looking at it:

  1. Combine to single field, status => out_of_date, no_security_updates, ok, latest (ok is a bit of a random 'status' here, latest could be combined into it too)
  2. Rename to more verbose flags such as (for PHP 5.2) out_of_date => true & receiving_security_updates => false

I think we're okay to assume that PHP will retain it's supported versions security deprecation schedule for 7.x, but I question if it'll stay that way come 8.0, having a insecure_php <= 5.6 might not work too good in a far distant future, one we can cross when we get there (and lets ignore that Dec 3rd -> 31st window where 5.6 gets updates, but 7.0 doesn't :) )

Last edited 7 years ago by dd32 (previous) (diff)

#4 in reply to: ↑ 3 @flixos90
7 years ago

Replying to dd32:

Returning latest_php probably isn't going to be useful to the implementation, nor actually be updated as often as PHP releases are made, so it's probably worth dropping that field entirely. BrowseHappy the website IIRC pulls the data from another source, which I don't think we'd want to implement in an API.

Agreed, I can see the maintenance cost for that being too high for now, especially since it's not absolutely necessary.

Can we cache the API endpoint responses based on the REQUEST_URI alone? (If possible, reliance upon $_POST and $_SERVER vars means caching isn't really viable) At present it appears as such.

Right now, the code uses $_REQUEST to get the parameters passed (php_version). WordPress typically has used POST requests for all the APIs afaik, but we could also use GET requests for this one instead, like api.wordpress.org/core/serve-happy/1.0/?php_version=5.3. Should we change the code so that it checks $_GET instead of $_REQUEST? Regarding $_SERVER, that is only used to detect the protocol.

How is it planned for the upgrade_url parameter passed to be host-specific? What data would be used?

This is currently unknown. Maybe the IP address could be used. I thought it may be future-proof to include this field and also support it in core. This would mean whenever we get there, we don't need to wait for a core update. But since none of us has concrete ideas yet, we may also get rid of it.

Including the URL to https://wordpress.org/support/upgrade-php/ seems like a worthwhile idea at first, although having it hard-coded in core would allow for locales to translate it into their own languages - which is far more important than it being included in the API.

I thought about this too. I think we could support a locale parameter being passed (in the same way as the php_version parameter). Then this could return the correct URL. There's benefits and downsides to the API approach: The advantage would be that we can simply change the URL if it changes (which it may in the future, per recent discussions). The disadvantage is that it's probably more time-consuming to change URLs for locales in the codebase than in a translation string.

The 'upgrade' flag isn't whether it needs upgrading, but if WordPress.org believes it's out-of-date. Calling it upgrade seems like a misnomer, It's not "Oh you're running 7.0.1, so you need to upgrade to 7.0.27".
Two options I thought of while looking at it:

  1. Combine to single field, status => out_of_date, no_security_updates, ok, latest (ok is a bit of a random 'status' here, latest could be combined into it too)
  2. Rename to more verbose flags such as (for PHP 5.2) out_of_date => true & receiving_security_updates => false

That makes sense. I prefer the first approach, with a single status field. I think we don't need latest, particularly as it would require us to keep up with the latest PHP version available (see above). Maybe there could be another status like up_to_date for when the version is >= WordPress' recommended PHP version (that name is probably not good though). That would leave us with 4 statuses. But I don't have a strong opinion on this, we could also just have three (only ok as "good" status).

I think we're okay to assume that PHP will retain it's supported versions security deprecation schedule for 7.x, but I question if it'll stay that way come 8.0, having a insecure_php <= 5.6 might not work too good in a far distant future, one we can cross when we get there (and lets ignore that Dec 3rd -> 31st window where 5.6 gets updates, but 7.0 doesn't :) )

Good point. I'm not sure whether we should change behavior for that, but we could consider not including these version numbers in the API responses. It would however be less transparent to the client then.

#5 @dd32
7 years ago

In 6728:

API: Add a /core/serve-happy/ API endpoint for the Serve Happy project.

Props flixos90 for the API direction.
See #3474.

#6 @flixos90
7 years ago

@dd32 Should this be closed now that the API is live?

#7 @dd32
7 years ago

@flixos90 I left this open incase the initial API had any issues or missing stuff :)

If you can confirm it looks good, fine to close it.

I've left the "out_of_date", "receiving_security_updates" fields in there along side the status field for debugging purposes - If you can confirm if you'd like to keep those or remove them or whatever that'd be good.

#8 @flixos90
7 years ago

After writing code for https://core.trac.wordpress.org/attachment/ticket/41191/41191.4.diff, I think it's actually better to continue relying on the individual parameters, instead of status, as for our specific case at this point in core, we need to look at both things individually (out of date and insecure). So the status property could as well be removed.

The other minor thing that could be improved is: I suggest changing receiving_security_updates to something like insecure so that it aligns with the "negative" connotation of out_of_date, for better consistency. That means if any of the two values is true, you have something to improve. :)

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


7 years ago

#10 @flixos90
7 years ago

Per today's PHP meeting, we decided to go with the following approach for the API endpoint:

  • Only return the WordPress recommended PHP version as an actual number, in a recommended_version field.
  • Return the following boolean flags (based on the passed PHP version):
    • is_supported: Whether the PHP version is actively supported. Currently that means anything >= 7.0.
    • is_secure: Whether the PHP version receives security updates. Currently that means anything >= 5.6.
    • is_acceptable: Whether the PHP version is still acceptable by WordPress (meaning no notice needs to show in core). Currently that means anything >= 5.3.
  • The whole IP address-related functionality should be removed. It's unknown whether it will ever be needed and may also have privacy implications not worth thinking about at this point. Handling host-specific update URLs should rather be possible via core functionality.

To summarize, every API response should only consist of the four fields explained above.

@flixos90
7 years ago

#11 @flixos90
7 years ago

3474.2.diff implements the changes requested above, including modified tests to account for this.

With that approach in place, the API v1 can be considered final.

#12 @dd32
7 years ago

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

In 6806:

API: Serve Happy: Update the API to return the final selection of fields.

Props flixos90.
Fixes #3474.

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


5 years ago

This ticket was mentioned in Slack in #meta by starbuck. View the logs.


14 months ago

Note: See TracTickets for help on using tickets.