Links with query params with integer keys reset the keys [#3191456]

Problem/Motivation

Using the link field with a link like this: /my-page?myparam[2021]=2021 when printed looks like this: /my-page?myparam[0]=2021&myparam[1]=2021

This is a problem when you try to add links to filtered views because even though the filters get applied, the boxes don't get checked.

Steps to reproduce

- Create a link field
- Point to an internal view page with filters in the url like: /my-page?myparam[2021]=2021

Expected behavior

- Link will be printed as entered

Current behavior

- Link gets printed like this: /my-page?myparam[0]=2021&myparam[1]=2021

Proposed resolution

Issue seems to be in line https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C... that gets the options wicked up.

If we change that line to

$options = NestedArray::mergeDeepArray([$element['#url']->getOptions(), $element['#options']], TRUE);

it will work, but I'm unsure of the underlying effects of this.

Remaining tasks

- Analyze proposed solution
- Create patch and commit

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Comment	File	Size	Author
#4	3191456-drupal-use_merge_deep_array-4.patch	837 bytes	kporras07
#4	9.2.x: PHP 7.3 & MySQL 5.7 28,037 pass

Issue fork drupal-3191456

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3191456-links-with-query changes, plain diff MR !6212
Check out this branch

About issue forks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

7 January 2021 at 14:50

kporras07 created an issue. See original summary.

Comment #2

cilefen CreditAttribution: cilefen as a volunteer commented 7 January 2021 at 14:58

Is this the same as #3079694: URL query string is broken on render?

Comment #3

kporras07 CreditAttribution: kporras07 as a volunteer commented 7 January 2021 at 15:07

Hmmm, not 100% sure but it looks related. I guess we could concentrate discussion only in one of the issues and then test the other one when fixed.

I'm not sure which issue is the best to keep the discussion.

Comment #4

kporras07 CreditAttribution: kporras07 as a volunteer and at Evolving Web commented 8 January 2021 at 16:41

File	Size
3191456-drupal-use_merge_deep_array-4.patch	837 bytes
9.2.x: PHP 7.3 & MySQL 5.7 28,037 pass

I'm using this patch in the site where I found this issue. Hopefully it will help to trigger the discussion

Comment #5

8 January 2021 at 16:41

Version:

9.2.x-dev

» 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #6

8 January 2021 at 16:41

Version:

9.3.x-dev

» 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #7

8 January 2021 at 16:41

Version:

9.4.x-dev

» 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #8

8 January 2021 at 16:41

Version:

9.5.x-dev

» 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #9

8 January 2021 at 16:41

Version:

10.1.x-dev

» 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #10

pcambra

he/him

Spanish

Asturies

CreditAttribution: pcambra at Cambrico commented 16 January 2024 at 18:04

Status:

Active

» Needs review

I've found this bug on a site today and patch in #4 works great.

Comment #11

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented 16 January 2024 at 18:25

Status:

Needs review

» Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Comment #12

16 January 2024 at 20:29

arisen made their first commit to this issue’s fork.

Comment #13

arisen

Goa, GMT +5:30

CreditAttribution: arisen at Material commented 16 January 2024 at 20:45

1 file was hidden/shown/deleted

File	Size
3191456-drupal-use_merge_deep_array-4.patch	837 bytes
9.2.x: PHP 7.3 & MySQL 5.7 28,037 pass

The patch in #4 resolves the issue. Created a MR.
mergeDeep is internally using mergeDeepArray with preserve integer keys set to FALSE by default.
In the patch we are passing it as TRUE to keep the keys as it is. Should be fine IMO.

This might need some tests.

Comment #14

pcambra

he/him

Spanish

Asturies

CreditAttribution: pcambra at Cambrico commented 17 January 2024 at 14:14

Issue tags:

+Needs tests

@arisen seems you've created the branch but not the actual MR?

Comment #15

17 January 2024 at 14:35

arisen opened merge request !6212

Comment #16

pcambra

he/him

Spanish

Asturies

CreditAttribution: pcambra at Cambrico commented 17 January 2024 at 16:28

Status:

Needs work

» Needs review

Comment #17

arisen

Goa, GMT +5:30

CreditAttribution: arisen at Material commented 19 January 2024 at 18:54

What would be the ideal place to add a Kernel Test for this?

  /**
   * Test Link element with integer key parameters
   */
  public function testLinkWithIntegerKeyParams() {
    $link_array = [
      '#type' => 'link',
      '#title' => 'foo',
      '#url' => Url::fromUri('internal:/my-page?myparam[2021]=2021'),
    ];
    $link_rendered = (string) \Drupal::service('renderer')->renderRoot($link_array);
    $expected_link = '<a href="/my-page?myparam%5B2021%5D=2021">foo</a>';
    $this->assertSame($expected_link, $link_rendered);
  }

Wondering if it can added as part of core/modules/system/tests/src/Kernel/Common/UrlTest.php or core/tests/Drupal/KernelTests/Core/Render/Element/RenderElementTypesTest.php

Comment #18

smustgrave CreditAttribution: smustgrave at Mobomo commented 24 January 2024 at 15:15

Status:

Needs review

» Needs work

For the tests.

Comment #19

arisen

Goa, GMT +5:30

CreditAttribution: arisen at Material commented 29 January 2024 at 09:09

Status:	Needs work	» Needs review
Issue tags:	-Needs tests

Comment #20

smustgrave CreditAttribution: smustgrave at Mobomo commented 30 January 2024 at 20:30

Status:

Needs review

» Needs work

Ran the test only feature and it seemed to pass without the fix https://git.drupalcode.org/issue/drupal-3191456/-/jobs/707698 so not testing what we think it is. While updating please add :void to any new test function.

Comment #21

arisen

Goa, GMT +5:30

CreditAttribution: arisen at Material commented 1 February 2024 at 12:57

Status:

Needs work

» Needs review

@smustgrave The issue is no longer reproducible on D11 but still existing on D10(checked on 10.2.2).

Seems like NestedArray::mergeDeep was not the root cause.
Looks like there has been some changes to the link preprocessing logic.

$options = NestedArray::mergeDeep($element['#url']->getOptions(), $element['#options']);
On D11, $element['#options'] is an empty array so the issue is no longer occurring, whereas on D10 its having the params info same as $element['#url']->getOptions().