Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

Status: Active » Needs review
FileSize
739 bytes

Here is something to get started.

hussainweb’s picture

I suppose we should also check that restricted tokens are not shown on other trees. This patch should help.

Status: Needs review » Needs work

The last submitted patch, 3: add_tests_to_check_for-2646952-3.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
690 bytes
3.9 KB

I think this should fix it.

Status: Needs review » Needs work

The last submitted patch, 5: add_tests_to_check_for-2646952-5.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
4.48 KB

I have commented out the test that fails with a todo as the issue is something else entirely. I have not removed it because it could be quite useful once we fix the issue with caching. Here is the comment for quick reference:

    // @todo: This test is commented out for later addition. The equivalent
    // functionality is tested elsewhere anyway. This test causes failures now
    // due to incorrect handling of caching for tree pages.
Berdir’s picture

Status: Needs review » Needs work

Ah. That's not token caching, that's the dynamic page caching.

This should be easy enough to fix, we just need to instruct it that url arguments are relevant for the output of the page. See https://www.drupal.org/developing/api/8/cache/contexts.

Add '#cache' => ['contexts' => ['url.query_args']] to the render array returned by that controller and it should work.

The last submitted patch, 7: add_tests_to_check_for-2646952-7.patch, failed testing.

hussainweb’s picture

Yes, already done here: #2647012: Add cache context for the token/tree page. I thought of keeping it separate but if you want, it can be in one patch as well.

Berdir’s picture

Ah, cros post. Yes, lets just do that together in this issue.

The last submitted patch, 7: add_tests_to_check_for-2646952-7.patch, failed testing.

hussainweb’s picture

Title: Add tests to check for restricted tokens » Add tests to check for restricted tokens and fix caching on token/tree route
Issue tags: -Quickfix
FileSize
4.67 KB

Sure, here it is.

hussainweb’s picture

Status: Needs work » Needs review

  • Berdir committed e2dfba6 on 8.x-1.x authored by hussainweb
    Issue #2646952 by hussainweb: Add tests to check for restricted tokens...
Berdir’s picture

Status: Needs review » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

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