Problem/Motivation

The code in PluginBase::viewsTokenReplace() replaces $twig_tokens[$top] on every loop, if multiple tokens have the same $top, only the last actually ends up in the array.

Proposed resolution

Merge the tokens together instead of overwriting.

Remaining tasks

User interface changes

API changes

Data model changes

Why this should be an RC target

From #5:

Trivial fix with test coverage that really shouldn't break anything, so maybe we can get it in before 8.0.0?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
764 bytes
Jaesin’s picture

Priority: Normal » Major

I just wrote basically the same patch:

<?php
        $twig_tokens[$top] = !isset($twig_tokens[$top]) ? $token_array : $twig_tokens[$top] + $token_array;
?>
Lendude’s picture

Status: Needs review » Needs work

Still needs tests, so back to needs work.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests +rc target triage
FileSize
1.03 KB
1.75 KB

Lets add them, then.

Trivial fix with test coverage that really shouldn't break anything, so maybe we can get it in before 8.0.0?

Berdir’s picture

FileSize
1.75 KB

Weird double post is weird

The last submitted patch, 5: views-token-2591125-5-tests-only.patch, failed testing.

Jaesin’s picture

Status: Needs review » Reviewed & tested by the community

The easiest to test this is to create a view with two "nid" contextual filters and a path like: "test-view/%/%". Maybe disable caching for the view for good measure.

Add a "No results behavior" of "Global: Text area" with the following content:

1st: {{ raw_arguments.nid }} <br />
2nd: {{ raw_arguments.nid_1 }}<br />
Final

You will notice that if you go to the url: "test-view/12345/54321" without the corresponding nodes existing, you will get an output like:

1st:
2nd: 54321
Final 

After applying the patch in #6 , you should see:

1st: 12345
2nd: 54321
Final

as expected.

xjm’s picture

Issue summary: View changes

Adding the RC target reasoning to the summary.

The last submitted patch, 5: views-token-2591125-5-tests-only.patch, failed testing.

tim.plunkett’s picture

Went to RTBC this, see I'm behind the times. +1 from me :)

effulgentsia’s picture

Issue tags: -rc target triage +rc target

I discussed this with @xjm, and we agreed that "tokens are broken in Views" qualifies as sufficiently high impact and that the patch qualifies as sufficiently low disruption, so it's good as an RC target per https://www.drupal.org/core/d8-allowed-changes#rc.

  • xjm committed aca6ac9 on 8.0.x
    Issue #2591125 by Berdir, Jaesin: Only the last views argument token...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks for the fix! Thanks also @Jaesin for the excellent testing steps.

Status: Fixed » Closed (fixed)

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