Fix slug and date validation for new, unpublished posts #2303
Conversation
…shed post with a blank slug
|
I think in the handle_terms() you want to leave it as it was and drop the |
fixes a php notice when the array is empty
|
For the conditional to array_map in handle_terms() maybe change the conditional to be if ( is_array( $request[ $base ] ) ) {
$terms = array_map( 'absint', $request[ $base ] );
} |
|
@BE-Webdesign good suggestion, that fixes the issue for me and the failing test now passes. The one test that is failing agains trunk is also failing in develop: its not related to this fix. |
| 'context' => array( 'view', 'edit', 'embed' ), | ||
| 'arg_options' => array( | ||
| 'sanitize_callback' => 'sanitize_title', | ||
| 'sanitize_callback' => 'sanitize_key', |
There was a problem hiding this comment.
We need to use sanitize_title() because core uses sanitize_title() for the slug, not sanitize_key(). See #1585
There was a problem hiding this comment.
Ok, I will try to dig in to see why this change resolves the issue I am seeing.
|
@adamsilverstein Can you make sure there's test coverage for the issues you've found? |
| switch ( $args['format'] ) { | ||
| case 'date-time' : | ||
| if ( ! rest_parse_date( $value ) ) { | ||
| if ( $value && ! rest_parse_date( $value ) ) { |
There was a problem hiding this comment.
I don't think we should necessarily only permit null values for dates, but not for other types or formats.
@WP-API/amigos Should we give null a free pass in validation (e.g. null is always an acceptable value for a field)?
There was a problem hiding this comment.
Thanks for the feedback: note that what I am trying to do is update a post that is new, and unpublished. I've tested with posts created in wp-admin or via the api. new, unpublished posts have '0000-00-00 00:00:00' for their date_gmt and blank slugs. Updates to the endpoint as an update with these values fails validation.
perhaps the api should just not return a slug and date_gmt for new unpublished posts? i'm going to try adjusting the client to skip sending these values to see if that resolves my issue.
There was a problem hiding this comment.
new, unpublished posts have '0000-00-00 00:00:00' for their gmt_date
Oh, huh. Maybe we should be special casing these, and returning null.
There was a problem hiding this comment.
@danielbachhuber I'd be in favour of treating null as the value not being included. Maybe we should do an array_filter to exclude null before we validate?
Also, would this break things if we handled it that way?
There was a problem hiding this comment.
Also, would this break things if we handled it that way?
Potentially. Someone should look at existing validation libraries to see how null cases are handled.
There was a problem hiding this comment.
The backbone client now includes a workaround for this issue.
|
@danielbachhuber for now I added a patch to the client to have it skip sending the blank slug and null date_gmt when saving the post: WP-API/client-js@8ed4be6 I think instead of changing the validation, we could change the return: and not return the null/blank values. Basically: if the values are not valid, the api should not return them. |
@adamsilverstein did you want to update this PR, or open a new one? |
|
@rachelbaker Sure, I will work something up. |
|
@rachelbaker I created a PR with my alternate proposed solution: #2393 I'm not sure what the best solution is, I this this is right - that the API should omit empty values in the return data; I'm curious to hear what you think or if this creates any issues I'm missing. |
|
I created #2657 to make a decision on |
|
Created a new PR for this: #2763. Closing. |
7 participants
When trying to save/update unpublished posts with the current validation in place, I am getting an error:
In my testing, newly created posts (via api or wp-admin) get a
post_date_gmtof0000-00-00 00:00:00- the js client can't parse that as a date and stores 'null. Sending this value pack to the API via a PUT request results in arest_invalid_dateerror before this patch. Checking for $value truthy before attempting to parse the date fixes the issue.Newly created, unpublished posts have a
post_nameakaslugthat is empty. The API returns this as an empty string ("slug": "") - sending an empty string back to the API before this PR results in therest_invalid_paramerror. I tracked this down to applyingsanitize_titleto the slug, this is somehow turning the empty string into an empty object. Changing the sanitization callback tosanitize_keyfixes this issue and I believe thats the appropriate sanitization call to use here in any case.also ensure the empty keys are not used in
handle_terms