documentcloud / wordpress-documentcloud

Embed DocumentCloud documents that won't be eaten by the visual editor
https://wordpress.org/plugins/documentcloud/
GNU General Public License v2.0
9 stars 14 forks source link

Changes to `wide_assets` and `documents` post metadata #22

Closed reefdog closed 9 years ago

reefdog commented 9 years ago

When a post is saved, we store a couple pieces of metadata:

We have to make at least one change: both pieces of metadata are stored as hashes keyed by document slug. But now that we support notes – which are children of documents and thus whose URLs contain the document slug – we can have two embedded resources on the same post with the same hash key.

My recommendation:

  1. Determine if documentcloud documents post meta is necessary (@eyeseast?) and if not, remove it
  2. If resource is a note, include note ID as part of hash key to distinguish from documents.
reefdog commented 9 years ago

Sorry; the first bit of metadata is documents, not documentcloud.

reefdog commented 9 years ago

So, realized that no matter how many embeds are on a post, only one documents and wide_assets gets created, keyed by the last embed. (Note how the update_post_meta() calls are outside the foreach() loop in save().) Which means we don't have to worry about colliding IDs, because only the last man standing is saved anyway.

Is this by design?

eyeseast commented 9 years ago

The idea with wide_assets was that it only needs to be set once per post. It's a boolean, there's either something wide or there isn't, and having anything wide in the post means it needs to deal with that.

I'm trying to remember what the $documents key was for. It's an array, so it makes sense that there's only one, but I can't remember why we were saving it. Maybe just because we could. (This was four years ago now.)

reefdog commented 9 years ago

I'm inclined to remove documents unless we can figure out its purpose. What do you think?

Grokked on wide_assets.

eyeseast commented 9 years ago

I think it's fine to dump $documents.

aschweigert commented 9 years ago

that seems ok to me too

reefdog commented 9 years ago

Cool. I'll remove.