Destination URL breaks preview

#1510544: Allow to preview content in an actual live environment is in.

Just a quick drive by review.

1. +++ b/core/modules/node/src/NodeForm.php
   @@ -404,6 +402,11 @@ public function submit(array $form, FormStateInterface $form_state) {
   +    if ($destination = \Drupal::request()->query->get('destination')) {
   +      \Drupal::request()->query->remove('destination');
   +      $this->entity->preview_destination = $destination;
   +    }
   
   @@ -488,7 +491,10 @@ public function save(array $form, FormStateInterface $form_state) {
   +      if (!empty($this->entity->preview_destination)) {
   +        \Drupal::request()->query->set('destination', $this->entity->preview_destination);
   +      }
   

   You can use $this->request() instead. PS: It is odd to put arbitrary values onto the entity. Do you have an idea how to fix that on the longrun?

2. +++ b/core/modules/node/src/Tests/PagePreviewTest.php
   @@ -30,11 +30,16 @@ class PagePreviewTest extends NodeTestBase {
   +  protected $web_user;
   ...
   -    $web_user = $this->drupalCreateUser(array('edit own page content', 'create page content'));
   -    $this->drupalLogin($web_user);
   +    $this->web_user = $this->drupalCreateUser(array('edit own page content', 'create page content'));
   +    $this->drupalLogin($this->web_user);
   

   You can directly camelCase it, if possible.

Well, this works for sure in core, but how can we ensure horizontal extensibility?

+++ b/core/modules/node/src/Tests/PagePreviewTest.php
@@ -192,6 +197,15 @@ function testPagePreview() {
+    $this->drupalPostForm('node/' . $node->id() . '/edit', array(), t('Preview'), array('query' => array('destination' => 'user')));

Why uid is not included here in the destination?

@swentel thank you for the explanation but I was not asking about the uuid I was asking about uid ($this->web_user->id()

)
+++ b/core/modules/node/src/Tests/PagePreviewTest.php
@@ -192,6 +197,15 @@ function testPagePreview() {
+    $this->assertEqual($this->getUrl(), url('user/' . $this->web_user->id(), array('absolute' => TRUE)));

Here we are checking the user/uid url.

+++ b/core/modules/node/src/Tests/PagePreviewTest.php
@@ -192,6 +197,15 @@ function testPagePreview() {
+    $this->drupalPostForm('node/' . $node->id() . '/edit', array(), t('Preview'), array('query' => array('destination' => 'user')));

But here we are only adding 'user' to destination. I am just asking shouldn't it be 'user' . $this->web_user->id().
Sorry for the confusion.

I just accidentally realized this happens on contextual links as well (e.g. editing the node from the teaser view on the home page) which makes this at least major, since we're tossing out peolpes' edits.

Lol, wait. So you can engage contextual links on a preview?

Oh. That too. ;) But I was talking about being on the front page, hovering to get a contextual link, and choosing "Edit" from the list. Any edits you make from there before clicking "Preview" are lost, because you just get redirected back to the front page instead of the preview.

This still makes the bot happy.

I confirmed the bug still exists without the patch, but it works as needed with the patch.

Both preview link goes to the preview page rather than back to admin/content and the destination seems to be preserved.

Let's get this fixed.

I should note that the code looks fine to me and the tests are in place too.

+++ b/core/modules/node/src/NodeForm.php
@@ -424,7 +429,10 @@ public function save(array $form, FormStateInterface $form_state) {
+      if (!empty($this->entity->preview_destination)) {
+        \Drupal::request()->query->set('destination', $this->entity->preview_destination);
+      }

I think we should use $this->getRequest().

Ok.. I think this is the right format, so re-rolling:

+      if (!empty($this->entity->preview_destination)) {
+        $this->getRequest()->query->set('destination', $this->entity->preview_destination);
+      }

+++ b/core/modules/node/src/NodeForm.php
@@ -346,6 +346,11 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      $this->entity->preview_destination = $destination;

@@ -424,7 +429,10 @@ public function save(array $form, FormStateInterface $form_state) {
+        $this->getRequest()->query->set('destination', $this->entity->preview_destination);

Isn't storing arbitrary properties on entities a problem?
Wouldn't we then need to remove it later?

Why not just store it in the $form_state?

Just unassigning issues that haven't been developed for a bit in the D8 queue.

Adding the test from #26, even though I know it's going to fail, so that the test can be discussed.

+++ b/core/modules/node/src/Tests/PagePreviewTest.php
@@ -273,10 +273,12 @@ function testPagePreview() {
     $this->drupalPostForm('node/' . $node->id() . '/edit', array(), t('Preview'), array('query' => array('destination' => 'user')));

<?php
array('query' => array('destination' => 'user'))
?>

Should this be chopped off? Is it really possible to have a destination here?

OK, I think it should stay in PagePreviewTest though. The user doesn't always start out at admin/content; The user may have clicked a contextual link.

For moving the test into PagePreviewTest. By being in a separate test, a whole new Drupal gets installed within simpletest.

See: https://www.drupal.org/node/2640672

Adding an issue that might be related.

IMHO the patch looks good and works as expected. Perhaps we shouldnt introduce that new helper function when that will be fixed in the related issus

Just tried this on the 8.1.0 release and it worked a treat!

Tested it too, and confirming that it solved the issue. Here's a reroll to keep up with head.

+++ b/core/modules/node/src/Tests/PagePreviewTest.php
@@ -260,6 +260,31 @@ function testPagePreview() {
+    $web_user = $this->drupalCreateUser(array('access content overview', 'administer nodes', 'edit any page content', 'access user profiles'));

Shouldnt we use short array syntax here?

I have checked the functionality and it seems to work fine. Only the remark I have put above.

I fixed up the array shorthand, and tested the patch on 8.2.x in the UI. Works for me.

1. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -67,4 +67,25 @@ protected static function processStatesArray(array &$conditions, $search, $repla
   +  public static function redirectOptionsPassThroughDestination($options = []) {
   

   Is this the best place for this method, and does it need to be static? Also it's essentially directly manipulating a global, that seems very bad.

   Finally, should have an array typehint.

2. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -67,4 +67,25 @@ protected static function processStatesArray(array &$conditions, $search, $repla
   +    if ($destination = \Drupal::request()->query->get('destination')) {
   
   +++ b/core/modules/node/src/Form/NodePreviewForm.php
   @@ -72,7 +73,14 @@ public function getFormId() {
   +    if ($destination = $this->getRequest()->query->get('destination')) {
   

   I would think this would use the \Drupal\Core\Routing\RedirectDestination service.

I removed the static form helper and made the test simple again as in 26.
tim.plunkett's suggestion with RedirectDestination was not taken into consideration yet, though.

The test-only patch has both test variants and will fail for the same reason.
I took the less verbose one, but I guess it is a matter of taste.

1. +++ b/core/modules/node/src/Form/NodePreviewForm.php
   @@ -72,7 +72,14 @@ public function getFormId() {
   +    if ($destination = $this->getRequest()->query->get('destination')) {
   
   @@ -116,10 +123,16 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
   +    if ($destination = $this->getRequest()->query->get('destination')) {
   
   +++ b/core/modules/node/src/NodeForm.php
   @@ -341,10 +338,18 @@ public function preview(array $form, FormStateInterface $form_state) {
   +    if ($destination = $this->getRequest()->query->get('destination')) {
   

   #54.2 should be addressed.

2. +++ b/core/modules/node/src/Form/NodePreviewForm.php
   @@ -116,10 +123,16 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
   +    $options = $this->getRequest()->query->all();
   

   This is wrong. Query item should be under $options['query'], so probably:

   $options = [ 'query' => $this->getRequest()->query->all()];
   

3. +++ b/core/modules/node/src/Tests/PagePreviewTest.php
   @@ -260,6 +260,16 @@ function testPagePreview() {
   +    $this->drupalPostForm('node/' . $node->id() . '/edit', array(), t('Preview'), array('query' => array('destination' => $destination)));
   +    $this->assertEqual($this->getUrl(), \Drupal::url('entity.node.preview', array('node_preview' => $node->uuid(), 'view_mode_id' => 'default'), array('absolute' => TRUE, 'query' => array('destination' => $destination))));
   ...
   +    $this->drupalPostForm(NULL, array(), t('Save'));
   

   Let's be consistent and use the modern array square brackets syntax.

4. +++ b/core/modules/node/src/Tests/PagePreviewTest.php
   @@ -260,6 +260,16 @@ function testPagePreview() {
   +    $this->assertEqual($this->getUrl(), \Drupal::url('entity.node.preview', array('node_preview' => $node->uuid(), 'view_mode_id' => 'default'), array('absolute' => TRUE, 'query' => array('destination' => $destination))));
   

   Use $this->assertUrl(); instead assertEqual().

And I think is still 8.1.x.

Still broken!

Useful info: Use edit from within contextual-links. You can preview from that user journey.

Oh, but what are contextual links? These can be many things. In this context they are your edit content button, (it looks like a pencil). Edit and preview from there. Note the correct destination parameter.

Actually. Above suggestion by me does not work. I have just tried on a development server. Can a core committer advise on resolution scope?

Fixing issues mentioned by @tim.plunkett and @claudiu.cristea

The bugfix includes internal API changes so for that reason is targeted against the development minor, which is currently 8.3.x. Since this is a major bugfix, we could consider it for RC target triage if it is ready within the release candidate phase of 8.2.x. Otherwise, though, it should be fixed in 8.3.x or whatever the development minor is when it is ready. Thanks!

+++ b/core/modules/node/src/Form/NodePreviewForm.php
@@ -129,7 +142,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    if ($destination = $this->redirectDestination->get()) {

It is not that simple.

The injected redirectDestination service always returns a destination.

So to demonstrate why the last patch failed:

// from patch #57
if ($destination = $this->getRequest()->query->get('destination')) {
...
}
// in patch #63
if ($destination = $this->redirectDestination->get()) {
...
}
// but what it really meant in patch #57
if ($this->getRequest()->query->has('destination')) {
$destination = $this->getRequest()->query->get('destination')
...
}

It worked because \Symfony\Component\HttpFoundation\ParameterBag::get() returns NULL when the key is not set and thus the if statement is not evaluated. (This is why patch #63 fails seemingly unrelated tests).

We also still use $this->getRequest()->query

+++ b/core/modules/node/src/Form/NodePreviewForm.php
@@ -117,10 +137,16 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
+    $options = $this->getRequest()->query->all();


+++ b/core/modules/node/src/Form/NodePreviewForm.php
@@ -117,10 +137,16 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
+      $this->getRequest()->query->remove('destination');

So I am not sure if we really gain anything.

Discussed with @xjm, @alexpott, @amateescu, @fago, @berdir and @plach. We
agreed that this is major because it causes user input to be lost.

Also the bug is currently worse than it sounds in the issue summary: If you have a destination URL and click preview, the preview does not work at all, and it might appear the node has been saved.

Is there a reason why the destination is always added to the operations for nodes?
The content view has a configuration option to have (or not have) the destination in the links, but it has no effect as the destination is already set in the query attribute of the operations.

I tried looking into the matter (to the NodeListBuilder's getDefaultOperations method) but could not understand the reason. I'm quite new to Drupal so it would be cool if you could explain it to me.

Here we go...

EDIT: interdiff is against #57

@GroovyCarrot, I think it's impossible to predict how other multistep forms will behave.

EDIT: Problem below seems to be related to Rename Admin Paths module, after disabling module, everything works as expected.
----
I have another problem which may be related to this issue and I think was introduced with 8.2.0, same thing is present with 8.2.1 When clicking on Edit button on admin/content page the following (example) link is generated:

http://www.site.com/node/999/edit?destination=/admin/content

However, the following error is generated when accessing above url:

The website encountered an unexpected error. Please try again later.

The following log entry is generated:

Location: http://www.site.com/node/999/edit?destination=%2Fadmin%2Fcontent
Referrer: http://www.site.com/admin/content
Message: UnexpectedValueException: Invalid Host "www.site.com?destination=" in Symfony\Component\HttpFoundation\Request->getHost() (line 1238 of C:\inetpub\wwwroot\sites\site.com\vendor\symfony\http-foundation\Request.php).

Site is running under Windows Server (IIS).

For some reason, host name is parsed as "www.site.com?destination=" and not "www.site.com" and this results in invalid host error.

Encoded url works. Above 'Location' url can be opened in browser without an error.

If %2f is replaced with '/' error is generated.

Error is generated in any case there is '/' after ? character, not only in case of 'destination' parameter.

Example:

http://www.site.com/node/999/edit?test=/admin/content

I can also confirm the problem with preview button which is present since the first release of Drupal 8.

The patch in #70 fixed the issue for me on previewing.

There is another issue with this destination as well in that it interferes with custom redirects added to the form. I don't know if fixing that should be included here or in a new issue.

1. +++ b/core/modules/node/src/NodeForm.php
   @@ -82,8 +84,8 @@ public function form(array $form, FormStateInterface $form_state) {
   -      // Rebuild the form.
   -      $form_state->setRebuild();
   +      // Cache the form state for submission request.
   +      $form_state->setRequestMethod('POST')->setCached();
          $this->entity = $preview->getFormObject()->getEntity();
   

   Looks like this overlaps a bit with #2548713: Only one additional new value saved unlimited field and no non-field values are restored after preview .

2. +++ b/core/modules/node/src/NodeForm.php
   @@ -341,10 +343,19 @@ public function preview(array $form, FormStateInterface $form_state) {
   +    if ($query->has('destination')) {
   +      $options['query'] = $this->redirectDestination->getAsArray();
   +      $query->remove('destination');
   +    }
   +    $form_state->setRedirect('entity.node.preview', $route_parameters, $options);
   

   is this the only reason we need the redirectDestination service here? Since we access query already directly and only do it if we have an actual destination, can't we just read it from $query? Then we don't have to inject another service that possibly breaks subclasses that people might have created.

   Especially since I think this would fail if no service is injected as we don't all back to \Drupal::service(). We have to add that fall back if we are going to use it unconditioally.

Needs work atleast for making sure the optional service/argument is really optional/has a default.

To answer some recent questions, overriding a a defined redirect is in purpose, by the time it happens, there is no such thing as a custom or a non-custom form state redirect, they both look exactly the same. If you want to override, you need to to this yourself too, as storing and handling the destination will usually vary.

Hi Team,

Please find the my patch to fix the preview issue.

There is a non-core change in the patch.

+++ b/modules/custom/domain_site_settings
@@ -1 +1 @@
-Subproject commit 9ad07fb244835b70485139cda05f1c91262cfc19
+Subproject commit 9ad07fb244835b70485139cda05f1c91262cfc19-dirty

It is not obvious to me why this patch is so much smaller than prior patches (it could be a more elegant solution). However, there is no reason to have removed the test code.

Assigning to fix #76.

@cilefen
sorry for my last patch.
Please find the updated patch to fix the issue.

@aloknarwaria: Thanks but here already is a working patch that has test coverage and properly keeps the destination, so it actually works again *after* preview.

Just use the patch in #70. Which probably now needs a reroll, as the referenced issue is in.

Thanks @Berdir, I have rerolled the patch #70 against 8.3.x branch.

Looks like this overlaps a bit with #2548713

@Berdir, after #2548713: Only one additional new value saved unlimited field and no non-field values are restored after preview, the patch doesn't work anymore and I cannot find a good reason.

There were some errors in the rebase, two that I could find. We don't have to change anything anymore in the node form preview logic, there was still one change that caused an immediate redirect back. The second was that the uuid now needs to be always sent back, even for existing nodes.

Now PagePreviewTest passes, lets see about the other fails.

Fixing also #76.2. This is ready for RTBC now.

+++ b/core/modules/node/src/Form/NodePreviewForm.php
@@ -116,10 +121,12 @@ public function buildForm(array $form, FormStateInterface $form_state, EntityInt
-    ));
+    ];
+    $options = ['query' => $this->getRedirectDestination()->getAsArray()];
+    $form_state->setRedirect('entity.node.preview', $route_parameters, $options);

this must be conditional as well. the redirect destination stuff falls back to the current url for the destination.

To reproduce:

1. Edit, without destination
2. Preview
3. Switch view mode
4. Now you get a destination to the current preview URL
5. Go back, save, => 404

Just forget about redirect destination stuff, simply use request query like in buildForm, with the basically the same logic.

We should also have a test for this.

Implemented #89.

A 8.2.x version of #90.

Ok, I think this looks great now.

I uploaded one patch here as well, but technically, I just removed some lines of code to make it pass. So I think I can still RTBC this, I reviewed this multiple times and tested it pretty extensively.

Btw, another preview bug that I'd love some help with: #2834316: Node preview shows and defaults to "Default" instead of "Full" view mode (will conflict with this)

Committed c7da19c and pushed to 8.3.x. Thanks!

I think that the current patch does not have the internal API changes that the old patch in #65 had so might be eligible for 8.2.x too.

1. +++ b/core/modules/node/src/NodeForm.php
   @@ -21,11 +21,6 @@ class NodeForm extends ContentEntityForm {
   -   * Whether this node has been previewed or not.
   -   */
   -  protected $hasBeenPreviewed = FALSE;
   -
   -  /**
   

   This property is protected so it makes sense to clean it up in 8.3.x as we have, but I think we could also backport the patch to 8.2.x if we provided a small BC layer. Instead of removing the property, mark it as deprecated for removal in 8.3.0 with intstructions to use the form state flag instead.

2. +++ b/core/modules/node/src/NodeForm.php
   @@ -89,7 +84,7 @@ public function form(array $form, FormStateInterface $form_state) {
   -      $this->hasBeenPreviewed = TRUE;
   +      $form_state->set('has_been_previewed', TRUE);
   

   Here, we would retain both these lines. We would also need to add a small check to the form constructor to set the property from $form_state.

NR to see if this suggestion makes sense. Thanks!

Re-Committed 830934d and pushed to 8.3.x to adjust the issue credits. Thanks!

@aloknarwaria, thanks for your interest in contributing to fix this bug! Next time, in order to get issue credit, please be sure to look at the existing patches and reviews on the issue, and collaborate with others on the solution

@xjm, yes, it makes sense.

but...

We would also need to add a small check to the form constructor to set the property from $form_state

I'm not sure I understood this part.

Sorry, in the form "builder" in D7 lingo (NodeForm::form()), not the class constructor. In order to ensure that the protected property matches the value in $form_state, we need to set it from $form_state, since that can persist across requests. No?

Ok.

@xjm, here's the BC.

+++ b/core/modules/node/src/NodeForm.php
@@ -22,6 +22,9 @@ class NodeForm extends ContentEntityForm {
    * Whether this node has been previewed or not.
+   *
+   * @deprecated Scheduled for removal in Drupal 9.0.x. Use the form state
+   *   property 'has_been_previewed' instead.
    */
   protected $hasBeenPreviewed = FALSE;
 

we already removed it in 8.3, so this is not correct.

Fixed the @deprecated message.

Ok, looks fine now I think.

Random test fails.. undefined index 'database', multiple times.

That looks fine for bc and the deprecation message is correct now. Committed/pushed to 8.2.x, thanks!