Make WordPress Core

Opened 4 years ago

Closed 2 years ago

#21963 closed defect (bug) (fixed)

Consolidate post insertion APIs

Reported by: nacin Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch needs-unit-tests 3.6-early
Focuses: Cc:


In wp-includes, we have:

  • wp_insert_post()
  • wp_insert_attachment()
  • wp_update_post()
  • wp_publish_post()

For saving from the admin, we have:

  • edit_post()
  • wp_write_post()
  • write_post()

wp_publish_post() is, as of [21942], now wraps wp_insert_post().

wp_update_post() is a fairly mundane wrapper of wp_insert_post(), but we really should eliminate the differences between the two functions and make it a straight-up wrapper.

wp_insert_attachment() was a fork of wp_insert_post(), and wp_insert_post() has gotten a lot of improvements that haven't reached wp_insert_attachment(). It doesn't take much to merge these two, though, and make wp_insert_attachment() a wrapper.

wp_write_post() calls edit_post() if it has a post ID. And since we have had a post ID since the days of auto-drafts, this function is dead code. It's wrapper, write_post(), can also be deprecated.

I'm attaching a patch that takes care of wp_insert_post(), wp_write_post(), and write_post(). wp_update_post() will require a bit more concentration.

Needs testing and unit tests.

Attachments (8)

21963.diff (15.7 KB) - added by nacin 4 years ago.
21963.2.diff (16.1 KB) - added by ericlewis 4 years ago.
Freshness for 21963.diff, applies cleanly to trunk now.
21963.3.diff (14.3 KB) - added by wonderboymusic 3 years ago.
21963.4.diff (16.2 KB) - added by DrewAPicture 3 years ago.
21963.5.diff (1.6 KB) - added by kpdesign 3 years ago.
Restore hook docs removed in r28579
21963.6.diff (1.5 KB) - added by kovshenin 3 years ago.
21963.7.diff (17.8 KB) - added by kovshenin 3 years ago.
21963.8.diff (18.1 KB) - added by wonderboymusic 2 years ago.

Download all attachments as: .zip

Change History (46)

4 years ago

#1 @ocean90
4 years ago

  • Cc ocean90 added

#2 @scribu
4 years ago

  • Cc scribu added

#3 @nacin
4 years ago

See #11399. If [21942] needs to be reverted, we can do that here. See also [4077] #2737 #2715 #4620 for some history.

#4 @DrewAPicture
4 years ago

  • Cc xoodrew@… added

#5 @sirzooro
4 years ago

  • Cc sirzooro added

#6 @dd32
4 years ago

#22063 was marked as a duplicate.

#7 @toscho
4 years ago

  • Cc info@… added

#8 @nacin
4 years ago

#22047 was marked as a duplicate.

#9 @dnaber-de
4 years ago

  • Cc kontakt@… added

#10 @nacin
4 years ago

In [22189]:

Use wp_update_post() rather than wp_insert_post() in wp_publish_post() to avoid stomping of values like categories. props ericmann, fixes #22167. see #21963.

#11 @helenyhou
4 years ago

  • Keywords has-patch needs-refresh needs-unit-tests added

4 years ago

Freshness for 21963.diff, applies cleanly to trunk now.

#12 @ericlewis
4 years ago

  • Keywords needs-refresh removed

#13 @nacin
4 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release

#15 @nacin
4 years ago

In 23206:

Revert [21942] and have wp_publish_post() deal with the database directly. clean_post_cache() is now also called directly due to [21943].

fixes #22944 for trunk.
Unit tests: [1174/tests].

see #11399. see #21963.

#16 @nacin
3 years ago

wp_insert_attachment() sometimes inserts a null GUID, see #18310.

#18 @helen
3 years ago

#23399 was marked as a duplicate.

#19 @wonderboymusic
3 years ago

21963.3.diff makes wp_insert_attachment() a wrapper for wp_insert_post(). I am fairly confident all is good, plus I messed with them both a lot last week during #22400, but I will wait for someone else to chime in.

All unit tests pass.

3 years ago

#20 @DrewAPicture
3 years ago

  • Milestone changed from Future Release to 4.0

O_o purdy.

21963.4.diff updates the inline docs for wp_insert_attachment() and changes the $object parameter to $args since we're basically overwriting the function anyway.

#21 @wonderboymusic
3 years ago

In 28579:

Combine wp_insert_attachment() and wp_insert_post(). wp_insert_attachment() becomes a wrapper. Update inline docs.

Props wonderboymusic, DrewAPicture.
See #21963.

3 years ago

Restore hook docs removed in r28579

#22 @kpdesign
3 years ago

21963.5.diff restores the hook docs for wp_insert_attachment_data removed in r28579, and moves the hook docs for wp_insert_post_data to the proper place.

#23 @wonderboymusic
3 years ago

In 28582:

Fix some inline docs churn in wp_insert_post() after [28579].

Props kpdesign.
See #21963.

#24 @nacin
3 years ago

In 28601:

Revert [28579] and [28582]. see #21963.

#25 @nacin
3 years ago

This broke stuff. Specifically, was causing major issues with attachment metadata. I went with a revert for now as I previously looked at merging these functions and I was having all sorts of problems, so I suspect it is not an easy/quick fix.

#26 follow-up: @wonderboymusic
3 years ago

Killer - what specifically exploded? We should add unit tests for whatever it is. Every minor change I made to those functions caused like 500 tests to fail, but everything looked good in the end.

#27 in reply to: ↑ 26 @kovshenin
3 years ago

Replying to wonderboymusic: Patch + unit tests incoming.

3 years ago

3 years ago

#28 @kovshenin
3 years ago

It looks like update_attached_file() runs when updating the attachment, like adding a caption from the media modal, hence the _wp_attached_file meta value is gone. In wp_insert_attachment() we checked file with if ( $file ) before calling update_attached_file() which was false for updates, but in wp_insert_post() that became isset() which is always true because wp_insert_attachment() always sets the file argument.

I attached two patches. 21963.6.diff is against r28600 (before the revert) which shows what went wrong + a unit test. And 21963.7.diff which reverse-merges r28601 and applies .7.diff.

#29 @wonderboymusic
3 years ago

  • Owner set to nacin
  • Status changed from new to assigned

Please review at your leisure. Looks good to me, but I want it to be your fault next time.

#30 @wonderboymusic
3 years ago

kovshenin: what is your level of comfort with your latest patch?

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

3 years ago

#32 @kovshenin
3 years ago

Not sure what you mean, my patch is just empty vs. isset, which is pretty obvious and the first thing that broke. I haven't thoroughly tested the rest of it.

This ticket was mentioned in IRC in #wordpress-dev by rmccue. View the logs.

2 years ago

#34 @wonderboymusic
2 years ago

  • Owner changed from nacin to wonderboymusic
  • Status changed from assigned to reviewing

#35 @wonderboymusic
2 years ago

The original issue when we dropped this in last time was obvious once @kovshenin found it, he fixed it, and tests were added.

In .8.diff:

  • Use @kovshenin's patch ---> I tested it
  • Moved the guid logic outside of the attachment if/else - it should never be empty as per #18310. This fixes a unit test that would have broken when this ticket was marked closed.
  • Updated the unit test in Tests_Media::test_wp_prepare_attachment_for_js() to account for url no longer being empty - I can't imagine a scenario where this was ever desired

Let's try this again.

#36 @wonderboymusic
2 years ago

In 28788:

Reinstate the changes from [28579] with some adjustments:

  • Check ! empty( $postarr['file'] ) before calling update_attached_file()
  • Add a unit test: test_update_attachment_fields()
  • Run the same logic for empty guid for attachments that always ran in wp_insert_post(), as per #18310. This fixes a unit test that would have broken when this ticket was marked closed.
  • Updated the unit test in Tests_Media::test_wp_prepare_attachment_for_js() to account for url no longer being empty

Props kovshenin, wonderboymusic.
See #21963.

#37 @helen
2 years ago

Any issues now, a week in?

#38 @helen
2 years ago

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

Closing as fixed, then.

Note: See TracTickets for help on using tickets.