Allow to preview content in an actual live environment

Implementing this properly will require an upgrade to how we store temporary data.

Right now, in Drupal, we store temporary data in a cache by caching the generated form. In order to preview, we have to process that form and reassemble data. This is fine when you click preview, but in a system like this, you're actually visiting a different URL from the form.

Logically, we could put the form build id in the URL and load the form and process it, but there are several weaknesses with that:

1) It is wasteful (processing a form that was not submitted)
2) Form build IDs are sha generated strings. There is no way to reproduce it if you don't have it. This means you can't do content locking.
3) It continues the trend on Drupal of the form owning the data when the form typically should only be a representation of the data.

If you're familiar with Views, you've probably seen that once you start editing a view, the view is 'locked' to prevent cross editing from another user. This is something we absolutely should have with content as well. Right now, the only protection we have is that if someone else actually submits an edit before you, and the timestamp changes, your submission will be rejected. This is, however, *after* you've made all your edits and your edits are rejected, whereas the Views method happens *before* you've made your edits so you do not waste your time.

This is accomplished by using a separate non-volatile cache (and thus it's not truly a cache) for temporary storage which is tied to the session ID. (Note: Tying to the session ID is something I would loosen in core). When editing begins, this record is created; the existence of that record for a given view is considered a lock. A mechanism is provided to break stale locks, and locks > 14 days old are automatically wiped by cron.

The actual implementation of the underlying layer is very simple: http://drupalcode.org/project/ctools.git/blob/refs/heads/7.x-1.x:/includ...

I don't use a simple key-value store (i.e, CMI) for this because that would make it much more difficult to do the locking aspect. I personally think the locking aspect is very important. Joomla! has had content edit locks since the day I first looked at it, back in 2006. They were clumsy at the time, but even then I thought that such things were essential, which is why Views have them today and I have never once regretted that decision.

IMO, to implement the above preview, we should implement something like my object cache system in core and implement a layer directly with entities so that all entities, everywhere, can automatically have content edit locking with no more difficulty to implement than editing the entity.

At my core conversation in Denver I suggested that if core accommodates "forward revisions" (saved revisions that are newer than the vid in in the node table) we may no longer need previewing tied directly to the form system. Content authors could just look at the saved node revision.

Agentrickard has a patch that starts to implement forward revisions. #218755-106: Support revisions in different states

That being said, getting CTools-style object/form caching/locking in core would be great too.

I personally would prefer the CTools approach because it would also solve the "Oops I left the page" problem. I'll never forget the time I was teaching a class with 8 computers plugged into a powerstrip that blew right in the middle of everyone configuring views. Everyone was immensely impressed that Views could pick right back up where they left off after the whole class just had their computers shut down on them. Using the temporary object store approach would also give us the locking and auto-saving possibilities that Views has had for years. Not to mention contrib could use it too.

Forward-saving revisions could also solve the preview problem, though the niceties and reusability of the object store make me lean towards that approach. From a usability perspective, I don't think "Preview" and "Revisions" is something that people will immediately be able to make a connection between. I wouldn't expect a preview to create a revision, since I haven't saved anything yet.

I'll note that Views has a bit of an advantage because it has a LOT of very small forms where content editing will be just one form. That said, auto-save draft with the object cache back-end would be trivial with an AJAX timer hook that fires every few seconds, checks to see if any gadget on the form has changed, and does an auto-update-draft in the background and prints a little update saying the last time the draft was saved.

Wordpress does this and it's by far one of their best content editing features.

I lile the idea of ctools in core, and I *love* the idea of fixing previews :) +1 for everyone working on this!!!

I have a hard time understanding how the CTools cache and lock system actually gives us better previews. When I create a new View with a page display at '/my-new-view' I can't actually go preview how that view would work at mysite.com/my-new-view; I have to Save (essentially "Publish") the View before I can do that. The mockup in the original post shows interactions with menu links and blocks (aka how does this node look when actually on my site?), which I don't understand how that would work with cached form data and how the rest of the Drupal parts would be expected to work with that data when it doesn't "really" exist. I can see how it would work with a 'Save and preview' button saving an unpublished revision and actually letting you go to /node/1 to see how it interacts and fits.

Auto-saving and content locking would be great improvements for content editors to have but seem kinda unrelated to the actual issue of improved previews? I'm also not trying to say that either way is correct. I'm just trying to ensure we're asking a bunch of questions first about how it would fit together.

Side note: I would imagine that if improved previews were powered by revisions, that most of the revision aspects would actually be hidden to the end users by default (only if revisions are "actually" enabled for a content type). For example, the 'Save and preview' button would save a draft, unpublished revision. Editing that node would continue editing that draft revision or be silently creating new revisions on each edit. Clicking the 'Save and publish' button would publish the current revision being edited. Using revisions uniformly across multiple entity types requires a *lot* of cleanup to revisions in core (very hard-coded to nodes), but it may be more worth it in the end by improving revisions and better tying it together in the UI.

I have a hard time understanding how the CTools cache and lock system actually gives us better previews. When I create a new View with a page display at '/my-new-view' I can't actually go preview how that view would work at mysite.com/my-new-view;

Revisions don't give you this either.

The mockup in the original post shows interactions with menu links and blocks (aka how does this node look when actually on my site?), which I don't understand how that would work with cached form data and how the rest of the Drupal parts would be expected to work with that data when it doesn't "really" exist.

It's not showing that. It's showing how node/XXX will work with the edited node instead of the original, with a widget that lets you pick the view mode. The blocks that will appear there will appear on the preview. Also, 'revisions that are not the current revision' don't really exist any more than cached form data, so your argument is actually a little off base, in that it's positioning one as wildly different than the other.

Auto-saving and content locking would be great improvements for content editors to have but seem kinda unrelated to the actual issue of improved previews?

And so are forward-revisions. If we're going to talk about strictly required features for the preview presented, then revisions shouldn't even be on the table. Why is my pet feature less valid than the pet feature you are in favor of?

Let me also add:

1) A LOT of fixing of revisions would have to happen in order to have forward revisions working. For example, revisions are very node specific. They would have to be genericized. You can currently only save the current revision -- field API would have to be updated to let you save arbitrary revisions. (Note: Both of these things should happen regardless.)
2) This would leave out any entity that does not support revisions (either because they do not want or do not need them).
3) I doubt the ability to make revisions invisible.
4) Failed editing could lead to a proliferation of revisions -- typically in this kind of workflow (i.e, Wordpress which is where some of the inspiration is from) you'll "change your mind" about revisions and cancel. But in some proposed systems, revisions are never deleted. They'd just hang around. Need to be careful about that.
5) No matter how much you want to make the UX easy, I don't think it would be. Once you start using revisions this way, it becomes intrinsic to their nature when they are administered.

EDIT: My bad, 3 and 5 are basically the same. I've had a little to drink. Sorry. :)

I think I failed to express my primary concern: how does previewing work for *new* entities that do not yet have a node ID, have any of its data saved in the database, none of its menu links saved, etc? I totally understand how a CTools form cache system would work with existing entities and it makes sense to me. But when you hit the 'Preview' button on a new node, you can't go to node/1 - it does not exist.

Temporary data doesn't need an entity id. Typically I use a key that means "there is no id assigned to this data yet". In fact, this works better than revisions when there is no entity id; you cannot have a revision without an entity id, since the key is compound. The object cache has no such limitation.

Also, it's not a form cache. It's an object cache. You store a fully formed entity object in the cache. The form is merely what you use to transform it.

Solid stuff gentlemen :) It seems both approaches would bring us (much) more than "just" fixing previews.

From what I gather, the CTools way brings us more directly to real previews whereas improved revisions sounds a bit more forced, not really directly tied to the previews use case but could-be-used to make it work.

Are these options mutually exclusive? Obviously we'd need only one way to fix previews, but that doesn't imply we don't want both better revisions *and* have the "niceties and reusability of the object store", right?

I hope this discussion can focus on what's best for getting good previews in core.

These options are definitely not mutually exclusive. From the arguments made above, a CTools-style object cache is the better candidate to improve previews and it's help with auto-saving would be a huge win too.

Improved revision handling should be done too but it does not need to impact core previewing.

So to move forward, where should CTools-cache-in-core start? There's a meta-issue tracking entity changes. #1346204: [meta] Drupal 8 Entity API improvements One of the main changes is converting entities to classed objects. Merlinofchaos, does that make caching any easier/harder/irrelevant? Or should this be done at a level deeper than entities so that the same system could cache entity objects or CMI objects?

I agree with stevector; I think both setups are extremely useful, and at the end of the day I think it'd be awesome to be able to put them together. Fixing revisions so you can use them for more than just history is valuable all on its own, and having temporary data so you can lock and better support multi-step content forms would be a bigger win now.

I don't think converting entities to classes matters too much. As I see it, the steps forward go like this:

1. Analyze the object cache as it is now and see what improvements it needs for core. Since Core is transitioning to more OO, I was thinking it could be rewritten as an OO layer which would allow us to make use of PSR and streamline the API somewhat. (It could also hamper the API for OO unfriendly people. I want to discuss this one with Larry and a few others first.)
2. Figure out how to implement that toolbar. There are some questions about it: What is it, really? IS it specific to preview? I suspect this is a secondary toolbar that is part of toolbar itself that allows us to put some resource specific 'things' in the toolbar, which include links and a form.
3. Get a patch that gets that into core, with tests. The current implementation in CTools has tests. Now, we have the problem that firstphase doesn't actually use the system right away, but because it's generically useful I can only hope that catch thinks this is okay. If now we'll keep rolling into the next phase.
4. Integrate entities to use the object cache for new and existing entities; update the preview path to something completely different. This is all one big patch probably. We have to figure out the actual path the preview will take. Is it node/XXX with a query string indicating preview mode? IS it a totally different path? And how do we handle a new node? That part will be particularly tough, though I believe we could put a special XXX key that says "not-yet-existing entity" and have entity_load() automatically handle it.

That's what I can think of offhand.

Looks like a plan. Then point 1 is the next step for the code, point 2 is the next step for the design. I propose merlinofchaos & friends do the code discussion and ux team works on a more fleshed out design/prototype :)

Thanks.

I think both setups are extremely useful, and at the end of the day I think it'd be awesome to be able to put them together. Fixing revisions so you can use them for more than just history is valuable all on its own, and having temporary data so you can lock and better support multi-step content forms would be a bigger win now.

Yeah, this captures my feelings pretty well, too. I don't think this is an either/or situation. I think it's an "and" situation. "Future revisions" solves one ginormous class of problems, but in terms of both new entities, as well as the "Oops, my cat just stepped on the power strip" situation, the object cache seems like a good direction for solving the preview stuff.

Can we make sure there is (if there's not already) an issue about allowing forward revisions in core? Because that's also really important. #218755: Support revisions in different states is at 110 comments and has no issue summary so I can't quite tell if that's it or not.

Oops. Didn't mean to cross-post.

Is this issue a duplicate of #1167242: Expected a realistic preview of changes.?

A discussion has been going on there for a while, and from a design perspective it seems to have come to a similar conclusion as this one (full page preview with a backlink in the upper left, similar to the block region demo page)...

I have marked #1167242: Expected a realistic preview of changes. as a duplicate. Although it came first, this thread seems to have more direction.

@Bojhan. I like your mockup in #1, I'm not sure it needs the 'full node' select list, especially with the term 'node' in it. I also think it needs to be clearer that this is a preview. Imagine the use case that someone aims to click save, but clicks preview by mistake. They get this page which looks like their post is saved and looks good. So they close their browser and loose their work. For this use case I also think there should be a save button on the right of the preview bar.

Note that the following is totally out of scope for the initial patch, but.

We should probably anticipate if not core then at least contrib wanting to stick more options on the preview bar. For example, preview as a given role, preview in a given language, preview as a user from a given ip2geo location, etc. That one drop-down could quickly become 7-8 drop-downs, some checkboxes, a calendar widget, and a partridge in a pear tree. How do you anticipate the design dealing with that?

We don't since we have to protect developers, builders and end users from getting calendars and pear trees in there :)

At least a "theme" selector would be needed for multi-themed sites

My 2 cents : Have we considered using an iFrame to preview content directly from the node form ?

With revisions, in the back of my mind I'd been considering the possibility of google docs-style autosave with revisions (so you can get a list of changes and roll back to specific ones either during editing or after), so I'm not sure it's poorly suited to previews necessarily except for the content ID issue (at the very least it'd be necessary to reserve an ID when preview is clicked). However as merlinofchaos says there's a tonne of work necessary to get close to doing that, and no-one's working on it at the moment. #1082292: Clean up/refactor revisions is an issue just for fixing some obviously broken things with revisions without adding any new features, I haven't had time to get to it, but it's a lot more focused than the 'different states' issue.

I haven't reviewed the ctools object cache code (or at least not for a long time), but in principle I'm not opposed to trying to get that into core at all - if we end up improving revisions for entities, there's still potentially lots of other forms that could benefit from it that aren't entity based at all.

Thanks catch. As for #1082292: Clean up/refactor revisions, looks like real progess is being made in #218755: Support revisions in different states

I had a first stab at an issue summary, using merlinofchaos's list in #16.

Is CTools object cache flashed when all caches are cleared? Would this flush any auto-saved data too if they were based on that?

No, they are not. They flush stale data after a set amount of time but they are non-volatile and are not cleared with normal caches.

It's not truly a cache. When I rewrite it for core inclusion I think I will call it a TempStore instead.

Thanx for taking the time to explain this Earl. The reason I asked was because I could grasp (roughly) how this would be done with revisions, but not the CTools way.

Anyways, as long as we get this feature in D8, I don't really care how it's done. I guess whichever way is easier/faster for the people that'll code it + maintain it afterwards. I agree with others though that revisions should receive a major overhaul anyways, but that's a matter to be discussed in #218755: Support revisions in different states, #1082292: Clean up/refactor revisions and elsewhere - not here. Same goes for thoughts about implementing auto-saves, and supporting revisions for all entities too. They too deserve their own separate issues.

...sorry.

There is no patch here, so hence we set the status to active.

It's not truly a cache. When I rewrite it for core inclusion I think I will call it a TempStore instead.

Which I believe we should also use instead of the for the extremely abused "cache_form" table, which isn't really a cache either and also shouldn't be flushed with cache clears.

Any issues filed so we can keep track of these tasks then?

Crickets ...

crick·et
/ˈkrikit/
noun

1. An insect (family Gryllidae) related to the grasshoppers. The male produces a characteristic rhythmical chirping sound.
2. An outdoor game played on a large grass field with ball, bats, and two wickets, between teams of eleven players, the object of the game...
3. A low stool, typically with a rectangular or oval seat and four legs splayed out.

I've gotten a start on the TempStore object. I haven't created an additional issue (perhaps there should be one) and I haven't had a chance to finish it. There's a lot tugging on my time right now, but it's important to me for many reasons.

Early TempStore patch posted here: #1642062: Add TempStore for persistent, limited-term storage of non-cache data

TempStore is in, this should use that!

Hah, cool. I was just about to bump this issue for a status update. Good news!

I guess people might want to know where to find the latest code for this TempStore and any pointers on how to apply it here would be welcome too. People will want to read comment #16 and #30.

Here's the change notice with some examples: http://drupal.org/node/1805940

Can you explain why this is more than a feature request?
Also, the issue summary is fairly out of date.

I believe it's categorized a task because it causes pretty drastic usability issues, as seen in the Google UX study earlier this year. However, it's clearly a new feature, so this is sort of a gray area.

I updated the summary a bit to recommend recent core API additions.

Some considerations:

• Hook invocations. Since the entity isn't saved yet, hook_entity_insert() or hook_entity_update() hasn't fired. Which is by design I think, but might affect how actual the actual content preview is.
• Access. The aforementioned unfired hook invocations might change the entity's access restrictions. So we probably want to restrict access to the preview to the owner, or perhaps administer content for nodes.

Would it also be possible to make a preview for different devices i.e mobile, ipad etc?

First stab at this, needs a lot of work, but could use a lot of reviews/tips on how to implement this cleanly. Still lots of things to cover like images, tags (and more stuff along the road I guess).

Note that I won't dedicate full time on this, so feel free to hack away at this at all times, assign yourself or ping me on IRC in case so we don't do double work :)

Attached a screenshot of how it currently looks like - the link to go back is at the top left.

Note, the uuid is available, will use that as the tempstore_id in the next patch

New patch: uses uuid, you can now switch view mode as well, css needs love :)

I doubt that calling drupal_render() is the way to go here. In general we want to return a render array and let drupal_render_page() do its thing.

Also noticed a variable thats not used: $trimmed = drupal_render($elements);.

Thanks!

Great progress! Interdiffs would be useful though :)

Page callback now returns render array, $trimmed variable is gone, css been updated (copied action-button from 7 basically).
Sorry for not posting interdiffs so far, probably after this one as it becomes more mature.
Oh and Wim, I need a way to tell the edit module to not fire on the preview page, see the temporarily extreme hack in the patch right now ;)

That's all for today, more this weekend.

Quick re-roll after #1838114: Change node form’s vertical tabs into details elements went in. Now *really* off.

Last one - cleaner check for edit module by setting $entity->in_preview to true in preview callback and checking on that in edit_preprocess_field()

#60: Something like this should work:

hook_toolbar_alter(&$items) {
  if (isset($items['edit']) {
    unset($items['edit']);
  }
}

This version fixes the newline in the css file and makes tags work again when reloading the content

// @todo can we do this somewhere different ?
$tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');
if (!empty($tempstore_id) && ($data = node_tempstore_load($tempstore_id))) {
$node = $data;

+ // We have to prepare the node to fix the autocomplete tags widget.
+ // @todo now we do a double prepare... Is that allowed? Probably not...
+ node_invoke($node, 'prepare');
+ module_invoke_all('node_prepare', $node);
}

Patch is crap. Will reroll...

Second try....

There is also a problem with query redirects. When we have a redirect in the query we should ignore it when pressing the preview button. We don't ignore it at the moment :).

You can test this by editing a node through contextual links. After doing that you get on the node edit form. But when you press preview you get redirected back to the node page.

PS: great... ignore the dpm for now...
PS2: suddenly tags are broken again :s something weird is going on

Ok some more digging:

My patches above are incorrect they just seemed to work because I was dealing with existing terms.

Problem:
When defining new terms in an autocomplete, they are not yet saved, they get a tid "autocreate" when going to the preview.
When going back to the node form all the "autocreate" tags can't be loaded so they don't get rendered.

This feels like a general issue with references that don't exist. And probably needs a general solution.
I have no idea how the autocomplete system works but this seems to me the only situation where this could happen.
If this is a central system it would be cool if we could fix it there.

But I fear this needs a unique solution for each entity that is facing this problem.

Dirty workaround for now. :)

This patch should turn back green, removed some todo's as they are actually ok, did some fixes for node preview titles etc. I think after this, it's time for interdiffs. I've asked a colleague frontender to look at the css and the javascript this weekend.

- edit - note that I don't necessarily consider the code for taxonomy dirty, it's just the way that its autcomplete feature works, however, optimization is always possible of course .. :)

Nice progress — the code already looks much better!

+++ b/core/modules/node/node.pages.incundefined
@@ -121,11 +121,15 @@ function node_add($node_type) {
+  // @todo move access check earlier ?
   if (node_access('create', $node) || node_access('update', $node)) {

We can move the access check to the hook_menu (in stead of returning true). The doc has to be changes as we return a render array now. And not pure html.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/widget/TaxonomyAutocompleteWidget.phpundefined
@@ -52,7 +52,13 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+      $tid = $item['tid'];
+      if ($tid == 'autocreate') {
+        $tags[] = $item;
+      }
+      else {
+        $tags[$item['tid']] = isset($item['taxonomy_term']) ? $item['taxonomy_term'] : taxonomy_term_load($item['tid']);

I should have used $tid everywhere here

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -950,8 +950,17 @@ function _taxonomy_get_tid_from_term(Term $term) {
+    if (is_array($tag) && $tag['tid'] == 'autocreate' && $label = $tag['name']) {
+      // Commas and quotes in tag names are special cases, so encode 'em.
+      if (strpos($label, ',') !== FALSE || strpos($label, '"') !== FALSE) {
+        $typed_tags[] = '"' . str_replace('"', '""', $label) . '"';
+      }
+      else {
+        $typed_tags[] = $label;
+      }

I considered this dirty because it is duplicate code. We can refactor this to be a bit cleaner.

Used an access check now, changed doc block and changed the tests because of the way that the testbot runs (in a subdirectory).

I haven't touched the term workaround yet and I think we need to think about one important thing and that's how we're going to deal with the tempstore when anonymous users have access to edit nodes, because then $node->uuid won't be enough.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -26,6 +26,16 @@ class NodeFormController extends EntityFormController {
+    // Get the tempstore id.
+    $tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');
+    if ($tempstore_id) {
+      $node->tempstore_id = $tempstore_id;
+    }
+    else {
+      $node->tempstore_id = $node->uuid;

We can probably just remove the tempstore_id property completely and use the uuid. So replace all tempstore_id everywhere with uuid.

+++ b/core/modules/node/node.moduleundefined
@@ -2696,6 +2808,24 @@ function node_node_access($node, $op, $account) {
+  if (node_access('create', $node) || node_access('update', $node)) {
+    return TRUE;
+  }
+  return;

return FALSE instead of 'return;'

- edit - we also need a test for switching the view mode on the preview page and maybe also a 'clickLink' on the button at the top.

Moshe also suggested to use a lock mechanism like views does, so we can finally change the mechanism where you'd potentially get a 'This content has been changed by another user' when trying to save a node. Basically create the tempstore object immediately after creating on node/add or node/x/edit and then checking the expire time we can add a message like 'This node is being edited etc'. This would solve the problem even for anonymous users.

- edit 2 - after actually checking the user.tempstore class the user with aspilicious this should actually be *really* easy and probably working fine for everyone :)

However, I'd leave that for a follow up, although I don't think it would be *that* much code.

Some cleanups, added tests for view mode switching and simplified some tests by using clickLink() method.
I was wrong re: removing the tempstore_id property, we actually do need it.
Not sure also why Drupal\system\Tests\Form\RebuildTest failed, works perfect on my local machine.

@Bojhan - The font comes from Bartik because we are in that theme.
Also, do you have an idea how we would handle locking nodes ? The idea is that when a user tries to edit a node which is edited by someone else would get a warning that this one is locked and can potentially (with permission) can break that lock, so just like views. We can do that in a follow up if/when (crossing fingers) this goes in.

The locking of nodes isn't actually even in the OP, though it could be built on this.

This is just about a true preview, not just "your words maybe run through a text format".

Node destination redirects are working again. Not sure if this is the best we can do.
Cleaned my taxonomy code a bit.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -86,6 +86,14 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
+      // Restore the destination if we previewed the node
+      if (isset($node->destination)) {
+        $form['node_destination'] = array(
+          '#type' => 'hidden',
+          '#value' => $node->destination,
+        );

Missing dot in the comment. Let's use #type 'value' instead of hidden.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -391,6 +399,17 @@ public function submit(array $form, array &$form_state) {
+    // In case the destination parameter is set

Missing dot

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -950,7 +950,18 @@ function _taxonomy_get_tid_from_term(Term $term) {
+      // Make sure we have a completed loaded taxonomy term.

complete ?

- note - we'll upload a new version with css and js changes as well

Updated some css to make the preview look like the provided design. Also updated the node.preview.js to disable all links except for the 'back to content editing'. Swentel addressed soms issues mentioned in #87.

I would be concerned that actually disabling links will be a confusion for users. Links that are disabled need to be visually identified, can the whole page except preview and the button be greyed out for example? Or JS warning attached to clicking somewhere else?

You can add an invisible overlay. For inspiration on how that could work, see core/modules/edit/js/views/overlay-view.js (if memory serves me well), along with the corresponding CSS. Instead of making the CSS white out everything, just make it transparent instead and ensure the z-index is lower than that of both the toolbar and your "back to content editing and view mode" bar. Now you can very simply intercept every click event; but at the cost of no longer having :hover events on the previewed content.
I'm not sure if that's the best approach, but I think it's a step in the right direction, at least for now.

I'd vote against a greying out or anything else, because that would prevent the preview from being a proper preview.

I'm confused why add a new layer if the patch already have a way to prevent clicks to work.

My comment wasn't specific to the disabling clicks. My concern is that disabling clicks will be a UX disaster unless we give users some visual indication that they can only click the Back button (either before they click or after they click)

2) If you click away from the screen you can't go back to edit the node. Unless you know the UUID...

It's a bigger WTF to edit a complete node, go to the preview and loose everything because you clicked a link.

#91: Apologies — I didn't know that was already implemented. I was just providing a suggestions as per #90.

#93.1: we could make it so that only links inside the previewed node are clickable. Or potentially add a Google Docs-like tooltip when clicking the link.
#93.2: It doesn't make any sense to click links other than those inside the previewed node's contents when previewing a node. If you disagree, please explain why. So: I disagree it's a work-around for not having an autosaving capability, IMVHO it's common sense.
#93.3: I wasn't arguing for disabling links in the sense that they would no longer be <a> tags, or would get some class attribute. Nothing should look differently. But a user should not be able to start navigating elsewhere, and if he should, then he should explicitly confirm that first.
#93.conclusion: I agree, this the perfect example of a follow-up-able issue.

#94: It's not very clear to me what you mean, but I presume it's a reply to #93.2? Because of the temp store, they wouldn't lose everything, but you'd lose your context, so in that sense I agree :)

Just open a parenthesis on the feature raised by @merlinofchaos about content locking in #3. I use Drupal 6 on an intranet with 1800 users based on Organic Groups and it would be impossible to implement it without content locking.

You probably already know, but worth mentioning, the Content Locking module do it, and do it very well. It is absolutely perfect for our needs and it has the entire workflow cited by @merlinofchaos. It's a great reference.

Autosave is also very welcome and necessary, but it is essential to lock content in a collaborative environment.

(Sorry for my english)

Small update where the font is used from the toolbar, as requested by Bojhan.

I'd say, RTBC on 99, commit on 100 ? :) (and of course follow ups after that)

Remove some redundant code, thanks aspilicious.

Lots of feedback. Boatloads of nitpicks, but also a few big questions, amongst one which is a very fundamental concern — also unanswered by the rest of this issue AFAICT.

+++ b/core/modules/edit/edit.moduleundefined
@@ -164,7 +164,9 @@ function edit_library_info() {
+  if (empty($entity->in_preview)) {

Good catch :)

+++ b/core/modules/node/images/btn-arrow-preview.pngundefined
@@ -0,0 +1,5 @@
+<rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"> <rdf:Description rdf:about="" xmlns:xmpRights="http://ns.adobe.com/xap/1.0/rights/" xmlns:xmpMM="http://ns.adobe.com/xap/1.0/mm/" xmlns:stRef="http://ns.adobe.com/xap/1.0/sType/ResourceRef#" xmlns:xmp="http://ns.adobe.com/xap/1.0/" xmpRights:Marked="False" xmpMM:OriginalDocumentID="uuid:2C69DF84A6C0DF1182FCEF8E87685653" xmpMM:DocumentID="xmp.did:9834D3895B1F11E29836AA070337CF68" xmpMM:InstanceID="xmp.iid:9834D3885B1F11E29836AA070337CF68" xmp:CreatorTool="Adobe Photoshop CS5.1 Windows"> <xmpMM:DerivedFrom stRef:instanceID="xmp.iid:5698AC1B3A7BE11187FEA4A0A56EE5FD" stRef:documentID="uuid:2C69DF84A6C0DF1182FCEF8E87685653"/> </rdf:Description> </rdf:RDF> </x:xmpmeta> <?xpacket end="r"?>

All this metadata should be stripped from the PNG file, but this can be a follow-up commit. UPDATE: considering the loads of other feedback, you may want to fix this now. Optimize it using http://imageoptim.com/ (i.e. which uses PNGOUT, AdvPNG, Pngcrush, extended OptiPNG).

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -26,6 +26,16 @@ class NodeFormController extends EntityFormController {

@@ -26,6 +26,16 @@ class NodeFormController extends EntityFormController {
    * Overrides Drupal\Core\Entity\EntityFormController::prepareEntity().
    */
   protected function prepareEntity(EntityInterface $node) {
+
+    // Check if we can retrieve a node from the tempstore.
+    $tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');
+    if ($tempstore_id) {
+      $node->tempstore_id = $tempstore_id;
+    }
+    else {
+      $node->tempstore_id = $node->uuid;

Does this mean we only support node entities? Is there a reason we can't do this for all entities generically?

Or is that a follow-up issue?

I saw some discussion about this in comments #9 and #11, but it's not explained in the issue summary. So can we please clarify this?

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -381,9 +397,22 @@ public function submit(array $form, array &$form_state) {
+    if (isset($_GET['destination'])) {
+      $entity->destination = $_GET['destination'];
+      unset($_GET['destination']);

Is it still allowed to manipulate $_GET directly? Shouldn't we be using drupal_container()->get('request') to modify this?

+++ b/core/modules/node/node.moduleundefined
@@ -943,6 +939,23 @@ function node_load($nid = NULL, $reset = FALSE) {
+ * Load the node from the tempstore.

the -> a ?

+++ b/core/modules/node/node.moduleundefined
@@ -943,6 +939,23 @@ function node_load($nid = NULL, $reset = FALSE) {
+ *   The id of a tempstore object.

s/id/ID/, no?

+++ b/core/modules/node/node.moduleundefined
@@ -943,6 +939,23 @@ function node_load($nid = NULL, $reset = FALSE) {
+ * @return Drupal\node\Node|false

Leading slash missing.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+

Extraneous newline.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+  $node = menu_get_object('node_tempstore', 2);

Should we still use menu_get_object()?

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+

Another extraneous newline.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+ * @param Drupal\node\Node $node

Missing leading backslash.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+ * @return $form

array $form

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+function node_preview_form_select($form, $form_state, Node $node) {

array $form, array $form_state

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+  // Always add default.

"add default"? Sounds strange.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+  $link = 'node/add/' . $node->bundle();

I understand why you call this variable "link", but wouldn't "path" be more in line with the rest of Drupal?

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+    $view_mode = 'full';

Maybe $current_view_mode instead? That makes the default value setting for the select less weird, twenty lines further.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+    //'#prefix' => '<div class="node-preview-backlink-container">',
+    //'#suffix' => '</div>'

Should be removed.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+ * Submit handler for the node preview select form.

s/node preview select form/node preview view mode select(ion) form/

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+function node_preview_form_select_submit($form, &$form_state) {

array $form, array &$form_state

+++ b/core/modules/node/node.moduleundefined
@@ -2696,6 +2808,24 @@ function node_node_access($node, $op, $account) {
+ * @return

"@return bool" ?

+++ b/core/modules/node/node.pages.incundefined
@@ -119,79 +119,50 @@ function node_add($node_type) {
+  // Set status to true so we don't get the 'unpublished' css.

s/css/CSS/

+++ b/core/modules/node/node.pages.incundefined
@@ -119,79 +119,50 @@ function node_add($node_type) {
+  // Load the user's name when needed.

s/user/author/ ?

+++ b/core/modules/node/node.pages.incundefined
@@ -119,79 +119,50 @@ function node_add($node_type) {
+      $node->uid = 0; // anonymous user

This comment is unnecessary, especially because it's already explained in a comment a few lines higher.

+++ b/core/modules/node/node.pages.incundefined
@@ -119,79 +119,50 @@ function node_add($node_type) {
+  // Property so we can manipulate $page in template_preprocess_node.

Bad sentence, and it doesn't seem to match what the code is doing?

+++ b/core/modules/node/node.preview.cssundefined
@@ -0,0 +1,70 @@
+  position: fixed;
...
+

Extraneous newline.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -950,17 +950,24 @@ function _taxonomy_get_tid_from_term(Term $term) {
 function taxonomy_implode_tags($tags, $vid = NULL) {

Why are changes to this function necessary?

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -950,17 +950,24 @@ function _taxonomy_get_tid_from_term(Term $term) {
+      // Make sure we have a complete loaded taxonomy term.

s/complete/completely/

Is it still allowed to manipulate $_GET directly? Shouldn't we be using drupal_container()->get('request') to modify this?

You're right we should manipulate the object.

Additionally, we need to check the accessibility (and usability, for that matter) of just having a dropdown in the middle of a blue bar that says "Full". Methinks that should have a label of some kind. The feature in general needs to be tested as well to ensure it passes the accessibility gate before it can be committed. Hopefully since this is just largely re-using stuff already in Views there won't be a problem here.

This doesn't need to be a commit blocker, but one thing we should think about is making this preview bar something other modules can easily hook into, so say for example the http://drupal.org/project/ad_geoip module could preview how the node will look if the user comes from Spain vs. Canada, and http://drupal.org/project/browscap could let you preview it on such and such browser, and so on. Right now, implementation-wise, this is highly tied to nodes, and node is an optional module. It would be cool if node was simply implementing hook_preview_bar_alter() (or whatever) instead.

Thanks for the review! :)

• Image has been optimized
• I'm not sure if there's an equivalent for menu_get_object() yet - there are other places in node.module still using it, so leaving as is for now.
• This patch currently only supports nodes indeed. I've been thinking about generalizing this, but let's start here first. I'm willing to investigate this in a follow up issue.
• The code in taxonomy is needed because the format can be different, depending on whether the tag already exists or not. In case it doesn't exist yet, the format on the $entity is not good to be added in the form field.

I've tried removing the 'destination' get parameter with $request->query->remove('destination'); but that didn't seem to stop Drupal from redirecting, so still using unset($_GET['destination']) as drupal_goto() looks directly for $_GET['destination'].

All other things should be addressed and are in the interdiff. Please recheck those extraneous newlines, because it's not always clear for me to find out, but I *think* it's ok.

Re: webchicks remarks: yeah, the hook_page_build() doesn't 'feel' right, it would be more awesome to have more flexibility there and also the ability for other modules to alter the node before preview as well.

Reroll after poll is gone and some entity info related changes.

And now fixing good this time, API changes after #1822458: Move dynamic parts (view modes, bundles) out of entity_info()

Novice task for this issue: test the patch against the core accessibility gate. Four parts of the accessibility gate are straightforward to test:

1. HTML structure meets WCAG 2.0
2. Text color has sufficient contrast
3. Form fields are labeled
4. Javascript is keyboard-usable

Choose one (or more) of these points, apply the patch and clear your caches, and follow the instructions provided for each in the accessibility gate information. Post a comment describing your testing and the results here on the issue. Thanks in advance!

@xjm - There's no label on the view_mode form to switch between full & teaser modes.

Took a brief look at the color contrast. With white text & a gradient between #4F9FEA & #4481DC for the background, unfortunately it fails for all but large text (and this isn't large text) - http://webaim.org/resources/contrastchecker/

Other than that it looks fine. Keyboard navigation was fine & http://wave.webaim.org/toolbar didn't complain about anything else.

Will be a good addition when we get those two things cleaned up.

@mgifford

- is adding a 'label', with #label_display = 'hidden' enough, or does it have to be visual ?
- I'm absolutely not sure how to handle that color/contrast thing, never done that honestly, so if you could tell me what todo, that would be fine :)

Also, feel free to reroll yourself of course :)

So this uses #title_display => 'invisible' to add the label for screen readers.
Changed the css to use the same gradient as the action buttons in seven.
The top of this gradient fails the contrast, but the bottom passes.
At least this way if we decide that we need to modify these, we need to modify all of them - and we can search for the rgb.
Also fixed the fall-back background for the preview bar for browsers with no gradient support (was the same color as the button) and removed the underline from the :hover pseudo-class which made the button look like a link (it doesn't do this in seven).

#114: node-preview-1510544.114.patch queued for re-testing.

If I enable comments on the node and go to preview, I see no comment form. If I then go back to editing the comments are back disabled.
Also menu options (and it seems other options).

I am also not sure why I see strange flickering once I click on Preview - the site first opens the page in Overlay and then switches off the overlay.

The overlay flickr is because the preview page callback is not an admin path, so it does not use the overlay. Users without permission to use the admin theme won't see that flickr.
The preview does not belong in the overlay in my opinion, the intent is to use the actual theme, not the admin theme.

The preview does not belong in the overlay in my opinion, the intent is to use the actual theme, not the admin theme.

True, I guess this is the same behavior of the block demonstration page.

I don't think we'll able to get the menu's working easily (although theoretically possible I guess), comments will be a bit easier, however, dataloss when going back is something we'll have to look at.

Comments are slated to become an ordinary field in #731724: Convert comment settings into a field to make them work with CMI and non-node entities so that problem should just go away.
Ideally once the entity reference field lands #1801304: Add Entity reference field and menu links become content entities #916388: Convert menu links into entities theoretically (and ideally) node menu links would become and entity reference field too.

Swentel showed me this today. It already works so naturally, it's basically not exciting anymore! Which is a good thing, because that means it does what is expected.

I think the view mode label should be visible.

This fixes most of the data loss, except for the menu handling.
Haven't been able to get the comment form rendered though.

I'm pretty much offline until monday, so someone else can take over to bring this home.

Awesome work.
I just have one suggestion. Can we have save button next to 'Back to content editing' link? So that if I have done editing I can save content immediately. Maybe we can do it in followup if it is out of scope.

Good idea. It would be a Save dropbutton as per #1751606: Move published status checkbox next to "Save". Could be a follow-up.

Spent some time with this patch this afternoon. It is COMPLETELY AMAZING! I literally laughed out loud when I went back to Drupal 7 and previewed my content. The difference is stunningly remarkable. Screw web services and configuration management, this is the one feature everyone's going to want to upgrade to Drupal 8 for. ;)

The one dark mark on its eye is that #1848210: [Tests] Submitting a form in Overlay shows content + dsm() for a split second before redirecting to front-end theme gives it a really jarring/ugly experience in the Overlay. However, that's a pre-existing bug that pre-dates this patch, and without the Overlay it seems to work great.

I think the last commit-blocker from my side is I feel like we need a label on the view mode selector. I'm not sure what to call it that's not overly technical, but "Full" floating out in space doesn't really make any sense. The existing label "Select a view mode" might be too technical though.

Agreed with jibran that I found my natural inclination while testing this was to want to be able to save right when I'm looking at the page. However, also agreed with Bojhan that it's likely better as a follow-up in case we run into issues. I'd also love a follow-up to discuss how other modules can add their preview doo-dads to this bar. We're working on a mobile preview doo-dad at #1880606: Introduce a configuration UI for theme-based breakpoints and it'd be great to have somewhere to put it.

For the label, how about just "Mode:" or "View mode:" — doesn't seem too technical for a content editor, who should at least understand there are different ways a bit of content will be displayed.

Added #1899360: Add save dropbutton next to 'Back to content editing' link in node preview. follow-up as per #123, #124, #125 and #126.

+++ b/core/modules/node/node.pages.inc
--- /dev/null
+++ b/core/modules/node/node.preview.css

We need some eyeballs on node.preview.css in this patch from a frontend/markup perspective.

Made the exercise on where CSS should be and I'm guessing this the wrong approach. So what probably should happen is that:

• there is is some minimal styling in node.preview.css
• the actual colors etc (maybe more ?) go into bartik somewhere ?

Assigning to sun for review - look I'm learning ;)

Also, in the proposed resolution, there's a mention about moving some things to EntityFormController::prepareEntity(), which would probably make sense - and would make it easier for a generic entity approach.

Hah. Thanks for looping me in. ;)

Interesting issue. To my knowledge, one of the top-notch Drupal agencies scoped a feature like this to require a mere ~$500,000 budget for being developed. Of course, that included to take care of the utter pitfalls and consequences a functionality like this would entail.

The changes to Menu module in this patch pretty much demonstrate the problem space: As long as you assume that you're on your own and on your own isle, you can do whatever you want. But as soon as you take multidimensional modularity into account, any kind of simple idea blows up completely and leaves the overall system in a state we definitely do not want to be in. In turn, that gets you pretty quickly to the aforementioned budget.

This patch does not address that. It ignores the problem space of entities getting enhanced by modules, which, each, have their own storage, and which need to participate in the rendering process, but whereas the rendering process relies on the previously mentioned storage. Worse, the submitted, entity-specific form values can have a significant effect on other parts of the page/site, outside of the rendered entity.

Due to that, it is only a showcase of a hypothetical feature. The feature cannot be implemented in the proposed way, as it would have massive implications on contributed modules down the line.

However, I do think that it would be very beneficial and worthwhile to actually leverage the most recent improvements to the entity system, in order to make a relatively big leap in the right direction for Drupal 8. Details are following below.

So, alas, most of this is not the front-end related review that has been asked for, but instead, a much more significant, architectural review:

+++ b/core/modules/menu/menu.module
@@ -566,6 +566,13 @@ function menu_form_node_form_alter(&$form, $form_state) {
+  // In case we're in preview and there is no mlid yet but a title
+  // has been supplied, trick the menu form to store it's information.
+  if (!empty($node->in_preview) && empty($link['mlid']) && !empty($link['link_title'])) {
+    $link['mlid'] = TRUE;
+    // @todo move all other properties to attributes as well ?
+    $link['options']['attributes']['title'] = $link['description'];
+  }

1) I do not understand what this "trick" is doing, and, which consequences of this manipulation have been considered (and thus are expected) and which have not. Right now, I assume that plenty of consequences were not considered, since this entire feature request is a million-dollar effort in the current architecture, and when accounting for all consequences.

2) I don't understand why the conditional code futzes with the link's attributes.

3) Why is the preview shown in a form in the first place...?

+++ b/core/modules/menu/menu.module
--- /dev/null
+++ b/core/modules/node/images/btn-arrow-preview.png

Arrow buttons are a thing of the pre-HTML5 era.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -26,6 +26,16 @@ class NodeFormController extends EntityFormController {
+    // Check if we can retrieve a node from the tempstore.
+    $tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');

A preview like this, on a separate page, should not call into the entity form controller in the first place. That's view vs. edit.

+++ b/core/modules/node/node.module
@@ -964,6 +960,23 @@ function node_load($nid = NULL, $reset = FALSE) {
+function node_tempstore_load($tempstore_id) {
+  $node = drupal_container()->get('user.tempstore')->get('node')->get($tempstore_id);
+  if ($node) {
+    return $node;
+  }
+  return FALSE;
+}

OK, so here's the utterly funny part about this:

Ever since, I'd argued and preached that we simply need to save content entities, if we want to preview them. Or in more common speak: Autosave.

You're essentially doing exactly that here — merely with the exceptionally complex workaround of going through a custom tempstore to achieve a preview.

The tempstore approach prevents each and everything else from working like it is supposed to work — which essentially circles back into the Menu module change: Any module that integrates with the content entity suddenly has to take extra care for the possibility of a fake-stored entity in a tempstore, and adjust ALL of its code to work with that edge-case.

So here's the essential question:

Why do we even try to store something in a fake storage and invent tons of custom plumbing for dealing with that, instead of actually storing it?

Make "live" previews limited to entities that can be stored and have a publishing status. — No one will complain.

We've significantly improved entity revisions for D8. — The moment you click Preview, you surely have reached a state of art that is worth to backup in a new revision.

If necessary, we can look into "tagging" auto-created revisions later on, so as to prune and clean them up upon final save.

However, the big, important, and essential difference is this:

1) We're a Content Management System. We care for your content, even if you merely hit "Preview".

2) We do not force 13,000+ contributed modules to deal with a (strange) concept of unsaved-but-then-again-saved-but-then-again-not-for-realz entities that may or may not be rendered.

3) Instead, we pave the way to actual, site-wide previews, which actually include entire site sections, content, menu link tree structures, permissions, and whatever else is needed. This is the multi-million-dollar part that will take until D10 to be figured out in total. But at the very least, all modules are able to rely on clearly defined concepts (revisions) in working their way towards the New World Order™.

And of course, everything else related to this, is mostly baked-in already: Permissions to view a (unpublished?) node in preview? A router path to view a node revision? Differences to your last preview? etc.pp.

+++ b/core/modules/node/node.module
@@ -1761,6 +1774,13 @@ function node_menu() {
+  $items['node/preview/%node_tempstore'] = array(

Can we reverse this into /node/%node_tempstore/preview ?

+++ b/core/modules/node/node.module
@@ -2277,6 +2297,103 @@ function node_page_view(Node $node) {
+function node_preview_form_select(array $form, array $form_state, Node $node) {

It looks like this form should be in node.pages.inc.

Or even better, part of NodeRenderController. That is, because this form is attached to a rendered node entity.

That said, the view mode to render the entity in should be part of the router path; i.e., /node/%node_tempstore/preview/%[view_mode]

Or even more appropriate, leave it out of the path, but turn it into a ?view_mode=xyz query string parameter instead.

In turn, this form can be changed to use 'method' => GET, which in turn eliminates the submit handler, and instead, moves the view mode processing into the actual code path that should care for it.

+++ b/core/modules/node/node.preview.css
@@ -0,0 +1,70 @@
+.node-preview-container {
+  background: #d1e8f5;
+  background-image: -webkit-linear-gradient(top, #d1e8f5, #d3e8f4);

Minor, in terms of the overall patch: ;)

As you already figured, any kind of gradient or special background, positioning, and any other fancy visual styling doesn't belong into module CSS. :)

3) Why is the preview shown in a form in the first place...?

It's not, it's basically just like a node/nid page but with the node loaded from tempstore. The only form on that node preview page is the view mode select form to switch view mode, that's it :) The code in NodeFormController() is to figure out if we have something for this node/add node/x/edit page or not.

Or even more appropriate, leave it out of the path, but turn it into a ?view_mode=xyz query string parameter instead.

That's already the case.

Addressed following things

• change path to node/%tempstore/preview
• moved node_preview_form_select to node.pages.inc
• removed the 'dirty' tricky in menu and path- and others. It's not needed because of the new code that disables form_state cache and sets it to rebuild if wanted. This isn't commit/production ready code as it's not perfect, but this should allow us not to make hacks, and contrib modules can sleep nicely as submitted their data is preserved when coming back to the edit screen. Need to digg into form.inc - or if someone with Form API voodoo power can always help along to figure out where we can easily plug this in.

The interdiff is against #114

Related #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -67,6 +67,25 @@ protected function prepareEntity(EntityInterface $node) {
+    // Don't use form cache.
+    $form_state['no_cache'] = TRUE;

I don't think that's possible, since #ajax and a couple of other things rely on the form cache.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -67,6 +67,25 @@ protected function prepareEntity(EntityInterface $node) {
+    // Check if we can retrieve from the tempstore.
+    $tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');
+    if (!empty($tempstore_id) && ($data = node_tempstore_load($tempstore_id, FALSE))) {
+      $form = $data['form'];
+      $form_state = $data['form_state'];
+      $node = $form_state['entity'];
+      $form_state['rebuild'] = TRUE;

That said, this code pretty much resembles what Form API's form cache is doing already... unless I'm terribly mistaken, the entity is cached in there already.

It looks like you should be able to simply leverage the Form API cache by doing this:

node/%nid/preview/%form_build_id

And with that:

$form_state['input']['form_id'] = 'node_form';
$form_state['input']['form_build_id'] = $form_build_id;
$form = drupal_build_form($form_id, $form_state);

(give or take some details)

That said, I still don't understand why this code gets executed in the code path of the entity form.

I think sun raised a good point - wouldn't it be better to simply save the node unpublished? 90 % of Drupal users have some kind of Workflow module installed anyway so would expect this behaviour.

When you PUBLISHED a node and you edit it and you want to PREVIEW your changes the node can not be autosaved because that would destroy the entire point of the preview.

@aspilicious: There have been significant improvements to node/entity revisions in D8 (#218755: Support revisions in different states) and we continue to work very hard on that (#1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text), so what you're saying was the behavior of D7, but with all the entity revision improvements, we should be in a state that allows for that. Unless I'm mistaken, that's what seems to be called "forward-revision" (#1776796: Provide a better UX for creating, editing & managing draft revisions.).

@Sun - I think saving temporary work revisions also has problems. Namely, hooks like to fire during Save operations and all those hooks would need to decide if they want to act on temporary saves. So we have traded off complexity by avoiding tempstore. I don't know which is the worse evil (tempstore or meaningless revisions).

As for autosave, I see that as a different thing, and it is confusing to mention it here. I use the word autosave to refer to client side caching of forms. This form storage is robust enough to survive when you navigate off a form, close a tab, etc. Core patch in progress at #153313: ckeditor input is lost when using the browser's back button

@moshe:

2) There's admittedly some confusion around the term "autosave", potentially caused by the contributed Autosave module. However, all client-side editor library plugins called "autosave" that I know of are actually issuing a HTTP request to the server, in order to simply save the content being edited by the user on the server-side, because the client-side is not a reliable storage and not what a user expects from a system that claims to manage content (e.g., switching between clients, browsers, power outages for more than 6 hours, whatever). Which in turn brings me to:

1) I think I've sufficiently clarified that unpublished draft revisions will not magically work. In fact, I'm relatively sure that entity-integrating modules are full of bugs right now (e.g., Menu module creating a publicly visible menu link for a unpublished node/entity). However, what I'm saying is this: We need to cater for that anyway, because draft revisions are absolutely going to happen. So why do we introduce yet another possible entity state + storage, next to draft revisions, which adds a considerable amount of complexity across the entire system?

In the end, the experience that users expect from a system that claims to manage content is exactly this:

Just save what can be saved, and manage the content for me. Don't make me think of "save." And, screw you if you ever fail to save my previous work.

That's the today's experience of Google Docs/Drive, and alas, also WordPress (apparently, since years already, since they're using the TinyMCE Autosave plugin, which essentially triggers a form submit every 30 seconds or so). What both have in common is that they truly manage the content for you and make you forget about "saving".

Additionally, both are automatically aggregating, pruning, and cleaning up needless intermediate automatically saved revisions in between (hence, "autosave"). AFAIK, WordPress achieves that by "tagging" auto-saved revisions, in order to identify them later (but since it's just WordPress, they're [ab]using the revision log message field to contain an auto-generated log message, and simply querying on that unique string identifier in the revision clean-up operation).

In the end, I'd like us to move on (and actually forward) on these pathways:

1) Become an actual Content Management System. It's surprising that we're even listed as one currently. Drupal is a pure Input/Output layer, there's factually no user content that is actually "managed". Entity revisions are the key facility towards that.

2) Leverage the architectural functionality that we already have, instead of inventing a new, custom "temporary storage" for user content.

3) Push the entity revision efforts forward.

4) Pave the way to CRAP (Create, Read, Archive, Prune) instead of CRUD (..., Update, Delete).

5) Pave the way to previews of complete site sections, instead of just a single entity.

In essence, I'm not saying that the tempstore approach cannot work out. But what I'm saying is that it's step in a very wrong direction. Technically, architecturally, and also from a product perspective. The web and end-user-facing technologies and applications are on their way to completely abandon and leaving behind the stone-age approach of a form that needs to be submitted or saved. Instead, users expect to interact with something, and as soon as they do, the app takes care of making sure that the user's interactions are persistently recorded somewhere. That is what enables "Undo" and "Redo". That's what enables you to forget about "Save". That is what gets you draft revisions that you can compare and revert, *before* making your post final.

In turn, a "Preview" button that saves into a tempstore, which will inherently forget about my entire precious work in X amount of minutes is the exact opposite of that. All we really gain from that is a fake-preview of content, which is inherently limited to exactly the entity itself, by design. It won't be able to support anything beyond that in the future. And it's totally not what end-users expect from a CMS in 2013.

It looks like you should be able to simply leverage the Form API cache by doing this:

Looked some more into this today and this is hard. If rebuild is not set, then the form is not cached, so no entity, so I can't leverage the form api cache at all and if set to true - even only in the preview method - the form won't redirect, unless I force it. Oh, the fun ..
Even #512026: Move $form_state storage from cache to new state/key-value system won't help me any further on that.

#1913742: Allow custom form state to be passed to entity_get_form() got in, so I'll give it one last try to see if I can some up with something so we don't loose data - it won't be perfect, but at least something ..

I concur with #145. We terribly broke preview in D7, and it needs to be fixed.

Let's not fool ourselves here. We are *not* going to tackle preview in a live environment in Drupal 8. I'm moving this as a feature request to Drupal 9.

The main problem of this kind of feature, other then the reservations that @sun raised, is that a piece of content never, ever, works in isolation. Pieces of content are displayed in Views, put in Menus, pulled by Panels, displayed in Blocks, sliced in meta-tags, indexed in Solr, etc.

This patch hardcodes the preview as the full page view, this is not going to be useful to anything except trivial sites.

The problem space described here is the one of the "Future preview", which is one very complex beast to get right. We have built full-site previews before, but that was by content staging / synchronization between Drupal instances. Building a real preview mode inside a single instance of Drupal requires all the components to be aware of the preview (including Solr indexes, database denormalization etc.), so it needs to be deeply rooted at the core of the system.

Adding some half-backed preview right at the last minute will not do Drupal 8 any good.

Sorry, but no. Even if this only worked for full node view (which it doesn't... see screenshot in #88), it's still a huge improvement on the current preview functionality, which doesn't actually let you preview anything. It shows you your node content, twice, stacked on top of each other, in the admin theme + overlay, with a yellow background. Nothing about that is even remotely what your content will actually look like when posted to your actual site.

An imperfect preview is what we already have in core today. This issue's just about fixing it so it actually works, within its current limited scope. I agree that a full-fledged time-based preview ala http://drupal.org/project/sps is definitely a D9 thing, but that's not what this issue is about (or at least, was about for the first 130 comments).

Given that we have the option to edit in place now, which is the best preview you can build in a non-hardcoded way, why not simply removing the preview option from the node module?

Because the full node form still exists, and you need a way to preview changes from there as well. Also, you don't "add in place," only "edit in place." Adding is still done through the primary node form. And finally, "edit in place" only works for visual properties, but there may be other things you need to preview (e.g. switching the display: hidden taxonomy term causes a theme change to kick in).

I'm joining this issue late, but I quickly skimmed through the recent comments. I think I agree with sun that ultimately, we'll want preview functionality to be based on unpublished forward revisions, but until the work in #1776796: Provide a better UX for creating, editing & managing draft revisions. is settled, I don't see a problem in proceeding with the tempstore approach. Tempstore is now as much a part of core architecture as $form_state is, and better suited to preview functionality, because $form_state is limited to when you stay in form context. In fact, maybe when we want to refactor previews to pull from an unpublished revision, we'll actually want to make that swappable/configurable, so that whether it's tempstore or a revision can vary.

I also agree that this issue is a bug fix more so than a new feature. If that means there's more interest in going straight to a revision-based approach (either after #1776796: Provide a better UX for creating, editing & managing draft revisions. or in parallel with it), rather than tempstore first, I think that would be ok too.

I strongly agree this is a bug. There is a reason why we disable the preview button by default on every site we make (even if our clients are asking for it)

This remains one of my top 2 favourite Drupal 8 issues.

Here's an attempted re-roll from #122, back when this was passing testbot, and before things went completely off the rails.

See what testbot has to say about that.

The title doesn't seem to be coming through.

The title was definitely not Error. That would be messed.

Check watchdog, when the site title is Error there is something useful in there.

This doesn't look like a useful message to me:

Recoverable fatal error: Argument 1 passed to node_access_preview() must be an instance of Node, instance of Drupal\Core\Entity\EntityBCDecorator given in node_access_preview() (line 2719 of /DRUPAL8/core/modules/node/node.module).

There were 6 similar php errors.

That is the most useful error it could have been. It explains exactly what is wrong, on which line, and roughly how to fix it.

EDIT: This sort of EntityInterface nonsense will go away once #1391694: Use type-hinting for entity-parameters is in.

Patch only applied with offsets. Here is a directly applying reroll. Also the fail seemed to be unrelated, due to the test system, so intrigued if this would still fail.

Note at some point there was lovely blue styling on that back link that seems to have gotten lost. Probably my fault.

Restoring the blue styling. @webchick did not include the node.preview.css file. Did not fix the fail yet around the log going missing when going into preview. Current state:

IMO, the dropdown with the view modes looks terrible. I would prefer a list of buttons or links, there will rarely be more than a handful of view modes. If we have to do a dropdown, lets use one of our dropbutton thingies.

@Bojhan/@moshe: not sure of best next step there, the design in the issue summary was pre-seven-style-guide and it does not seem to look anywhere close to how Seven is evolving with http://groups.drupal.org/node/283223

Right, so the point of reviewers above was that the functionality is already greatly improved vs. the current "preview", which is a joke :) We have a passing test that works. What else do we need then? :)

OK, we can defer style issues.

#159 suggests that we can cleanup once a dependant issue lands. That issue has landed so Needs Work? That's my reading of the situation. If I'm wrong, I see no other blockers.

+++ b/core/modules/menu/menu.moduleundefined
@@ -544,6 +544,13 @@ function menu_form_node_form_alter(&$form, $form_state) {
+  // In case we're in preview and there is no mlid yet but a title
+  // has been supplied, trick the menu form to store its information.
+  if (!empty($node->in_preview) && empty($link['mlid']) && !empty($link['link_title'])) {
+    $link['mlid'] = TRUE;
+    // @todo move all other properties to attributes as well ?

This is a hack I added at some point, however, this won't fly at all as we'd have to 'fix' this for every form that alters the node form, e.g. the alias, comment settings ... Going back to the node edit form will loose data.

I've tried getting around that in #132, but got completely stuck on Form API/tempstore.

Patch did not apply anymore. First a straight reroll.

@Moshe: re #170, to get rid of EntityInterface as a generic and use NodeInterface instead, the patch does at two new functions where EntityInterface is used, however there is already 50+ uses of EntityInterface in node.module (where these 2 functions are added), and absolutely no use whatsoever of NodeInterface. So I think for consistency, it makes most sense to use EntityInterface in this patch and do a generic EntityInterface => NodeInterface conversion at a different issue.

@swentel: re #171, what do you think should be the way forward here? Rip that code out and accept the form data loss? Sounds bad. :/ What did you get tripped up with above? It is not clear from the comments, so I could use a short summary. @sun's idea in #135 to use the form cache is also an interesting one, might actually be the easiest approach?

#172: node-1510544-172.patch queued for re-testing.

Straight reroll since one of the node.module hunks did not apply anymore.

@swentel, feedback is still welcome!

+++ b/core/modules/path/path.moduleundefined
@@ -99,7 +99,10 @@ function path_menu() {
+  if (!empty($node->in_preview) && !empty($node->path['alias'])) {
+    $path = $node->path;
+  }
+  elseif (!empty($node->nid)) {
     $conditions = array('source' => 'node/' . $node->nid);

This is problematic, already mentioned by sun, see his review in #131. It means that we should do this in every module that form alters on the node form, but the DX is just, well, crap :)

My follow up patch in #132 (see interdiff) uses a different trick where it manipulates the form state in the form method itself. However, ajax was broken with that. Trying to leverage the form cache alone is not easy either, because on a submit, that cache is cleared, so we loose it.

#1913742: Allow custom form state to be passed to entity_get_form() is now in, so maybe we can leverage that. We'll need to store the form state ourselves and add it again in the node add/edit page callback, but I haven't tested that.

I've been thinking about and been experimenting with alternate solutions. One idea I had is to keep the preview on the same path and just do alternate renderings of the form. When in preview, render the frontend theme and only affordances to go back to the form and when in editing, keep the admin look if configured. Now overlay breaks this whole thing, because it cannot redirect a form POST operation, it will do a GET request on the same path when we switch from the same path being admin/frontend, so all your ongoing changes are lost. Damn overlay! It looked too simple otherwise (even if a bit hackish). Attached the start of the experiment which in itself proved a fail :/ (it works nicely if you turn off the overlay altogether).

So looks like our best bet here is to capture the form cache and reinstate it when coming back to the form. Meh.

All right, once again a straight reroll because this did not apply anymore. Status merely for the testbot.

Dropped all path.module, taxonomy.module and menu.module special casing code that should not be present as discussed above.

All right based on that not working really for fields, here is another concept idea based on @swentel's suggestion above.

1. In the node form preview(), we store the current form_state in tempstore and just modify it a tiny bit to flag it as already coming from tempstore.
2. We redirect to the preview page with the tempstore ID passed on
3. The preview page takes the tempstore ID, loads form state from the tempstore and reignites the form.
4. Based on where we froze the form state, it will end up in preview() again but now we have our own flag on telling this is now an actual preview of the node, so it will do the render instead of the storage.
5. Most other things for rendering the actual preview page can stay, I just did not include them here to make the concept easier to understand...

The only little problem is I have been banging my head into the wall on some parts of this for a while, eg. how to get a node entity form for a yet non-existent but half-submitted node... Help on making this concept work would be welcome :)

Theoretically this uses the same tricks and rendering from the form state as before, it would just do it in the frontend theme, so the preview would appear in the actual design that it will end up in. Second experiment attached.

Bringing the experiment forward. This will render the node on a different path based on the frozen form state. It does not work with exposing the different view modes though (and for some reason it may not really preview the full view mode even though that is what we request). The preview is displayed as part of a re-rendering of the form. We should not show the actual form :) Then reinjecting the form state back into the form when going back is not working yet either. I looked but not sure where could that happen in the routing/controller setup that we have.

I didn't think it was possible, but we somehow made the preview in D8 even worse than in D7, because it now displays the node four times (two copies each of trimmed/full) instead of two, which was already freaking ridiculous. :) I still feel extremely strongly that we need to do this.

I can't quite parse out how to help with #183 and #184 though.

How can we help here? Not sure if I should reroll #182 or #184

Ugh.

@tim: essentially what we found with intense discussions with @swentel is that the current API only allows the temporary node to be built from a form state. You cannot just save the temporary node in tempstore because existing code only reacts to save and has special case code for preview. So unless we want to rewrite all the special case code for preview (and change the APIs for that), we need to pass around the form state and rebuild the node preview from that. Ideally we could keep posting the form from page to page, but overlay makes it totally impossible, since if you are editing the node in an overlay, the only way to break out of the overlay is with a GET request for the parent page. So we need a temporary form state storage on the side. I built a custom form state store in tempstore in the experiments above, but we may be able to retrieve the form state from the form cache if we pass around the form build ids. Then we can display the form again on a later page with the prior form state and/or use the form state to initiate a preview. The experiments only went so far to prove that storing the form state elsewhere can let us initiative a preview on a different path for the same form. It is far from complete. It would need to complete the rest of the pieces:

1. See if the form state can be directly retrieved from the form cache instead of custom tempstore.
2. Make it possible to restore the form based on the prior state on a later page.
3. Ensure this works with overlay :)

Or scrap this whole exploration and look into all the special casing code for previews and rewrite them for a different workflow of rendering. :)

Note that EntityViewController might help here, if we can set the entity from form state to the $defaults['_entity'] and also set $defaults['view_mode'] we can get the output from EntityViewController. Sounds like a job for a RouteEnhancer?

#182: node-1510544-182.patch queued for re-testing.

#2123085: Add preview options may be related to this issue, but I'm not sure (yet).

No, I don't think we can postpone this to D9, at least without fixing the critical regression we currently have where previewing your content shows it *four* times instead of only 2 in D7. Since this patch *also* happens to fix the critical problem we introduced in D7 (but didn't discover until after we shipped) of showing content intended for the front-end of your site in your *admin theme* (wtf?!?) I'd prefer to do this instead.

#2123085: Add preview options seems very cool, but ideally would hook into the preview bar added here.

It's tricky, because the preview bar "feature" certainly isn't critical by any definition of critical. Fixing the "4x" regression in the preview feature is, and wouldn't necessarily need this approach to solve it. But. It's also known that in UX testing Drupal vs. WordPress, "my front-end content shows up in my admin theme" consistently comes up as a critical UX issue for Drupal to solve, and this patch solves that very well. Additionally, fixing preview this way in core would open the doors to the reality of what's needed to preview in 2014+ (#2123085: Add preview options touches on some of that), that could then be extended by contrib, once the basis was in core.

So given all of that, I think I agree that this belongs as a critical issue. See #145 for the reasoning. Just because we shipped D7 with this in a completely broken state wrt this feature doesn't mean that the status quo shouldn't block release of D8.

The currently existing content previews are historically based on the idea that user input != output due to input filters. This kind of "text content preview" was the one and only purpose of our current previews.

Fast-forward to 2014, end-users reasonably assume that a text content preview can be easily delivered via Ajax, perhaps even in a "live" (real-time) updating fashion.

Our "Preview" buttons attempted to advance on that base concept over time, in order to preview additional data to the user. We added a "preview" of how e.g. the entered author name of your comment will be interpreted, as a [form] #item.

And over time, we further advanced on that concept to actually take your user input, process it into a "fake" entity (that doesn't exist) with plenty of fake property values, in order to be able to actually render your user input into a "real" entity in your preview. (A contrib developer's nightmare.)

Heck, for nodes, we're even generating two previews instead of one for you: A summary preview + A full preview.

But in a modular system (like Drupal) and a system in which final appearance vastly depends on context (like Drupal), these kind of previews are 99% inaccurate at max.

By turning the preview from an in-page preview to a full-page preview, you're improving the accuracy from 1% to 2% (at max). You will still not preview the associated menu link, the URL alias, nor anything else. Instead, you're showing me (1) an arbitrary page, (2) on which my entered content happens to be output, but (3) that is not an improvement over status quo.

I'd agree with the proposed solution, if it would not be a PITA for the application's architecture, storage, and DX at glance.

Given the architecture of D8, the only solution for this issue is #131 and #141. Revision that shit. Store forward-revisions. Only read the latest and published revisions/versions in production. Other applications did succeed using that approach already. There's no point in re-inventing the wheel.

Anything else will leave us in cluster of WTFs, both from a developer perspective and user perspective.

A concrete plan exists. You may dislike it, due to the massive changes being required. But that is not a reason to favor a "stop-gap" approach that is going to add a good amount of complexity to a system that is way too complex already, and which is not going to improve the situation in substantial ways.

FWIW, I disagree with bumping this to critical. Drupal's "previews" suck, I couldn't agree more. +1 on that.

However, this is a feature that requires serious architectural rethinking in order to work out. You had ~4 years (1,350+ days) to attack this problem space and, if necessary (certainly necessary), fully revamp how content is handled in Drupal. That did not happen.

By bumping this to a critical, release blocking "bug", the notion of "API/code freeze" has lost each and every meaning.

Regardless of how lackluster current previews are, this is a (major) feature request.

Hm. I don't recall saying that the current patch was necessarily a viable way to fix this issue, only that the issue itself is critical to resolve in D8, at least from a UX POV.

I don't think D7 shipped with this more broken than previous versions - just with the problem more visible (especially in user testing) since the admin theme was part of the standard install profile. Anyone who used an admin theme before that hit the problem before that.

@sun's ideas look pretty interesting to me. #140 raises some possible complications:

@Sun - I think saving temporary work revisions also has problems. Namely, hooks like to fire during Save operations and all those hooks would need to decide if they want to act on temporary saves. So we have traded off complexity by avoiding tempstore. I don't know which is the worse evil (tempstore or meaningless revisions).

But I would say that's arguably a good thing because we already have a need for this distinction even independent of previews. For example, if your module wants to fire off a notification e-mail every time someone saves a node, you already have the issue where if nodes are saved programmatically (like in an update function which runs in order to fix something about the node) you can wind up with a lot of e-mails you didn't want. Being able to distinguish "housekeeping" saves from "real" saves is important generally, and temporary revisions might just be a particular instance of a "housekeeping" save.

We've explored saving temporary revisions many times before. The main problem is only files have a garbage cleanup operation, where the files that were uploaded for a preview which was then not saved are later removed. Other items, like menu items or taxonomy terms, which would be created with this preview would need a garbage collection feature as well and be able to save as temporary as a consequence.

AFAIS keeping the in-memory entities that are being displayed is not *adding* complexity to the system, because that is already what we do. The only thing we don't do is to preview it in a frontend theme. That becomes much easier to do if overlay is not being used (eg. removed from core), because then we can post around data between different URLs easily, and some of them could display the in-memory entity with a frontend theme, while others could display the node form. I don't think that is any more of a hack than what we do now, we also do in-memory entity creation based on post data and then rendering that. We'd just post to a different URL and let the user come back to the editing URL.

My 2 cents.