WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 8 months 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:

Description

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 2 years ago.
21963.2.diff (16.1 KB) - added by ericlewis 2 years ago.
Freshness for 21963.diff, applies cleanly to trunk now.
21963.3.diff (14.3 KB) - added by wonderboymusic 10 months ago.
21963.4.diff (16.2 KB) - added by DrewAPicture 10 months ago.
21963.5.diff (1.6 KB) - added by kpdesign 9 months ago.
Restore hook docs removed in r28579
21963.6.diff (1.5 KB) - added by kovshenin 9 months ago.
21963.7.diff (17.8 KB) - added by kovshenin 9 months ago.
21963.8.diff (18.1 KB) - added by wonderboymusic 9 months ago.

Download all attachments as: .zip

Change History (46)

@nacin2 years ago

comment:1 @ocean902 years ago

  • Cc ocean90 added

comment:2 @scribu2 years ago

  • Cc scribu added

comment:3 @nacin2 years ago

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

comment:4 @DrewAPicture2 years ago

  • Cc xoodrew@… added

comment:5 @sirzooro2 years ago

  • Cc sirzooro added

comment:6 @dd322 years ago

#22063 was marked as a duplicate.

comment:7 @toscho2 years ago

  • Cc info@… added

comment:8 @nacin2 years ago

#22047 was marked as a duplicate.

comment:9 @dnaber-de2 years ago

  • Cc kontakt@… added

comment:10 @nacin2 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.

comment:11 @helenyhou2 years ago

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

@ericlewis2 years ago

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

comment:12 @ericlewis2 years ago

  • Keywords needs-refresh removed

comment:13 @nacin2 years ago

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

comment:15 @nacin2 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.

comment:16 @nacin20 months ago

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

comment:18 @helen15 months ago

#23399 was marked as a duplicate.

@wonderboymusic10 months ago

comment:19 @wonderboymusic10 months 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.

@DrewAPicture10 months ago

comment:20 @DrewAPicture10 months 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.

comment:21 @wonderboymusic9 months 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.

@kpdesign9 months ago

Restore hook docs removed in r28579

comment:22 @kpdesign9 months 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.

comment:23 @wonderboymusic9 months ago

In 28582:

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

Props kpdesign.
See #21963.

comment:24 @nacin9 months ago

In 28601:

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

comment:25 @nacin9 months 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.

comment:26 follow-up: @wonderboymusic9 months 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.

comment:27 in reply to: ↑ 26 @kovshenin9 months ago

Replying to wonderboymusic: Patch + unit tests incoming.

@kovshenin9 months ago

@kovshenin9 months ago

comment:28 @kovshenin9 months 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.

comment:29 @wonderboymusic9 months 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.

comment:30 @wonderboymusic9 months ago

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

comment:31 @ircbot9 months ago

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

comment:32 @kovshenin9 months 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.

comment:33 @ircbot9 months ago

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

comment:34 @wonderboymusic9 months ago

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

@wonderboymusic9 months ago

comment:35 @wonderboymusic9 months 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.

comment:36 @wonderboymusic9 months 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.

comment:37 @helen8 months ago

Any issues now, a week in?

comment:38 @helen8 months ago

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

Closing as fixed, then.

Note: See TracTickets for help on using tickets.