Problem/Motivation

In AJAX views, Views used to set the current path with two leading slashes in some scenarios. This was fixed as a side effect of #2820347: Exposed filter reset redirects user to 404 page on AJAX view when placed as a block, but that fix didn’t include test coverage for this part of the problem.

Proposed resolution

Add test coverage.

Remaining tasks

Review and commit patch.

User interface/API/Data model changes

None.

Original bug report:

In the AJAX request, Views passes the current path (retrieved in views_views_pre_render()) in the page request as the view_path parameter – for example, /admin/content. As nearly everywhere in Drupal 8, a leading slash is included.

However, in \Drupal\views\Controller\ViewAjaxController::ajaxView() there is then the following code dealing with this parameter:

$path = $request->request->get('view_path');
// …
$this->currentPath->setPath('/' . $path, $request);

As you can see, a second slash is prepended to the method argument. When other code then tries to get, e.g., the route match for that request, the router will throw a ResourceNotFoundException with message No routes found for "//admin/content"..
It seems there is normally just no code that does this (or otherwise uses the current path without left-trimming it first) during Views AJAX requests, so this bug could go undetected so far. But it does cause errors in combination with some contrib modules, like Facets.

CommentFileSizeAuthor
#19 3000383-17.patch1.77 KBanushrikumari
#15 3000383-15.patch1.77 KBravi.shankar
#8 3000383-fix_views_ajax_request_current_path-8-4-interdiff.txt620 bytesmbovan
#8 3000383-fix_views_ajax_request_current_path-8.patch1.77 KBmbovan
#4 3000383-4.test-only.patch1.77 KBalexpott
#2 3000383-2--fix_views_ajax_request_current_path--simple.patch678 bytesdrunken monkey
#2 3000383-2--fix_views_ajax_request_current_path--bc_safe.patch696 bytesdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Active » Needs review
FileSize
3000383-2--fix_views_ajax_request_current_path--bc_safe.patch696 bytes
3000383-2--fix_views_ajax_request_current_path--simple.patch678 bytes

These would be patches to fix this – one simple one, just removing the code that prepends the slash, one that it safer regarding sites that already work around this problem by stripping the leading slash from the view_path parameter.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Writing a test for that would be really hard, I think. Personally I vote for the BC safe way, you never know.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Needs review
FileSize
3000383-4.test-only.patch1.77 KB

I think we can write a test because have \Drupal\Tests\views\Unit\Controller\ViewAjaxControllerTest which already does all the mocking necessary.

Also I think the test makes me wonder about the fix. Without the fix in place \Drupal\Tests\views\Unit\Controller\ViewAjaxControllerTest::testAjaxViewViewPathNoSlash() passes and the assertions are:

    $this->redirectDestination->expects($this->atLeastOnce())
      ->method('set')
      ->with('test-page?type=article');
    $this->currentPath->expects($this->once())
      ->method('setPath')
      ->with('/test-page');

So I'm not entirely sure whether we should be adding a slash - or whether a version without a slash would work as expected anyway.

Status: Needs review » Needs work

The last submitted patch, 4: 3000383-4.test-only.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pcambra’s picture

pcambra
he/him
Primary language Spanish
Location Asturies
CreditAttribution: pcambra at Cambrico commented

I've found this error recently while dealing with ajax in blocks embedded with twig, patch in #2 is really effective in combination with #2866386-4: Assert the view path is set correctly after second ajax request

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
3000383-fix_views_ajax_request_current_path-8.patch1.77 KB
3000383-fix_views_ajax_request_current_path-8-4-interdiff.txt620 bytes

Since #2 was already committed in #2820347: Exposed filter reset redirects user to 404 page on AJAX view when placed as a block, and #2866386: Assert the view path is set correctly after second ajax request is about tests now, I am uploading a patch that fixes code style issues in #4.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Title: Current path on Views AJAX requests is set with two leading slashes » Assert the current path on Views AJAX requests is not set with two leading slashes

Updated the issue title.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Just like in other related issues, more explicit test coverage doesn't hurt. I guess considering that the fix has been committed, the questions in #4 are also no longer relevant?

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

omarlopesino’s picture

omarlopesino
he/him
CreditAttribution: omarlopesino as a volunteer and at Metadrop commented
Related issues: +#2820347: Exposed filter reset redirects user to 404 page on AJAX view when placed as a block

The same solution of the safe path from #2 is already commited in core, by this issue: https://www.drupal.org/project/drupal/issues/2820347.

Then this task should be closed, isn't it?

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented

@mistermoper: #8 adds additional test coverage.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 3000383-fix_views_ajax_request_current_path-8.patch, failed testing. View results

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
3000383-15.patch1.77 KB

Added patch for Drupal 8.9.x

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

init90’s picture

init90
Location Ukraine
CreditAttribution: init90 at ZANZARRA Drupal Agency, Drupal Ukraine Community commented
Category: Bug report » Task
Issue tags: +Needs issue summary update
anushrikumari’s picture

anushrikumari CreditAttribution: anushrikumari at OpenSense Labs commented
FileSize
3000383-17.patch1.77 KB

patch #15 doesn't apply to 9.2.x, so I rerolled for the same

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Updated the issue summary.

Also reviewed the patch, which looks great, and verified that it indeed fails after reverting #2820347: Exposed filter reset redirects user to 404 page on AJAX view when placed as a block. Setting to RTBC accordingly.

  • catch committed 4634506 on 9.2.x 
    Issue #3000383 by drunken monkey, mbovan, anushrikumari, alexpott, ravi....

  • catch committed 8f1f2a4 on 9.1.x 
    Issue #3000383 by drunken monkey, mbovan, anushrikumari, alexpott, ravi....
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.