Comment form should redirect to the current page when the form is in a block

Part of #1920044: Move comment field settings that relate to rendering to formatter options was to make the redirect path configurable.

I think this means we need it now, rather than later.

Block is not a content entity, so how form is attached?

I assume it is BlockContent

I think this is major

Simplest fix is to check access.

Nice to see more entities tested with comments!

Hm, looks #13 pointed the cause - it does not bound to block, we can expose the same state with any entity

For example create a view that displays nodes in full mode with comments, so when comment form submitted user redirected to view page, not commented node

+++ b/core/modules/comment/src/CommentForm.php
@@ -381,6 +382,8 @@ public function save(array $form, FormStateInterface $form_state) {
       // Redirect the user to the entity they are commenting on.
     }
-    $form_state->setRedirectUrl($uri);
+    if ($uri->access()) {
+      $form_state->setRedirectUrl($uri);

Looks that comment form should handle a case to pass "destination" when attached to entity that displayed not at its canonical route

Also if there's "destination" no need to calculate the page for new comment (tagged performance)

We could put some alter hooks in block content module to add a new #submit handler for any comment-form that had a comment-type associated with the block_content entity-type.
The #submit handler would redirect back to the previous page.

The issue is that the comment form has an explicit #action.

I don't think comment-form should be aware of nuances of particular entity-types, so it makes sense to be in block_content.module - it can be aware of itself obviously.

I still sure better to check that current route is canonical then calculate comment page
if not provide a redirect to current page

Oh, now I get it - good call - will work on this tomorrow

prodding

Ran out of time, progress patch

The core committers and entity system maintainers discussed this issue and agreed it can probably be handled as a normal bug per the criteria for major bugs: https://www.drupal.org/core/issue-priority#major-bug

Thanks!

Just attaching a patch against latest 8.x-5.x-dev based on #27 above.

The test should be moved to tests/src , the rest looks good

here is an update patch from #35 following suggestion from #36 ,also fixed some coding standards.
Please review.

Thanks

1. +++ b/core/modules/comment/src/CommentForm.php
   @@ -105,7 +112,7 @@ public function form(array $form, FormStateInterface $form_state) {
   +    if ($entity->url() == $this->currentPath->getPath() && !$comment->id() && !$comment->hasParentComment()) {
   
   @@ -397,7 +404,9 @@ public function save(array $form, FormStateInterface $form_state) {
   +    if ($entity->url() == $this->currentPath->getPath() && $uri->access()) {
   

   Do not use deprecated url() method

2. +++ b/core/modules/comment/tests/src/CommentBlockContentTest.php
   @@ -0,0 +1,104 @@
   +namespace Drupal\comment\Tests;
   ...
   +use Drupal\simpletest\WebTestBase;
   ...
   +class CommentBlockContentTest extends WebTestBase {
   

   Tests should be converted according to #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

here is an updated patch according to #39.

Please review.

Thanks

here is an updated patch deleting the old one

Please review.

@guptahemant please provide interdiff of changes - it makes review much easy

here is an interdiff between patches in #41 and #38.
Please review.

Let's see if test runner is happy. :-)

One more try to make test runner happy =)

We should still redirect after submission in paths like /comment/reply/[entity_type]/[entity_id]/comment so I made some changes to fix this.

Setting CommentBlockContentTest to extend from BrowserTestBase instead of CommentTestBase which was causing test to fail.
Also, fixed test fails for comment edition cases.

Updated the patch to work with latest dev. Also noted that on line 298 of core/modules/comment/src/CommentForm.php, its still using the old entityManager reference. Not sure if that needs to be changed.

Updated previous patch that was redeclaring the commentedEntity by mistake.

Rerolled the above patch for 8.8.x

Is it possible use a destination parameter in the path when commenting on an entity (not replying to another comment)?

Currently if you invoke the new comment form with /comment/reply/{entity_type}/{entity}/{field_name}?destination=someinternalpath, after saving the new comment you get redirected to to the entity commented and destination is not respected.

patch still applies to 9.1.x
patch still applies to 9.0.x
patch still applies to 8.9.x

+++ b/core/modules/comment/src/CommentForm.php
@@ -74,12 +92,32 @@ public static function create(ContainerInterface $container) {
+    $this->entityTypeManager = $entity_type_manager ?: \Drupal::service('entity_type.manager');
...
+    $parameters = $route_match->getParameters();
+    if ($parameters->has('node')) {
+      $this->commentedEntity = $parameters->get('node');
+    }
+    elseif ($parameters->has('entity')) {
+      $this->commentedEntity = $parameters->get('entity');
+    }
+    elseif ($parameters->has('comment')) {
+      $comment = $parameters->get('comment');
+      $this->commentedEntity = $this->entityTypeManager
+        ->getStorage($comment->getCommentedEntityTypeId())
+        ->load($comment->getCommentedEntityId());

@@ -117,8 +155,15 @@ public function form(array $form, FormStateInterface $form_state) {
+    if (isset($this->commentedEntity) &&
+      $this->commentedEntity->getEntityTypeId() == $entity->getEntityTypeId() &&
+      $this->commentedEntity->id() == $entity->id() && !$comment->id() &&
+      !$comment->hasParentComment()) {

@@ -409,7 +454,11 @@ public function save(array $form, FormStateInterface $form_state) {
+    if (isset($this->commentedEntity) &&
+      $this->commentedEntity->getEntityTypeId() == $entity->getEntityTypeId() &&

Instead of that it could introduce a getCommentedEntity() into separate method but not sure about mentioning "node" in constructor

Fixed IS

Resolving failed test cases

#68 still needs to be addressed

In our case, we have the whole content canonical page on a separate URL, and this fixes that case also.

Re-roled patch #72

Actually with the reply button the same problem occurs, this patch doesn't fix that case.
And some with edit, delete and any other operation.

@golddragon007, Added a fix for redirection for rest of the available links on comment form like: delete, reply edit.

Please review.

Reuploading the updated patch for the failed test in #78, Tests has a deprecated code use case, I have updated that:-
Updated

a/core/modules/comment/tests/src/Functional/CommentTestBase.php

with ->accessCheck(FALSE).

 else {
+      $cids = \Drupal::entityQuery('comment')
+        ->accessCheck(FALSE)
+        ->condition('entity_id', $entity->id())
+        ->condition('field_name', $field_name)
+        ->sort('cid', 'DESC')
+        ->range(0, 1)
+        ->execute();
+      if (!empty($cids)) {
+        $cid = reset($cids);
+        \Drupal::entityTypeManager()->getStorage('comment')->resetCache([$cid]);
+        return Comment::load($cid);
+      }
+    }

Hi @vakulrai,
Thanks fir the updated patch.
Patch applies cleanly.
RTBC+1

Verified and tested patch #80.
Patch applied successfully and looks good to me.

Testing Steps:
# Go to Structure -> Comment types -> Add comment type - Save it
# Go to Structure -> Add Custom Block -> Save it
# Go to Structure -> Block layout -> Place the created block into any region -> Save it.
# Go to respective page and user should see the comment form.

Expected Results:
# User should be redirected to the current page once adding the new comment.
# User should see Delete, Edit and Reply link under the added comments and that links should be working as expected.

Actual Results:
# Currently User is able to redirect on custom block page which is wrong behaviour.

Can be a move to RTBC.

Thanks for picking this up again folks.

We need a test here that asserts the new behaviour.

There's quite a lot of work being done here to load the commented entity, when we already have that available as $entity. Here's some suggestions

1. +++ b/core/modules/comment/src/CommentForm.php
   @@ -76,12 +94,32 @@ public static function create(ContainerInterface $container) {
   +    $parameters = $route_match->getParameters();
   +    if ($parameters->has('node')) {
   +      $this->commentedEntity = $parameters->get('node');
   +    }
   +    elseif ($parameters->has('entity')) {
   +      $this->commentedEntity = $parameters->get('entity');
   +    }
   +    elseif ($parameters->has('comment')) {
   +      $comment = $parameters->get('comment');
   +      $this->commentedEntity = $this->entityTypeManager
   +        ->getStorage($comment->getCommentedEntityTypeId())
   +        ->load($comment->getCommentedEntityId());
   

   this is a comment form.

   it has the scope of the entity (the comment) available as $this->entity

   from there we can determine the commented entity type using $this->entity->entity_type

   We need not hard-code this to be about the 'node' parameter or the 'entity' parameter - and we don't need to use the route match to load the comment, its there as $this->entity

2. +++ b/core/modules/comment/src/CommentForm.php
   @@ -119,8 +157,15 @@ public function form(array $form, FormStateInterface $form_state) {
   +    if (isset($this->commentedEntity) &&
   +             $this->commentedEntity->getEntityTypeId() == $entity->getEntityTypeId() &&
   +             $this->commentedEntity->id() == $entity->id() && !$comment->id() &&
   +             !$comment->hasParentComment()) {
   

   I think we could simplify this to something like so (pseudo code)

   <?php

   $entity_url = $entity->toUrl();
   if ($entity_url->getRouteName() === $this->routeMatch->getRouteName() && $entity_url->getParameters() === $this->routeMatch->getRawParameters()) {
   // ...

3. +++ b/core/modules/comment/src/CommentForm.php
   @@ -411,7 +456,11 @@ public function save(array $form, FormStateInterface $form_state) {
   +    if (isset($this->commentedEntity) &&
   +      $this->commentedEntity->getEntityTypeId() == $entity->getEntityTypeId() &&
   +      $this->commentedEntity->id() == $entity->id() && $uri->access()) {
   +      $form_state->setRedirectUrl($uri);
   

   similarly here

4. +++ b/core/modules/comment/src/CommentLazyBuilders.php
   @@ -165,14 +176,22 @@ public function renderLinks($comment_entity_id, $view_mode, $langcode, $is_in_pr
   +    $current_route = $this->currentRouteMatch->getRouteName();
   +    $route_options = $this->currentRouteMatch->getRawParameters()->all();
   +    if ($current_route) {
   +      $current_route = Url::fromRoute($current_route, !empty($route_options) ? $route_options : [], ['absolute' => FALSE])->toString();
   +    }
   

   we can use Url::fromRoute('<current>')->toString() here and avoid the change to inject the route match

5. +++ b/core/modules/comment/tests/src/Functional/CommentTestBase.php
   @@ -183,6 +183,20 @@ public function postComment($entity, $comment, $subject = '', $contact = NULL, $
   +    else {
   +      $cids = \Drupal::entityQuery('comment')
   +        ->accessCheck(FALSE)
   +        ->condition('entity_id', $entity->id())
   +        ->condition('field_name', $field_name)
   +        ->sort('cid', 'DESC')
   +        ->range(0, 1)
   +        ->execute();
   +      if (!empty($cids)) {
   +        $cid = reset($cids);
   +        \Drupal::entityTypeManager()->getStorage('comment')->resetCache([$cid]);
   +        return Comment::load($cid);
   +      }
   

   why is this change needed?

I've tried to address the issues mentioned in points 1 to 4.
For now, I haven't removed the 5th test case to check that refactoring passes the other test cases.
Refactoring related changes seems to be working fine on local.
Added test only patch as well.

I thinking of point 4 as that doesn't handle the query parameters (after the ?) and the anchors (#). The first one can be more interesting as a block can be placed on a page with some filters (that usually using query parameters), and then we may need to take care of that. But the anchors, those may not as I think drupal add the new comment's anchor automatically...

A rollback to 8.9.x of #87 as the new patch doesn't apply to that cleanly. Actually, this patch fixes an issue that we noticed in #76. (On the revision page the site crashes.)

I confirm that patch #87 is working. Using D9.2.6

#87 still applies (with fuzz) to HEAD of 9.3.x

Thanks for the work on the patches!

I'm on Drupal 9.3.6.

Patch #87 applies and works (with fuzz).

Patch #91 does make some changes, but I get the following message in my terminal after applying the patch:

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2021-06-07/drupal-comment_form_redirect-2559833-91-8.9.x.patch

And on my web pages with a comment block I get a white screen with:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getRouteName() on null in Drupal\comment\CommentForm->form() (line 148 of core/modules/comment/src/CommentForm.php).
Drupal\comment\CommentForm->form(Array, Object) (Line: 106)
Drupal\Core\Entity\EntityForm->buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 531)
Drupal\Core\Form\FormBuilder->retrieveForm('comment_comments_block_form', Object) (Line: 278)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 48)
Drupal\Core\Entity\EntityFormBuilder->getForm(Object) (Line: 123)
Drupal\comment\CommentLazyBuilders->renderForm('block_content', '4', 'field_comments', 'comments_block')
call_user_func_array(Array, Array) (Line: 101)
Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #lazy_builder callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725', 'exception', 'Drupal\Core\Render\Element\RenderCallbackInterface') (Line: 772)
Drupal\Core\Render\Renderer->doCallback('#lazy_builder', Array, Array) (Line: 342)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 201)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 157)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 158)
Drupal\Core\Render\Renderer->renderPlain(Array) (Line: 172)
Drupal\Core\Render\Renderer->renderPlaceholder('', Array) (Line: 649)
Drupal\Core\Render\Renderer->replacePlaceholders(Array) (Line: 534)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 201)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 145)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 146)
Drupal\Core\Render\Renderer->renderRoot(Array) (Line: 279)
Drupal\Core\Render\HtmlResponseAttachmentsProcessor->renderPlaceholders(Object) (Line: 71)
Drupal\big_pipe\Render\BigPipeResponseAttachmentsProcessor->processAttachments(Object) (Line: 45)
Drupal\Core\EventSubscriber\HtmlResponseSubscriber->onRespond(Object, 'kernel.response', Object)
call_user_func(Array, Object, 'kernel.response', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.response') (Line: 191)
Symfony\Component\HttpKernel\HttpKernel->filterResponse(Object, Object, 1) (Line: 179)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 191)
Drupal\page_cache\StackMiddleware\PageCache->fetch(Object, 1, 1) (Line: 128)
Drupal\page_cache\StackMiddleware\PageCache->lookup(Object, 1, 1) (Line: 82)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Re-rolling for 9.4.x
Patch was being applied, however it was being applied with fuzz.

I think we might need to review our solution approach.
Here is the scenario that I spotted on local and it seems breaking things.

- Add a few comments on node.
- Click on one of the comment's permalinks. It will change URL like https://drupal8.ddev.site/comment/16#comment-16
- Now when we submit the comment form, page is not loading with new comment. Comment form is also hidden.
- Reason is that, when URL is like comment/{comment_id} neither #actions or form_state->redirect() are being called.

Digging into this further.

Just a re-roll against Drupal 10.2.x

@sylus, curious to know, which patch did you reroll from? #87 ?
I just triggered the tests for the reroll.
Thanks

Re-queuing tests for 10.2.x , I did a dry run myself, should be no issue applying. We'll see what the results give.

The test failures appear to be a result of some issue with the ci test runner.

Error found: No space left on device

Perhaps turn this into a merge request and use the gitlab pipeline instead

Unless this is a major regression in the patch, possibly not.

9 years, we've been using this for likely at least 6 years.

Pleaser review merge request 6622

The WxT distribution is still using this fix, between 200 and 300 installs (likely many more installs that have uninstalled the update module and aren't reporting), multiple public sector organizations, multiple installs in each organization.

Left some comments on the MR.

Removed some unused services that were being added. Tests are green thoughts on the change?

@joseph.olstad mentioned you've been using this patch for a while. Did the changes in the MR continue to work for you?

**EDIT**
ignore this comment
**END EDIT**

ah, nevermind, I tried git apply and it went in clean.

The merge request applies cleanly against 11.x and 10.3.x but not 10.2.x.

Thanks @smustgrave,
Reviewed the code quickly, it's functionally the same as (or very close to) patch 100 except rerolled.

Test coverage is fixed now and all tests are passing.

Also, phpcs seems to be happy with this.

If I revert all the changes to core/modules/comment/src/CommentForm.php the tests still pass and reading the code I'm not sure why we're making those changes.

@alexpott, I responded to your concern and pushed up a new commit. Pipeline is running.

LOL, reverting my last commit, that code in question actually does do something.

Hmm, so on "17 February 2024" , the merge request was passing.

I made which seems like a reasonable change to make, the tests fail the failure actually might look like a test bot or ci issue not the actual MR.

Then I reverted the "reasonable" change, same result.

So, it looks like something outside of this merge request changed between February 17th 2024 and today that is affecting the unit tests.

May also need a rebase? With latest 11.x

Rebased with the latest 10.3.x

I had a look at patch 100, this is the patch that we've been using for several years.

The code in patch 100 is as follows:

@@ -411,7 +448,11 @@ public function save(array $form, FormStateInterface $form_state) {
       $this->messenger()->addError($this->t('Comment: unauthorized comment submitted or comment submitted to a closed post %subject.', ['%subject' => $comment->getSubject()]));
       // Redirect the user to the entity they are commenting on.
     }
-    $form_state->setRedirectUrl($uri);
+    $entity_url = $entity->toUrl();
+    if ($entity_url->getRouteName() === $this->routeMatch->getRouteName() &&
+      $entity_url->getRouteParameters() === $this->routeMatch->getRawParameters()) {
+      $form_state->setRedirectUrl($uri);
+    }
   }
 
 }

but the code in the MR is like this:

-    $form_state->setRedirectUrl($uri);
+    $entity_url = $entity->toUrl();
+    if ($entity_url->getRouteName() === $this->routeMatch->getRouteName() &&
+      $entity_url->getRouteParameters() === $this->routeMatch->getRawParameters()) {
+      $form_state->setRedirectUrl($uri);
+    }
+    $form_state->setRedirectUrl($uri);

Somewhere between patch 100 and the current MR, things went weird.

Specifically according to git blame it is the feb 17th commit by smustgrave f522c159ac3a that brought in this change.

So I pushed up dropping the last setRedirectUrl so that the preceding condition becomes once again meaningful.

ah crap, I made a mistake using 10.3.x , sorry folks

I should have checked that the MR was against 11.x not 10.3.x
Sorry about this. what a mess I just made!

ok @smustgrave, I did my best to recuperate your work into the MR 7171 , cherry-picking commits into it.

MR 7171 is based from 11.x

10.3.x was accidentally merged into 6922 by me, sorry.

Should be fixed up with 7171. I believe all of your commits were cherry-picked in.

The current failure is completely unrelated to what we're doing here.

Suspect something is going on with the ci system or maybe a patch that was pushed into 11.x

core/tests/Drupal/Tests/Component/Scaffold/Functional/ManageGitIgnoreTest.php

Tried rebasing but still getting multiple test failures :(

Current test failures:

1. related to MR 7171

Exception Other      phpunit-162.xml      0 Drupal\Tests\comment\Functional\Vie
    PHPUnit Test failed to complete; Error: PHPUnit 9.6.15 by Sebastian
    Bergmann and contributors.
    
    Testing Drupal\Tests\comment\Functional\Views\CommentAdminTest
    E.                                                                  2 / 2
    (100%)
    
    Time: 00:24.360, Memory: 4.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\comment\Functional\Views\CommentAdminTest::testApprovalAdminInterface
    Behat\Mink\Exception\ResponseTextException: The text "zsjfmfvf" was not
    found anywhere in the text of the current page.

2. This one possibly unrelated(?)

---- Drupal\Tests\filter\Functional\FilterHtmlImageSecureTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-172.xml      0 Drupal\Tests\filter\Functional\Filt
    PHPUnit Test failed to complete; Error: PHPUnit 9.6.15 by Sebastian
    Bergmann and contributors.
    
    Testing Drupal\Tests\filter\Functional\FilterHtmlImageSecureTest
    F                                                                   1 / 1
    (100%)
    
    Time: 00:06.514, Memory: 4.00 MB
    
    There was 1 failure:
    
    1)
    Drupal\Tests\filter\Functional\FilterHtmlImageSecureTest::testImageSource
    http://localhost/subdirectory/core/misc/druplicon.png was found.
    Failed asserting that false is true.