Make WordPress Core

Opened 8 years ago

Last modified 7 years ago

#36462 new defect (bug)

Updating or publishing a (custom) post that hasn't loaded completely closes comments

Reported by: sebsz's profile SeBsZ Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4.2
Component: Posts, Post Types Keywords: has-patch 2nd-opinion
Focuses: administration Cc:

Description

I am using a custom post type, but I assume this happens to the default post type as well. On the edit post screen (post.php?post=1&action=edit) I have several custom meta boxes. Some of these have content that is quite slow to load. You can reproduce this behavior by adding a sleep(5) statement somewhere in the code that loads the content for a custom meta box. Now in the document's DOM, the sidebar is loaded before the custom meta boxes. This introduces a situation where it is possible to update or publish a post before all the meta boxes have completely loaded. In most cases this isn't a huge problem - I myself check to see if the $_POST fields are there and if they are not then I don't act upon them.

Unfortunately this does not happen for the included "Discussion" meta box. This box has a checkbox named "Allow Comments" which gets switched off when you update the post before this meta box has loaded into the DOM.

The culprit is the code in wp-admin/includes/post.php on line 133 in the _wp_translate_postdata() function:

if (!isset( $post_data['comment_status'] ))
  $post_data['comment_status'] = 'closed';

Since the comment_status field is not in the post data, it is automatically assumed it needs to be closed.

Of course there are two "workarounds" I can think of that would improve my current situation. One is for me to optimize the meta boxes so the page loads quicker, the other is to move the Discussion metabox to the top of the page, so it loads first.

Is this expected behavior? I would much rather see the current comment_status be preserved - don't touch it if I didn't intend to modify it. Of course there might be a reason for this implementation that I don't know about.

This post data is then finally presented to wp_insert_post in wp-includes/post.php which actually updates the post's comment_status to become closed, which finally answers my boss' question why comments kept getting disabled automatically.

Attachments (2)

patch.diff (2.1 KB) - added by SeBsZ 8 years ago.
patch.2.diff (2.1 KB) - added by SeBsZ 8 years ago.

Download all attachments as: .zip

Change History (17)

#1 @archon810
8 years ago

This has been driving me nuts for a long time now. To add, it's not always even the case with custom posts, even regular posts on our main blog close comments sometimes because of this bug (especially longer posts).

Plugins that do some processing on the edit page, a slowdown in db, or really quick fingers seem to bring it about.

I was only able to trace down the real cause after enabling the query log and seeing that comments get closed from the edit page, then reproducing quite easily.

#2 @archon810
8 years ago

Any updates? It's very easy to reproduce.

#3 @boonebgorges
8 years ago

  • Focuses administration added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Any updates? It's very easy to reproduce.

It does look like there's a problem. Do you have any ideas on how to resolve? The problem is that comment_status (and some other fields) are passed as a checkbox. So when we inspect the POST payload, and we don't see the comment_status value, it's not possible to interpret it: has comment_status been disabled, or did the checkbox not load into the DOM?

Moving the Discussion box to the top of the screen seems like a potential workaround, but it just punts the problem - there could be another checkbox further down the page that exhibits the same issue.

One answer is not to use checkboxes at all. This does not seem viable to me, since checkboxes are sometimes the best UX for a given setting.

Another is to output a hidden field that contains the *actual* value of comment_status, and to update that field via JS when the checkbox is toggled. Then, we look at this hidden field in the POST handler. The downside of this is that it doesn't degrade well when JavaScript is disabled. Perhaps *another* hidden field could be added via JS when the form is rendered - something like <input type="hidden" name="do-checkbox-compat" value="1"> (obviously, a better name would be nice) - and then the POST handler would only look for the value of the hidden fields if do-checkbox-compat were present. I think this strategy has potential, but it needs prototyping and testing around the edges - slow-loading pages, JS disabled, JS broken by a plugin, etc - to move forward.

#4 @SeBsZ
8 years ago

First of all, thank you for your reply.

So, what I do when using checkboxes is I always include a hidden field with a value of 0 or false, I forget which. When a checkbox hasn't been checked it won't show up in the POST data, but the hidden field with the same name will.

Then in the backend, only change the comment_status when it is in the POST data. If it is not in the POST data don't do anything with it, it means that part hasn't loaded into the UI yet.

I'm not sure why you are suggesting using JS, I don't believe there is a need for that.

Would you like me to write a patch for this based on this logic? It'll be my first patch for Wordpress so bear with me.

Thanks again.

#5 @archon810
8 years ago

What about moving the submit button area to the bottom in the DOM somehow, or even better and simpler - disable the submit functionality until the DOM is loaded in full?

Submitting forms before the whole page is loaded is a recipe for disaster.

#6 @boonebgorges
8 years ago

I'm not sure why you are suggesting using JS, I don't believe there is a need for that.

I was just trying to suggest a possible solution. If it can be done without JS, let's definitely look into it. I'd love to see a patch.

disable the submit functionality until the DOM is loaded in full?

Sure, this is worth exploring. I assume that this will require JS, so it might be considered a progressive enhancement to whatever @SeBsZ has in mind.

@SeBsZ
8 years ago

#7 @SeBsZ
8 years ago

I just created a simple patch, which I've tested and can confirm works.

I've added two hidden fields in meta-boxes.php, one for comment_status and the other for ping_status. Then I removed the code in post.php where it closes the comments and ping_status if the POST data is missing.

I checked and by default, if the POST data is missing both fields are set to what they currently are, so nothing is changed in that case. Then if the POST data does exist, both fields are set accordingly. This is exactly what we want.

Of course this is just for these two checkboxes, this should probably be expanded to work for other checkboxes so I'm not sure how to go about that here.

Let me know if I can be of further help to get this implemented.

Last edited 8 years ago by SeBsZ (previous) (diff)

@SeBsZ
8 years ago

#8 @archon810
8 years ago

Yeah, I would definitely want to see a more generic solution than one that applies only to those 2 checkboxes and not any that could be added by some plugin.

#9 follow-ups: @boonebgorges
8 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

Yeah, I would definitely want to see a more generic solution than one that applies only to those 2 checkboxes and not any that could be added by some plugin.

Not sure there's much we can do for plugins, at least if the plugins are building their own markup. But yeah, if we are going to adopt a technique for core checkboxes, we should probably do it consistently throughout.

I've added two hidden fields in meta-boxes.php, one for comment_status and the other for ping_status. Then I removed the code in post.php where it closes the comments and ping_status if the POST data is missing.

This is pretty clever. I'd been imagining something less clever but more straightforward:

<input name="comment_status_loaded" type="hidden" value="1">
<input name="comment_status" type="checkbox" id="comment_status" value="open" <?php checked($post->comment_status, 'open'); ?> />

and then the submission handler would check isset( $_POST['comment_status_loaded'] ) before saving a value. Your solution is a lot more elegant.

It feels sorta like cheating to have more than one input element on a page with the same 'name', and then to take advantage of the fact that PHP's behavior with respect to POST (ie, an array) will cause the second one to overwrite the first. But I can't find anything in the HTML spec that suggests that it's invalid markup, and I can't find anything that suggests that we shouldn't trust PHP's behavior here.

What do others think? Would be curious to hear a take from someone like @helen.

#10 in reply to: ↑ 9 @archon810
8 years ago

Replying to boonebgorges:

Yeah, I would definitely want to see a more generic solution than one that applies only to those 2 checkboxes and not any that could be added by some plugin.

Not sure there's much we can do for plugins, at least if the plugins are building their own markup. But yeah, if we are going to adopt a technique for core checkboxes, we should probably do it consistently throughout.

Well, what about my earlier suggestion to make sure the Publish box loads last? Or that it's disabled until the page has loaded?

#11 @boonebgorges
8 years ago

Well, what about my earlier suggestion to make sure the Publish box loads last? Or that it's disabled until the page has loaded?

Sure, these are worth exploring. I think we'd need a decent amount of UX thought around the idea of a Publish box whose appearance on the page may be delayed by slow-loading plugins. And if the Publish button is going to be disabled, there should probably be good feedback as to *why* it's disabled. If you have ideas about how the UX would work, please feel free to share ideas or patches.

#13 @archon810
8 years ago

Wow, a 6 year old bug.

#14 in reply to: ↑ 9 @helen
8 years ago

Replying to boonebgorges:

It feels sorta like cheating to have more than one input element on a page with the same 'name', and then to take advantage of the fact that PHP's behavior with respect to POST (ie, an array) will cause the second one to overwrite the first. But I can't find anything in the HTML spec that suggests that it's invalid markup, and I can't find anything that suggests that we shouldn't trust PHP's behavior here.

I agree that it feels like cheating and a little risky to trust PHP to not break this someday, but if inline docs tell us why it's like this for the sake of the future and we perhaps add some kind of automated test for it (not a blocker if this means wandering into testing frameworks we haven't implemented yet and aren't close to adding), it seems okay to me. I am not aware of any other place we might do this kind of thing in core - the most similar situation I can think of is with post category checkboxes, where there's a hidden input with the same name with a value of 0. However, in that instance it uses [] in the name since the checkboxes proper should send over an array.

#15 @archon810
7 years ago

Bump. Any updates?

Note: See TracTickets for help on using tickets.