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

Created on 29 August 2015, almost 9 years ago
Updated 26 March 2024, 3 months ago

Problem/Motivation

Comment form needs to override destination when comment form submitted on comment field attached to block content entity
After submission visitor should stay on the same page

Steps to reproduce

If a comment-form is attached to a block, the redirect after submission is wrong - it points to the page "block/[block-id]/2#comment-x - instead of the current page.

Proposed resolution

add protected method to return commented entity (to which comment is attached to)

Remaining tasks

fix patch, , commit

User interface changes

submission a comment form attached to block content entities keeps you on the same page (current URL)

API changes

TBD

Data model changes

no

Release notes snippet

no

πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
CommentΒ  β†’

Last updated 2 days ago

Created by

πŸ‡©πŸ‡ͺGermany katzilla Berlin, Germany

Live updates comments and jobs are added and updated live.
  • Usability

    Makes Drupal easier to use. Preferred over UX, D7UX, etc.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡¨πŸ‡¦Canada sylus

    Just a re-roll against Drupal 10.2.x

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 5 months ago
    Patch Failed to Apply
  • last update 5 months ago
    Patch Failed to Apply
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    @sylus, curious to know, which patch did you reroll from? #87 ?

  • last update 5 months ago
    Build Successful
  • last update 5 months ago
    25,781 pass, 1,822 fail
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    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.

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    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

  • Status changed to Needs review 4 months ago
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

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

    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.

  • Pipeline finished with Failed
    4 months ago
    Total: 470s
    #96190
  • Status changed to Needs work 4 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Left some comments on the MR.

  • Pipeline finished with Canceled
    4 months ago
    #97677
  • Pipeline finished with Failed
    4 months ago
    Total: 558s
    #97678
  • Pipeline finished with Failed
    4 months ago
    Total: 620s
    #97686
  • Pipeline finished with Success
    4 months ago
    Total: 560s
    #97695
  • Status changed to Needs review 4 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    @smustgrave, I am unable to test MR 6622
    This merge request is way behind head of any current releases and has conflicts with

    • the HEAD of 10.1.x
    • the HEAD of 10.2.x
    • the HEAD of 10.3.x
    • the HEAD of 11.x
     ╭─◀▢ πŸ”€β–Ά
     ╰❯ $ patch -p1 --dry-run < 6622.patch
    checking file core/modules/comment/comment.services.yml
    checking file core/modules/comment/src/CommentForm.php
    checking file core/modules/comment/src/CommentLazyBuilders.php
    checking file core/modules/comment/tests/src/Functional/CommentBlockTest.php
    Hunk #1 succeeded at 4 (offset 2 lines).
    Hunk #2 succeeded at 24 (offset 2 lines).
    Hunk #3 succeeded at 102 (offset 2 lines).
    checking file core/modules/comment/tests/src/Functional/CommentTestBase.php
    Hunk #1 succeeded at 188 (offset 2 lines).
    checking file core/modules/comment/src/CommentForm.php
    Hunk #1 FAILED at 7.
    Hunk #2 FAILED at 49.
    Hunk #3 FAILED at 101.
    Hunk #4 FAILED at 113.
    Hunk #5 FAILED at 160.
    Hunk #6 FAILED at 458.
    6 out of 6 hunks FAILED
    checking file core/modules/comment/src/CommentLazyBuilders.php
    Reversed (or previously applied) patch detected!  Assume -R? [n]
    
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

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

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

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

  • Status changed to RTBC 3 months ago
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    Reviewed the code, it's functionally the same as patch 100 except rerolled.

    Test coverage is added and all tests are passing.

    Also, phpcs seems to be happy with this.

  • Status changed to Needs work 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    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.

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

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

  • Pipeline finished with Failed
    3 months ago
    Total: 687s
    #127812
  • Status changed to Needs review 3 months ago
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

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

  • Pipeline finished with Failed
    3 months ago
    Total: 684s
    #127842
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    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.

  • Pipeline finished with Canceled
    3 months ago
    Total: 126s
    #127902
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    May also need a rebase? With latest 11.x

  • Pipeline finished with Canceled
    3 months ago
    Total: 411s
    #127905
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    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.

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

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

  • Pipeline finished with Failed
    3 months ago
    Total: 178s
    #127907
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

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

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

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

    MR 7177 is based from 11.x

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

    Trying to fix this up with 7177.

  • Pipeline finished with Failed
    3 months ago
    Total: 597s
    #127936
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    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

  • Pipeline finished with Failed
    3 months ago
    Total: 538s
    #129667
  • Status changed to Needs work 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Tried rebasing but still getting multiple test failures :(

  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    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.
Production build 0.69.0 2024