WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 9 days ago

#39072 closed defect (bug) (wontfix)

Add gmt_offset as a REST API setting

Reported by: jshreve Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: needs-unit-tests needs-patch close
Focuses: rest-api Cc:
PR Number:

Description

The timezone string can be seen and updated via the REST API, but there is no way to get the GMT offset (which I would imagine is more more valuable for calculating things). There is also no way to update it (aka manual offset).

The attached patch is one way to fix it. It makes sure to clear out the timezone string if you are manually overriding/setting the offset. I'm not sure if what I have is the best way to handle that case for the API, but I took a stab at it. If a timezone string is set, the offset is correctly returned for that timezone.

Attachments (1)

gmt_offset_rest_api.diff (1.9 KB) - added by jshreve 3 years ago.

Download all attachments as: .zip

Change History (26)

#2 @jnylen0
3 years ago

We need a way to expose gmt_offset and other site info, but currently we intend the settings endpoint to be used only for settings that can be updated (both reading and writing require manage_options capability). See also #38731.

#3 follow-up: @jnylen0
3 years ago

  • Keywords has-patch needs-unit-tests added

currently we intend the settings endpoint to be used only for settings that can be updated

My mistake about this part - sitting next to @jshreve and he pointed out that this is possible to update via wp-admin.

This ticket also made me think of the general need to expose gmt_offset and other data to API clients that may not have manage_options capability. For Quick Draft this is currently done via wp_localize_script which isn't a great solution (https://core.trac.wordpress.org/ticket/38342#comment:48).

#4 in reply to: ↑ 3 ; follow-up: @rmccue
3 years ago

Replying to jnylen0:

currently we intend the settings endpoint to be used only for settings that can be updated

My mistake about this part - sitting next to @jshreve and he pointed out that this is possible to update via wp-admin.

gmt_offset is actually a bit of a strange setting; it can either be set (the manual timezone offsets in the timezone dropdown), or derived from the actual timezone. Likely, the settings endpoint should expose the raw value (either unset or the manual timezone offset), and the site info can expose the derived value (which will always have a value).

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


3 years ago

#6 @pento
3 years ago

  • Milestone changed from Awaiting Review to 4.7.1

We can chat about this a bit more, but it doesn't need to be in 4.7.

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


3 years ago

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


3 years ago

#9 @dd32
3 years ago

  • Keywords needs-patch added; has-patch removed

I'm marking this as needs-patch, as it sounds like the approach in gmt_offset_rest_api.diff isn't complete for what we should be doing here.

#10 @jnylen0
3 years ago

  • Milestone changed from 4.7.1 to 4.7.2

gmt_offset and timezone_string are weird. I'd like to see this weirdness abstracted out of the API so that you can set and read either one any time that operation would make sense.

This isn't going to make it into 4.7.1 though.

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


3 years ago

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


3 years ago

#13 @jnylen0
3 years ago

  • Milestone changed from 4.7.3 to 4.8

Punting to 4.8 due to inactivity.

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


3 years ago

#15 @flixos90
3 years ago

  • Milestone changed from 4.8 to Future Release

Punting further due to inactivity.

#16 in reply to: ↑ 4 @nickylimjj
9 months ago

Replying to rmccue:

Replying to jnylen0:

currently we intend the settings endpoint to be used only for settings that can be updated

My mistake about this part - sitting next to @jshreve and he pointed out that this is possible to update via wp-admin.

Likely, the settings endpoint should expose the raw value (either unset or the manual timezone offset), and the site info can expose the derived value (which will always have a value).

I am picking this up. Could you @rmccue elaborate more on this? I am envisioning the interaction to be with settings endpoint to get the timezone and the posts endpoint to get the date_gmt, and calculating the local time_posted from there. Is this right?

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


5 months ago

#18 @TimothyBlynJacobs
5 months ago

@nickylimjj It looks like the values themselves were added to the site info endpoint in #39854. So the most straightforward solution would be to register the setting in register_initial_settings().

However, that wouldn't solve the weirdness that @jnylen0 mentioned. Perhaps @rarst has some thoughts on this with the current date-time improvements going on in core?

This ticket was mentioned in Slack in #core-datetime by timothybjacobs. View the logs.


5 months ago

#20 @Rarst
5 months ago

From the perspective of Date/Time component: absolutely do not add more gmt_offset operations. Among many things it doesn't account for summer time (!) when based on timezone_string. We have some huge pending fixes for that at the moment.

You do not want to ever have to use gmt_offset for anything and it's very unfortunate that we are not in practical position to drop it (or even further discourage) altogether.

#21 follow-up: @kadamwhite
4 months ago

@Rarst Thanks for chiming in! Based on that feedback, would you recommend we close this as wontfix?

#22 @kadamwhite
4 months ago

(Also relevant: GMT Offset is available read-only in the root / Rest API route response (or /wp-json/, with pretty permalinks). See #39854

#23 @kadamwhite
4 months ago

  • Keywords close added

#24 in reply to: ↑ 21 @Rarst
4 months ago

Replying to kadamwhite:

@Rarst Thanks for chiming in! Based on that feedback, would you recommend we close this as wontfix?

Probably. I mean if someone really needs it, they can always extend the API, right? So for general case it definitely should not be encouraged.

#25 @TimothyBlynJacobs
9 days ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing based on @Rarst's comment.

Note: See TracTickets for help on using tickets.