Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#43559 closed enhancement (fixed)

wp_ajax_add_meta() does not allow empty values

Reported by: charlestonsw's profile charlestonsw Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.0 Priority: normal
Severity: normal Version: 5.1
Component: Options, Meta APIs Keywords: has-patch has-unit-tests fixed-5.0
Focuses: rest-api Cc:

Description

Users should be allowed to set post meta keys to an empty string.

With custom post types there are many ways to use the custom meta data. In one of my use cases I use the meta values to drive a remote application. The REST API sends back a JSON response. If a key is present in the response it sets the corresponding value in the remote app.

Having the ability to send back a key with an EMPTY STRING value is critical for writing simple reactive applications (Vue , React, etc.) that call upon the WordPress REST API without having to craft a completely custom solution.

I am trying to use a standard WP REST endpoint to fetch a custom post type. Without the ability to set a custom post type "custom field" to an empty string I am going to have to write a custom post type so a user can enter something crazy like   into that field , catch that, and then convert it to before sending to the remote app. That is a bad hack -- when doing HTML/CSS values back to the remote app a user might actually WANT   in the output.

Why are values being rejected in WP Posts custom meta in the first place?

IMO this should be removed from \wp_ajax_add_meta() in wp-admin/includes/ajax-actions.php

		if ( '' == trim($key) )
			wp_die( __( 'Please provide a custom field name.' ) );

Or at least add a filter/hook so this default behavior can be mitigated:

		if ( apply_filters( 'wp_reject_meta_values' , '' , $pid ) == trim($key) )
			wp_die( __( 'Please provide a custom field name.' ) );

Even better - allow users to provide an array of values to reject:

                $rejected_values = (array) apply_filters( 'wp_reject_meta_values' , '' , $pid );
		if ( in_array( trim( $key ) , $rejected_values )  )
			wp_die( __( 'Please provide a custom field name.' ) );

Attachments (2)

43559.diff (3.6 KB) - added by soulseekah 7 years ago.
Tests + implementation
43559.2.diff (3.8 KB) - added by soulseekah 6 years ago.
Refresh for 5.0

Download all attachments as: .zip

Change History (20)

#1 @soulseekah
7 years ago

Thank you for opening this issue.

I think you're mixing up keys and values. Meta keys cannot be empty, and shouldn't be. Meta values can be empty all you want.

Can you please provide a fuller example of what you're trying to do via REST? What's the call?

#2 @SergeyBiryukov
7 years ago

  • Component changed from Posts, Post Types to Options, Meta APIs
  • Focuses rest-api added
  • Keywords reporter-feedback added

#3 @charlestonsw
7 years ago

Sorry - late night coding.

Same thing happens for values... copied the wrong code block.

		if ( '' == trim($value) )
			wp_die( __( 'Please provide a custom field value.' ) );

To reproduce:

Create a custom post type (probably not necessary based on the code from WP Core).
Add Custom Field.
Enter a key, "test".
Leave value empty.
Save.

You'll get the error above in the notification box.

So you have no way to add a custom field with a blank value.

I have a JS listener that does a standard WP REST call to fetch the post with full meta. I use this to remotely configure plugins, so if a setting for a text field, call it "Search Box Title" should be set to BLANK I cannot do this. I am now sending   and in the listener I then have to do if $json_data[ 'search_box_title' ] === ' ' $json_data['search_box_title']='';

Not I cannot set my "custom_html' setting to  

Granted that is unlikely to happen in my particular instance but this is a notable shortcoming in the custom fields/post meta.

I'd much rather use core REST functionality and send back a standard response and consume the meta fields with a reactive app than have to use custom settings and document for my clients "well, if you really want to set that value to blank enter   as the value in the field". Only code geeks will even know why I used that when really I wanted them to be able to turn the search box title OFF completely setting it from the default 'Enter Your Search Parameters' to and skipping the header rendering entirely.

Same premise as above, swap the word 'key' with 'value'.

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

@soulseekah
7 years ago

Tests + implementation

#4 @soulseekah
7 years ago

  • Keywords has-unit-tests has-patch added; reporter-feedback removed

Apparently, historically the Meta API did not allow empty values to be saved (see #11650).

The current version of WordPress, however, does allow empty values to be saved so with a couple of fixes here and there, empty values can now be saved. Attached patch with tests. Patch lifts the limitations imposed by both add_meta and wp_ajax_add_meta.

P.S. It's still unclear why REST has anything to do with it, though. Care to clarify?

Special thanks to @versusbassz for helping on the patch.

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

#5 @charlestonsw
7 years ago

REST is just one example use case.

The primary issue is not being able to save empty values on custom fields.

There are hundreds of use cases where empty values on custom fields would be desirable. A reactive app built on top of the standard WP REST API is one of many. I don't think I tagged this originally as a rest-api issue as that was not the primary issue being reported.

As for the current version of WordPress allowing empty meta values -- that may be the theory but that is not how the WordPress admin interfaces are working for pages/posts today (production or 5.0-alpha builds).

#6 @soulseekah
7 years ago

Reading the REST API code, I'm pretty sure there are no limitations on the meta value being empty. The REST API code uses the Meta API to store the values, which gladly accepts empty values. The schema validation does not appear to have any sort of limitations on the value.

If you have the time, please investigate the related REST API issue in a separate issue (and link it here, if possible). This issue is for add_meta and wp_ajax_add_meta (i.e. stuff being done in the admin).

Other than that, please feel free to try out our patches. Cheers.

#7 @charlestonsw
7 years ago

I will try the patches -- I"m sure they'll be fine as I've modified WP Core myself on my VVV box to test the theory behind the original request.

The question is will these patches make into core someday? My app is working fine , I wrote my own workaround so my users can work with the current limitations of WordPress as per the full use case here:

The full use case --

1a) Site admin logs into the main services site.
1b ) Open a custom post type from the WP Admin page.
1c) Add a custom field "box_title".
1d) Save the custom field (leaving value as blank) -- PROCESS FAILS per WP Core restriction.

2a) An end user consumes the meta from a remote app via the REST API.
2b) The app does not set the text inside a HTML element per the Vue app to blank because the post has not matching key (see 1b above).

Could I build a new app that uses the REST API to force meta onto the original post and set it to blank, sure can. But why would I go about writing a web app to replicate the base WordPress app with a new admin UX just so they can set a custom field value to blank? I could write an entire app to replace WordPress admin and front end UX -- possibly even using the REST API much like Calpyso. But why?

#8 @soulseekah
7 years ago

Understood. Thanks for your clarifications.

The patch and tests that I wrote should address both cases. It should make it into core, 5.0 very probably, so stay tuned ;)

Cheers.

#9 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#11 @danielbachhuber
6 years ago

@SergeyBiryukov If you feel comfortable with the patch, then I think this makes sense to land in 5.0 to ensure there's behavioral parity between Gutenberg and the REST API.

#12 @pento
6 years ago

Agreed, this would be nice to have in 5.0.

#13 @danielbachhuber
6 years ago

  • Keywords needs-refresh added

Patch doesn't apply cleanly against 5.0 branch and needs a refresh.

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


6 years ago

@soulseekah
6 years ago

Refresh for 5.0

#15 @soulseekah
6 years ago

  • Keywords needs-refresh removed

#16 @danielbachhuber
6 years ago

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

In 43811:

Meta: Allow empty strings to be set by Custom Fields meta box.

Because the REST API allows meta keys to have empty values, the Custom Fields meta box should permit the same behavior.

Props charlestonsw, soulseekah.
Fixes #43559.

#17 @danielbachhuber
6 years ago

  • Keywords fixed-5.0 added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merge to trunk.

#18 @desrosj
6 years ago

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

In 44153:

Meta: Allow empty strings to be set by Custom Fields meta box.

Because the REST API allows meta keys to have empty values, the Custom Fields meta box should permit the same behavior.

Props charlestonsw, soulseekah, danielbachhuber.

Merges [43811] to trunk.

Fixes #43559.

Note: See TracTickets for help on using tickets.