WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 2 months ago

Last modified 7 weeks ago

#39854 closed enhancement (fixed)

REST API: Add gmt_offset to base /wp-json response

Reported by: jnylen0 Owned by: jnylen0
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests fixed-major
Focuses: rest-api Cc:

Description

API clients often have a need to retrieve information about the current site. This doesn't fit into the wp/v2/settings endpoint for a couple of reasons:

  • That endpoint requires administrative privileges (current_user_can( 'manage_options' )). See also #38731.
  • We decided not to implement finer-grained permissions on the settings endpoint, as a read-only piece of information isn't a read/write "setting". See also ticket:38490#comment:10.

The most immediately obvious piece of information to add is gmt_offset. This is a crucial piece of information for any clients that need to manipulate datetimes returned via the API, but it's not currently available anywhere. See also ticket:38342#comment:48 for the specific case of Quick Draft inside wp-admin.

I expect we will need to enhance the base /wp-json response to accept a ?context=edit or similar parameter - so let's use this ticket for both the approach to /wp-json and the addition of the gmt_offset field specifically.

Attachments (6)

39854.patch (666 bytes) - added by sagarkbhatt 3 months ago.
Added gmt_offset in default wp-json endpoint.
39854.2.patch (1.3 KB) - added by sagarkbhatt 3 months ago.
This patch wil show gmt_offset only when context=edit
39854.3.patch (652 bytes) - added by sagarkbhatt 3 months ago.
Updated patch according to discussion.
39854.4.patch (1.3 KB) - added by sagarkbhatt 3 months ago.
39854.5.patch (2.0 KB) - added by sagarkbhatt 3 months ago.
Added unit tests
39854.6.diff (2.5 KB) - added by jnylen0 3 months ago.
Also update wp-api-generated.js fixture file

Download all attachments as: .zip

Change History (30)

#1 follow-up: @swissspidy
4 months ago

Why only gmt_offset and not timezone_string as well?

Am I understanding this correctly: this ticket wants to add this to /wp-json/, while #39072 wants to add the setting to the settings endpoint?

#2 in reply to: ↑ 1 @jnylen0
4 months ago

Replying to swissspidy:

Why only gmt_offset and not timezone_string as well?

gmt_offset is more immediately useful to clients because it's an easily parseable number and timezone_offset isn't always set, but we could certainly do both.

Am I understanding this correctly: this ticket wants to add this to /wp-json/, while #39072 wants to add the setting to the settings endpoint?

Yes - currently the settings endpoint is admin-only, so if we stick with that decision then there is value in having it in both places. See also #38731.

@sagarkbhatt
3 months ago

Added gmt_offset in default wp-json endpoint.

#3 follow-up: @jnylen0
3 months ago

Hi @sagarkbhatt! Thanks for uploading a patch. However, I think we need to only show this information for an authenticated request with ?context=edit.

We also need to make sure that gmt_offset returns an appropriate value even if the timezone_string is set instead, to e.g. America/New_York. These two options behave a bit strangely together.

Finally we'll want unit tests for each of these cases - https://make.wordpress.org/core/handbook/testing/automated-testing/phpunit/

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


3 months ago

@sagarkbhatt
3 months ago

This patch wil show gmt_offset only when context=edit

#5 @jnylen0
3 months ago

  • Keywords has-patch needs-unit-tests needs-refresh dev-feedback added
  • Owner set to jnylen0
  • Status changed from new to accepted

@sagarkbhatt thanks for working on this, though I think it is going to take a couple more iterations to get it finished.

Currently, I can visit e.g. https://nylen.io/wp-dev/wp-json?context=edit without authentication. Your latest patch would change that request to return an error, which I don't think is a good idea.

One way to work around this would be to just add the additional arguments if the request is correctly authenticated. This is backwards-compatible; however, it isn't very discoverable. Marking as dev-feedback to get other contributors' opinions here.

current_user_can( 'edit_posts' ) looks like a reasonable authentication check here, though I think simply is_user_logged_in() would also work.

I don't think wp_parse_args is the right function to use here, and we want to make sure the code reads well even after other values are added. Maybe something like this would work:

if ( 'edit' === $request['context'] ) {
    if ( is_user_logged_in() ) {
        $available['gmt_offset'] = get_option( 'gmt_offset' );
    }
}

Finally, as noted before, we need unit tests for this change. I can think of several cases to test:

  • Unauthenticated request: gmt_offset does not appear in the response.
  • Request with insufficient authentication: gmt_offset does not appear in the response.
  • Authenticated request: gmt_offset does appear in the response.
  • gmt_offset should always be a number, regardless of whether it is set or the timezone_string is set instead. (I don't recall exactly how this works, maybe we don't have to do anything special here.)

This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.


3 months ago

@sagarkbhatt
3 months ago

Updated patch according to discussion.

#7 @sagarkbhatt
3 months ago

gmt_offset should return int value regardless setting of timezone_string.

#8 @rmccue
3 months ago

  • Keywords needs-patch added; has-patch removed

Strong -1 on is_user_logged_in(), we should always be checking authorization (capabilities) not authentication (presence of user).

Why do we need to restrict this in any case? I see no potential security or privacy issues in exposing this.

Also, if we expose gmt_offset, we should also expose timezone_string. The former is not stable (i.e. it changes regularly in timezones with daylight savings), and the latter is the only real one you can depend on over time.

#9 in reply to: ↑ 3 ; follow-up: @rmccue
3 months ago

Replying to jnylen0:

We also need to make sure that gmt_offset returns an appropriate value even if the timezone_string is set instead, to e.g. America/New_York. These two options behave a bit strangely together.

Also on this point, gmt_offset is shimmed in wp_timezone_override_offset(), so reading it is always accurate. Only writes are tricky, but we're not handling this here, so no need to do anything special other than get_option()

#10 @sagarkbhatt
3 months ago

-Added gmt_offset and timezone_string field in wp-json response.
-A user can view both without any authentication.

#11 in reply to: ↑ 9 @jnylen0
3 months ago

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

Replying to rmccue:

gmt_offset is shimmed in wp_timezone_override_offset(), so reading it is always accurate. Only writes are tricky, but we're not handling this here, so no need to do anything special other than get_option()

Thanks - I always forget how this works 🙈

Given the discussion above, 39854.4.patch looks good to me, just needs tests.

@sagarkbhatt
3 months ago

Added unit tests

#12 @sagarkbhatt
3 months ago

Added unit test

#13 @jnylen0
3 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from 4.8 to 4.7.4

39854.5.patch looks good to me, thank you @sagarkbhatt.

I don't have any specific concerns with exposing this new information either, but my first thought was to restrict it since it's not really available elsewhere. Unless someone disagrees with the approach here, we'll get it committed after the 4.7.3 release ships.

#14 @dd32
3 months ago

Unless someone disagrees with the approach here, we'll get it committed after the 4.7.3 release ships.

I would prefer to see *everything*, especially "new" things committed to trunk for soak well in advance of 4.7.x merges, just to get extra testing of things prior to branch use - even though this is a small change, catching small discrepancies is hard if things get trunk commits delayed for any reason.

#15 @jnylen0
3 months ago

  • Keywords commit added

Noted, and makes sense. I'll get this committed as soon as we're done with shipping 4.7.3.

@jnylen0
3 months ago

Also update wp-api-generated.js fixture file

#16 @jnylen0
3 months ago

When you run the entire phpunit test suite it also updates the wp-api-generated.js fixture file if needed. When we make changes to the default API responses, this file needs to be updated as well. See #39264 for more info about that.

39854.6.diff is the same patch but with the updates to the fixture file.

#17 @jnylen0
3 months ago

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

In 40238:

REST API: Add gmt_offset and timezone_string to the base /wp-json response.

The site's current timezone offset is an important piece of information for any REST API client that needs to manipulate dates. It has not been previously available.

Expose both the gmt_offset (the site's current offset from UTC in hours) and timezone_string (which also provides information about daylight savings time) via the "site info" endpoint (the base /wp-json response).

Also update the wp-api-generated.js fixture file with the changes to the default API responses.

Props sagarkbhatt.
Fixes #39854.

#18 @jnylen0
3 months ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 4.7 branch.

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


2 months ago

#20 @swissspidy
2 months ago

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

In 40336:

REST API: Add gmt_offset and timezone_string to the base /wp-json response.

The site's current timezone offset is an important piece of information for any REST API client that needs to manipulate dates. It has not been
previously available.

Expose both the gmt_offset (the site's current offset from UTC in hours) and timezone_string (which also provides information about daylight
savings time) via the "site info" endpoint (the base /wp-json response).

Also update the wp-api-generated.js fixture file with the changes to the default API responses.

Props sagarkbhatt.
Fixes #39854.

Merges [40238] to the 4.7 branch.

#21 @adamsilverstein
2 months ago

@swissspidy thanks for this which will be super helpful when calculating dates.

I noticed you changed wp-api-generated.js in [40235], however, this file is generated by the phpunit test test_build_wp_api_client_fixtures and I think your changes will get overwritten when this test runs. I haven't had a chance to look carefully, I'm guessing you will need to update the generator test instead.

Last edited 2 months ago by adamsilverstein (previous) (diff)

#22 follow-up: @swissspidy
2 months ago

@adamsilverstein I think you mean [40336] , not [40235]? I am not the one to thank for as I only did the backport.

In my tests, test_build_wp_api_client_fixtures did not overwrite the wp-api-generated.js file. The test shouldn't need to be overwritten as it only prints out REST responses, which were appropriately adjusted in this ticket.

As for fixture generation, see also #40041. It's a bit weird that the file gets updated by a single PHP unit test.

#23 in reply to: ↑ 22 @adamsilverstein
2 months ago

Replying to swissspidy:

@adamsilverstein I think you mean [40336] , not [40235]? I am not the one to thank for as I only did the backport.

Ah right, looked odd to me on first glance as that file is generated and explicitly marked as 'shouldn't be edited'.
I will dig a bit deeper here to validate. Thanks.

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


7 weeks ago

Note: See TracTickets for help on using tickets.