#44975 closed feature request (fixed)
REST API support switching draft to unscheduled
Reported by: | mnelson4 | Owned by: | adamsilverstein |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | has-patch has-unit-tests commit |
Focuses: | Cc: |
Description (last modified by )
Ticket #39953 allows REST API clients to send a post's date in a POST request, and either leave the post as "date floating" (draft that says 'Publish immediately') switch it to be "non-date-floating" (draft that says 'Schedule for...').
However, REST API clients cannot make a request to switch the post BACK to being "date floating" (mentioned on https://core.trac.wordpress.org/ticket/39953#comment:25 and https://core.trac.wordpress.org/ticket/39953#comment:34).
This option isn't available in WordPress' default editor either (see https://core.trac.wordpress.org/ticket/8368#comment:20). However, from my testing, WordPress.com's Calypso editor DOES allow this. Also there's a very old ticket suggesting it be added: #8368, so it seems reasonable that the REST API should support this (and maybe Gutenberg too someday).
So how should a REST API client specify they want to switch a post back to date floating (say "Publish immediately" in the classic editor)?
Should they provide a post's date
set to null
, false
, ''
, or maybe even to the same value as modified
? (I suggest that last one because that's how you leave a currently-"date floating" post as-is, see https://core.trac.wordpress.org/ticket/39953#comment:25)
Attachments (7)
Change History (46)
#3
@
6 years ago
No, the API and its clients should use a separate field for this purpose.
Thanks for the feedback @jnylen0 and I mostly agreed, but you can see on #39953 that there's been quite a bit of resistance to adding a new field (not from me! See https://core.trac.wordpress.org/ticket/39953#comment:16. But I'm pretty un-opinionated and just want to help find a solution most people are happy with).
I suggest closing this ticket as a duplicate of #39953.
I opened this as a branch off of that ticket (see https://core.trac.wordpress.org/ticket/39953#comment:39) so that the currently-requested part of the feature (the ability to leave posts as "date-floating") can proceed un-impeded by this related-but-IMO-separate discussion. I'm pretty sure @kadamwhite ok'd this approach: https://wordpress.slack.com/archives/C02RQC26G/p1537482913000100.
Having said that, if there's a complete reversal on #39953, and we do end up using a date_floating
field, and we support this functionality using that field, then ya, we should close this ticket.
#4
@
6 years ago
The most convincing argument against a new field that I've seen so far is "feels like we shouldn't need it".
In principle I agree, but the way WP has worked for years is different. The task is then one of choosing the least ugly solution, with a design that makes things easier for API clients and encourages them to write clean, straightforward code themselves.
#5
@
6 years ago
- Keywords close removed
Respectfully, adding a field for the sole purpose of changing one piece of UI state in client interfaces is tremendous overkill. To springboard off your argument, the way WP has worked for years to my mind has been to embrace contextual solutions to historical inconsistencies, not force additional data and properties into classes, databases or API responses to perpetuate those inconsistencies. The intent of the REST API is to provide a consistent facade layer over those historical inconsistencies, not to expose those inconsistencies to clients through strange new response properties.
I do not regard this as perpetuating the mistakes of the past; I think we have already perpetuated those mistakes by returning a "false" date when a post is floating, rather than providing null
in that instance. That's an inconsistency we can address with v3 of the REST API, but I hold the ship has sailed for v2, and my goal therefore is to ensure that things work as consistently and predictably as possible when consuming the existing data.
I am in favor of this ticket, and believe the appropriate response to accept to restore floating date status would be null
, as that has the most explicit meaning of "unset this" in other APIs. However @danielbachhuber has requested that we validate whether we accept ''
or false
for unsetting values elsewhere in the API, to ensure internal consistency.
#6
@
6 years ago
If you were writing a new piece of code today that manipulates various date values, and also has a flag to control a specific date-related behavior, independent of the actual values of the dates, would you re-use a special value of one of the date variables to control this independent and separate flag, or use a different variable?
Put another way, as I understand the existing behavior, this argument inaccurate; the value of the date in the database, namely 0000-00-00T00:00:00
, _is_ the actual value that represents date floating. The presence of that zeroed-out value is a placeholder for a specific publish date's non-existence, i.e. the state of having floating date. That state is not stored as a separate flag in the data model, and representing it as such would further confuse the situation. Better, then, to do what is proposed in this ticket, and accept a null
value when writing a post, which will be converted to the zeroed-out placeholder on save.
#7
@
6 years ago
the way WP has worked for years to my mind has been to embrace contextual solutions to historical inconsistencies
When this involves encouraging API clients to write convoluted code, that's where I stop being convinced that it's the right solution.
I originally changed the behavior of the date_gmt
field in the API because posts should always have a usable UTC date field. I did that because working with UTC and possibly a timezone is the only provably correct way to handle dates, and so the date_gmt
field should always mean the UTC date and nothing else.
I think we have already perpetuated those mistakes by returning a "false" date when a post is floating, rather than providing
null
in that instance
It's not a null
date, though, because you still have the creation date of the draft. The root problem here is the lack of a clearly designed data model from the start. Some more thoughts on that here.
Anyway, I'm not going to be spending any further time on this. It's a tricky issue, and I trust y'all to come up with a solution that works.
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
6 years ago
This ticket was mentioned in Slack in #core by aldavigdis. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-editor by aldavigdis. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-restapi by aldavigdis. View the logs.
5 years ago
#13
@
5 years ago
There is a discussion about the same topic for Gutenberg over at https://github.com/WordPress/gutenberg/issues/12048. Perhaps it would be a good idea to coordinate on this and prioritise?
#14
@
5 years ago
I am in favor of this ticket, and believe the appropriate response to accept to restore floating date status would be null,
Accepting null
makes sense: inserting null for these crates the "floating" date (it is the default database value for these fields).
One potential issue however is that null
is the same as not sending any value at all for the field, right? This might lead to unexpected behavior if clients weren't always sending the date_gmt
field. I'm curious what precedent exists in other endpoints and APIs.
#15
@
5 years ago
44975.diff is a first pass at accepting an empty string for date or dage_gmt to reset the dates to floating ("publish immediately"). Needs tests, maybe a status check?
#18
@
5 years ago
In 44975.2.diff Add some unit tests.
#19
@
5 years ago
One potential issue however is that null is the same as not sending any value at all for the field, right?
Not sending any value for the field at all would, I would expect based on experience with both this and other APIs, result in no change to the record date of any sort. Sending null would either unset a previously specified date, or, if the date was already floating, cause no change.
#20
@
5 years ago
- Milestone changed from Future Release to 5.3
Let's try to land a fix for this ticket in 5.3.
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
5 years ago
#22
@
5 years ago
In 44975.3.diff:
- Use a
null
value in the test. - update doc blocks
In my testing sending a null value works just like sending a blank string. I updated the test to use null and adjusted the doc blocks a little as well.
Matching draft PR (with tests running): https://github.com/WordPress/wordpress-develop/pull/81
#23
@
5 years ago
- Keywords has-patch 2nd-opinion has-unit-tests added
- Version set to 4.7
@kadamwhite or @timothyblynjacobs can you please review 44975.3.diff when you have a chance!
This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
5 years ago
#26
@
5 years ago
44975.4.diff updated date types in schma" 'type' => [ 'string', 'null' ],
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
5 years ago
#29
@
5 years ago
Uploaded a patch that supports validation for type
being a list. Then, the post controller can specify that the date/date_gmt
is a 'type' => array( 'string', 'null' )
. Then, in prepare_item_for_database
check that the parameter was included in the request and set to null
.
This introduces WP_REST_Request::has_param
to make it simpler to check if a parameter with a null value was included in the request. The settings controller ( which also checks for null values ) works around this by using array_key_exists
on the existing ::get_params()
method.
This ticket was mentioned in Slack in #core-committers by kadamwhite. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
5 years ago
#32
@
5 years ago
Was asked to comment from Date/Time component perspective and... it's kinda highly not obvious.
- Sending
null
to null a date value makes sense on REST API level. - However underlying concept of null date in core is a mess.
"Zeroed" date is used as null value in database schema, but it's actually a valid date in PHP 7. So the legacy logic built on top of that is very likely in some state of broken at the moment.
So I suppose... If you want to literally zero a date then send null and call it done. Something is probably broken about it though.
Actual solution would really be designing a proper state of the post for this and how it transitions, which very likely would need to be dedicated and explicit, not derived from date fields.
PS for an extra level of not fun UTC dates specifically are unsuitable for scheduling purposes, because users schedule local time and any future time zone states relative from UTC are unknown.
#33
@
5 years ago
44975.5.diff worked for null in my testing - sending a null value for date_gmt reset the date correctly. Screencast - https://cl.ly/e8bb21e13ac3 - note I did have to reload to show the correct date, this is like a gutenberg issue though and might already be resolved in the plugin/5.3.
Checking the database, i see the expected 0 values for the post - https://cl.ly/8968c001c517/MySQL_5.7.26_wpdev.localhostdevelopwordpresswp_posts_2019-09-23_08-13-24.jpg
One question: is the intention to also allow an empty string to reset or only null? when I tested with empty strings the save was rejected with the error "Updating failed. Error message: Invalid parameter(s): date_gmt" - https://cl.ly/7d968427a5ff
#34
@
5 years ago
Thank you for the feedback @Rarst!
However underlying concept of null date in core is a mess.
In the patch we specifically set post_date
and post_date_gmt
to null
if the REST request parameter value is null
. However, we could instead set that to 0000-00-00 00:00:00
if that'd be better. AFAICT, they are handled the same.
So I suppose... If you want to literally zero a date then send null and call it done. Something is probably broken about it though.
Yeah, we want the field in the DB to be set zeroed. The null
value isn't persisted beyond the API layer. Do you have ideas what might be broken? IIRC from when we were researching this last year, it looks like passing an "empty" value to wp_insert_post
is the correct way to do this?
Actual solution would really be designing a proper state of the post for this and how it transitions, which very likely would need to be dedicated and explicit, not derived from date fields.
Did you see @kadamwhite's comment?
44975.5.diff worked for null in my testing - sending a null value for date_gmt reset the date correctly.
Awesome!
One question: is the intention to also allow an empty string to reset or only null?
Yeah, after discussing in #core-restapi we'll only allow null
and not try to convert an empty string to null
. This also matches behavior of how we handle null
in other endpoints like Settings and Meta fields.
Something I noticed when looking at the patch again, I think we need to check against the item schema for$schema['properties']['date(_gmt)']
the same way we do for the regular post date updates. I uploaded a patch to add that.
#35
@
5 years ago
In the patch we specifically set post_date and post_date_gmt to null if the REST request parameter value is null. However, we could instead set that to 0000-00-00 00:00:00 if that'd be better.
I don't think that makes a particular difference.
Do you have ideas what might be broken?
Not specifically and I don't have it in me to start untangling that mess at the moment.
Did you see @kadamwhite's comment?
Yeah, we chatted some out of thread as well.
Basically this is a mess on top of a mess. If your goal is to be most consistent with existing layers of mess then sending null
through REST API to reset date fields is probably passable solution.
I'd like to see this behavior entirely untangled from date fields (come on, this is not some peace of classical beauty, this is weird ancient kludge of crazy implementation detail) but that's probably not happening in context of REST API.
#36
@
5 years ago
- Keywords commit added; 2nd-opinion removed
The external interface being introduced here (accept null
to reset date) I believe to be solid, and we can address any internal inconsistencies we discover as a bugfix without changing that new interface. Marking for commit
for 5.3; thank you all for your input!
This ticket was mentioned in PR #77 on WordPress/wordpress-develop by adamsilverstein.
5 years ago
#38
Trac Ticket: https://core.trac.wordpress.org/ticket/44975
No, the API and its clients should use a separate field for this purpose.
If you were writing a new piece of code today that manipulates various date values, and also has a flag to control a specific date-related behavior, independent of the actual values of the dates, would you re-use a special value of one of the date variables to control this independent and separate flag, or use a different variable?
The correct answer is a different variable. Except that in this case, the decision made will end up affecting the design and implementation of any project that uses the REST API.
This same mistake was made 10 years ago, but please don't let that confuse the issue, and please don't perpetuate the mistake further. I suggest closing this ticket as a duplicate of #39953.