Make WordPress Core

Opened 15 years ago

Closed 14 years ago

Last modified 14 years ago

#9471 closed defect (bug) (fixed)

Attachments uploaded before post is saved or autosaved get 0 (zero) for post_parent (instead of temp_id value)

Reported by: aesqe's profile aesqe Owned by: scribu's profile scribu
Milestone: 2.9 Priority: normal
Severity: blocker Version: 2.8
Component: Media Keywords: has-patch tested commit bug-hunt
Focuses: Cc:

Description

Somewhat similar to #3757 and #6389, I guess. Using 2.8-bleeding-edge.

Attachments (3)

schema.patch (972 bytes) - added by scribu 15 years ago.
Make post_parent accept neggative values
9471.diff (5.4 KB) - added by Denis-de-Bernardy 15 years ago.
_relocate_children.diff (1.2 KB) - added by scribu 15 years ago.
use postmeta to save the temp_ID

Download all attachments as: .zip

Change History (58)

#1 follow-up: @azaozz
15 years ago

  • Component changed from Administration to Upload
  • Keywords needs_patch added; upload attachment autosave removed
  • Owner anonymous deleted
  • Priority changed from high to normal
  • Severity changed from blocker to normal

This is a limitation of the current uploading process. Attachments cannot be added before the post has a positive ID, the field in the db doesn't support negative numbers for post_parent either.

On the other hand autosave runs as soon as the user types a title which is logically the first step in creating a post, so this rarely happens. Also attachments ca be re-attached from the Media Library screen.

Perhaps we can tie autosave with the uploader buttons too so it would run if the post hasn't been saved yet and then open the uploader. This will make the uploader wait for autosave to complete and may take some time depending on the connection speed.

#2 in reply to: ↑ 1 @Denis-de-Bernardy
15 years ago

Replying to azaozz:

Perhaps we can tie autosave with the uploader buttons too so it would run if the post hasn't been saved yet and then open the uploader. This will make the uploader wait for autosave to complete and may take some time depending on the connection speed.

+1 to that

#3 @hakre
15 years ago

Add a new post-state: init. then everything can be attached w/o even needing an autosave (that needs javascript, right? so what for those w/o js?!).

posts with state init and without a recent change since a period of time (let's say 24 hours) can be deleted and cleaned up by cron.

looks much more mature and failsafe to me.

#4 @hakre
15 years ago

init could be named _new.

#5 @Denis-de-Bernardy
15 years ago

  • Milestone changed from 2.8 to Future Release
  • Priority changed from normal to low
  • Severity changed from normal to minor

whatever we do it can wait until 2.9

#6 @Denis-de-Bernardy
15 years ago

  • Milestone changed from Future Release to 2.8.1

#7 @Denis-de-Bernardy
15 years ago

see also #8932 (the same bug for uploads in QuickEdit)

#8 follow-up: @scribu
15 years ago

I think it would be safer and easier to change the db schema to allow negative integers. This way we don't have to rely on autosave. We already have a function called _relocate_children() which updates the attachments with negative parent ids.

#9 @scribu
15 years ago

  • Keywords has-patch needs-testing added; needs_patch removed

And here is the patch to prove it. It doesn't rely on autosave at all.

@scribu
15 years ago

Make post_parent accept neggative values

#10 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from 2.8.1 to 2.9

bigint(20) is large enough, and you'd need to change the other db tables to enforce consistent row types, so as to make it possible to create the foreign key constraints in an InnoDB setup.

#11 @Denis-de-Bernardy
15 years ago

see also #10217 (gallery not saved)

#12 @Denis-de-Bernardy
15 years ago

  • Component changed from Upload to Autosave
  • Keywords blocker added
  • Milestone changed from 2.9 to 2.8.1
  • Priority changed from low to normal
  • Severity changed from minor to major

up'ing the priority of this one a bit, as it's truly annoying. it's also worse than initially reported:

  1. create a new post
  2. enter a title
  3. click the insert image button (you can then see autosave happening)
  4. insert an image
  5. image is not attached to the post

whereas:

  1. create a new post
  2. enter a title
  3. click the post's content, wait for the autosave to finish
  4. insert an image
  5. image is attached to the post

#13 @Denis-de-Bernardy
15 years ago

I attached patch that tries to provide a partial fix, by making the autosave happen on the fly for new posts.

One issue I ran into is that thickbox seems to want to fire and load the iframe before the autosave has returned an ID. I worked around this by disabling the button until autosave is done, but one ends up needing to click it twice on drafts without any title or content.

If anyone knows how to delay an event until the ajax fires...

#14 @Denis-de-Bernardy
15 years ago

until the ajax returns a result, even...

#15 @hakre
15 years ago

Then I assume the problem with new Posts is that those aren't created until the first time saved? Why not save them on opening the new post screen? Then there would be no need for an initial autosave for which JS must be on.

#16 follow-ups: @Denis-de-Bernardy
15 years ago

That would clutter the database with huge amount of drafts. A better approach might come when we switch to using a UUID as the post_guid. We can then pass an arbitrary UUID on new posts. It'll be essentially guaranteed to be unique, without needing to fire any autosave at some weird moments.

#17 in reply to: ↑ 16 @hakre
15 years ago

Replying to Denis-de-Bernardy:

That would clutter the database with huge amount of drafts.

Not really. The Datanbase is currently cluttered with a huge amount of revisions. Imagine having one Draft for a Post that is being written. Timing out after 6 hours and then deleted. That's not an argument I can understand.

#18 in reply to: ↑ 16 @azaozz
15 years ago

Replying to Denis-de-Bernardy:
... We can then pass an arbitrary UUID on new posts. It'll be essentially guaranteed to be unique, without needing to fire any autosave at some weird moments.

Or perhaps can have another post_status of "new" that will be reused if exists until the post is saved and the status is changed. Then there will be only one "started" post or page regardless of how many times the Add New screen is loaded.

The bad thing is that it will have to be checked/created for every loading of the Dashboard too since images can be uploaded from QuickPress.

#19 @Denis-de-Bernardy
15 years ago

I'd personally advise the UUID approach for new posts. It would get passed as a hidden field, and if the post with such doesn't exist we can then create it on the fly without risk for collisions.

#20 @hakre
15 years ago

But this means another field in the db, doesn't it?

#21 @Denis-de-Bernardy
15 years ago

No no, it would be the post_guid. The latter needs to be unique anyway. It currently contains the url of the post, as it would appear with no fancy urls. It could just as well contain a UUID (see #6492).

It's not safe to pre-assign a numerical ID to a post, but it's perfectly safe to pre-assign a UUID. Instead of passing a $temp_ID around, we'd pass a $post_guid. Using the $post_guid, we get the ID (creating the post on the fly if needs be).

#22 @azaozz
15 years ago

Having a "real" ID on yet unsaved post would simplify things and remove the need for temp_ID completely, but having another ID added to unsaved post would only make things more complex.

BTW according to #6492 UUID generated from MySQL cannot be used, perhaps something like:

md5( uniqid( get_option('siteurl'), true ) );

would be good enough.

#23 @Denis-de-Bernardy
15 years ago

@azaozz: we must be reading that ticket differently. UUID works fine and is globally unique in time and space (in theory anyway). md5(uniqid()) would introduce collisions (in theory again) if used to do the same thing.

The part that didn't work until MySQL 5.1 is this: servers s1/s2, with statements on s1 being replicated over on s2. the statement, insert into table (guid) values(uuid()), would then make uuid() run on the two servers independently, resulting a separate uuid being generated on each server. the workaround is easy, too: store the results of select uuid() into $uuid, and then, insert into table(guid) values($uuid).

In my non-WP apps, I'm constantly using UUIDs to deal with double clicks on forms that have yet to be inserted data, e.g. when a user tries to register and clicks the submit button twice, when a new page is created and the save button gets clicked twice, etc. It's a lot more robust than trying to disable submit buttons.

But we're getting off topic. :-)

#24 follow-up: @azaozz
15 years ago

Perhaps we are reading it differently. The question that wasn't asked there is: what are the advantages (if any) of using the MySQL's UUID over PHP's uniqid( $siteurl, true ).

#25 in reply to: ↑ 24 ; follow-up: @Denis-de-Bernardy
15 years ago

Replying to azaozz:

Perhaps we are reading it differently. The question that wasn't asked there is: what are the advantages (if any) of using the MySQL's UUID over PHP's uniqid( $siteurl, true ).

http://en.wikipedia.org/wiki/UUID#Random_UUID_probability_of_duplicates

#26 @scribu
15 years ago

How about we just store the temp ID of the parent post in a custom field for each attachment?

#27 follow-up: @Denis-de-Bernardy
15 years ago

Works too. It might cause issues if several open windows related to the same draft are open at the same time, for whatever reason, and concurrent saves occur.

#28 in reply to: ↑ 27 @scribu
15 years ago

  • Cc scribu@… added
  • Owner set to scribu
  • Status changed from new to accepted

Replying to Denis-de-Bernardy:

Works too. It might cause issues if several open windows related to the same draft are open at the same time, for whatever reason, and concurrent saves occur.

Drafts don't have temporary ids, so they're not affected by this bug at all.

AFAIK, every time you load the Add New panel, you get a different temporary id, so it's a non-issue.

I'll start work on a patch.

#29 @scribu
15 years ago

  • Component changed from Autosave to Media
  • Keywords blocker removed
  • Severity changed from major to blocker

#30 follow-up: @Denis-de-Bernardy
15 years ago

Actually, the ticket is all about drafts.

Disable javascript, double shift-click the new post button, so as to get the same temp ID on separate posts; open the uploader in a new tab each. Save all four in a very short lapse of time to get a race condition, and see how it goes with your patch on a remote server (localhost will likely work fine no matter what).

With the UUID approach I'm suggesting, I'm sure it'll work, since the two posts will have two separate UUIDs. With your approach, I think it should as well, since the logic behind would be mostly the same, and the odds are tiny to get a race condition indeed. But there still is a risk.

#31 @Denis-de-Bernardy
15 years ago

Just to clarify, I think it's an acceptable edge-case. Any improvement to the current behavior in 2.8.1 is good to take imo.

#32 in reply to: ↑ 30 ; follow-up: @scribu
15 years ago

Replying to Denis-de-Bernardy:

Actually, the ticket is all about drafts.

For clarity, it's about the transition from status 'new' to status 'draft'.

Disable javascript, double shift-click the new post button, so as to get the same temp ID on separate posts; open the uploader in a new tab each. Save all four in a very short lapse of time to get a race condition, and see how it goes with your patch on a remote server (localhost will likely work fine no matter what).

With the UUID approach I'm suggesting, I'm sure it'll work, since the two posts will have two separate UUIDs. With your approach, I think it should as well, since the logic behind would be mostly the same, and the odds are tiny to get a race condition indeed. But there still is a risk.

I openened two tabs with the Add New panel and I did get the same temp id.

Maybe this should addressed in a separate ticket?

#33 in reply to: ↑ 32 @Denis-de-Bernardy
15 years ago

Replying to scribu:

Maybe this should addressed in a separate ticket?

Imo, no, because it's really the whole same issue in the background: assigning a unique ID to new posts as they're created, even if they're not stored in the database...

#34 @Denis-de-Bernardy
15 years ago

Adding to this, if you get the logic right with the temp_ID as a meta, it's trivial to switch to use UUIDs in 2.9, after we bump the MySQL requirements.

@scribu
15 years ago

use postmeta to save the temp_ID

#35 @scribu
15 years ago

  • Keywords has-patch added; needs-patch removed

_relocate_children.diff seems to work with and without autosave.

Now we just need to make the temp_ID unique.

#36 @scribu
15 years ago

  • Keywords needs-testing added

I looked at how and why the temp_ID is not unique and it seems it will have to be addressed in another ticket, since it's not trivial to change.

#37 @hakre
15 years ago

related: #10456

#39 @hakre
15 years ago

Some Feedback: Many of the users of installs we support run into that problem especially if using galleries. Because of the nature of galleries (a gallery is a post with attachments), they start with uploading the images without putting in a title first. therefore they get stuck after their upload finished.

it might be adviseable to save the post while the first upload is triggered? whouldn't that solve this easily?

#40 @azaozz
15 years ago

This is possible but all has to be done while opening the uploader (saving the post as draft, reloading the page or updating the ID with AJAX and then opening the uploader). It may be done with js but is not straightforward. And this should happen on the dashboard too for QuickPress.

Was thinking of more generalized solution, perhaps having an empty "auto-draft" for each user that will be opened when visiting the write screen or the dashboard. If the user doesn't save the draft it will be reused next time. When it gets saved or published (i.e. doesn't exist) new one would be created again. That way all negative post IDs will be obsolete.

#41 in reply to: ↑ 8 @alxndr
15 years ago

Replying to scribu:

I think it would be safer and easier to change the db schema to allow negative integers.

I did this, also taking Denis-de-Bernardy's advice and sticking with bigint(20), and it seems to have worked (knock on wood).

SQL:

ALTER TABLE wp_posts MODIFY post_parent BIGINT(20) NOT NULL DEFAULT 0;

#42 @scribu
15 years ago

  • Milestone changed from 2.8.5 to 2.9

#43 @iwords
15 years ago

Some more feedback, as it doesn't seem that this point has been recognised: currently, it is impossible to attach an image to a post when using QuickPress on the Dashboard. Since QuickPress doesn't appear to have an autosave feature like the full-blown editor, an ID isn't generated until the post is either saved or published - at which point the post's content is cleared from the QuickPress editor, so you can no longer attach an image to it anyway.

Currently, the only way to attach an image to a post written in QuickPress is to save it as a draft, then re-edit it using the full editor, which somewhat defeats the object of using QuickPress.

Just to clarify - yes, you can upload images and insert them into the *body* of the post. But you can't upload images and have them stored as attachments using QuickPress at all.

#44 @scribu
15 years ago

iwords, please open a separate ticket for Quickpress.

#45 follow-up: @azaozz
14 years ago

How about changing:

$temp_ID = -1 * time();

to

$temp_ID = time() . '00000000';

Don't think any site would ever have over a quintillion posts... Then the new attachments will be attached while saving the post. This would work for #10521 too.

#46 @scribu
14 years ago

  • Cc scribu@… removed

#47 in reply to: ↑ 45 @scribu
14 years ago

Replying to azaozz:

That could work, but we have to be careful about wp_write_post():

if ( time() + $temp > 86400 ) // 1 day: $temp is equal to -1 * time( then )

#48 @scribu
14 years ago

  • Cc scribu@… added

#49 @scribu
14 years ago

  • Keywords tested added; needs-testing removed

Come to think of it, changing the temp_ID to a positive number only solves the part of the problem addressed by _relocate_children.diff, but doesn't solve the potential race condition.

#50 @azaozz
14 years ago

Could probably do

$temp_ID = time() . rand(10000000, 99999999);

But still this is only a workaround.

#51 @scribu
14 years ago

Possibly related: #11145

#52 in reply to: ↑ 25 @scribu
14 years ago

  • Keywords commit added

Let's review:

_relocate_children.diff simply saves the temp_ID in postmeta, instead of wp_posts.post_parent.

This fixes the current problem with attachments getting 0 as post_parent.

At the same time, it doesn't introduce any regressions, as the race condition mentioned by Denis-de-Bernardy would have occured before the schema was modified.

Furthermore, changing the way $temp_ID is generated would cause problems with autosave collisions etc.

I say we go with _relocate_children.diff until a more robust ideea emerges.

#53 @miqrogroove
14 years ago

  • Cc miqrogroove@… added

#54 @automattor
14 years ago

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

(In [12298]) Save parent temp ID for unattached attachments in postmeta. Props scribu. fixes #9471

#55 @Denis-de-Bernardy
14 years ago

  • Keywords bug-hunt added
Note: See TracTickets for help on using tickets.