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

[aesqe]

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

[azaozz]

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.

[Denis-de-Bernardy]

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

[hakre]

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.

[hakre]

init could be named _new.

[Denis-de-Bernardy]

whatever we do it can wait until 2.9

[Denis-de-Bernardy]

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

[scribu]

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.

[scribu]

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

[scribu]

Make post_parent accept neggative values

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

see also #10217 (gallery not saved)

[Denis-de-Bernardy]

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

[Denis-de-Bernardy]

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...

[Denis-de-Bernardy]

until the ajax returns a result, even...

[hakre]

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.

[Denis-de-Bernardy]

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.

[hakre]

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.

[azaozz]

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.

[Denis-de-Bernardy]

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.

[hakre]

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

[Denis-de-Bernardy]

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).

[azaozz]

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.

[Denis-de-Bernardy]

@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. :-)

[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 ).

[Denis-de-Bernardy]

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

[scribu]

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

[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.

[scribu]

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.

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

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.

[scribu]

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?

[Denis-de-Bernardy]

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...

[Denis-de-Bernardy]

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]

use postmeta to save the temp_ID

[scribu]

_relocate_children.diff seems to work with and without autosave.

Now we just need to make the temp_ID unique.

[scribu]

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.

[hakre]

related: #10456

[Denis-de-Bernardy]

see also: #10521

[hakre]

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?

[azaozz]

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.

[alxndr]

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;

[iwords]

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.

[scribu]

iwords, please open a separate ticket for Quickpress.

[azaozz]

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.

[scribu]

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 )

[scribu]

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.

[azaozz]

Could probably do

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

But still this is only a workaround.

[scribu]

Possibly related: #11145

[scribu]

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.

[automattor]

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