Rendered links ignore query array keys

Attached patch against Drupal 10.1.x

Added tests please review

Steps to reproduce

• create a link field on a content type(Link text should be enabled)
• create content with following link "/exampleview?param[89]=89" (Link text should have a value)

Thanks for working on the tests.

Making a new function means a new bootstrap. Since majority of the code is used already in LinkFieldTest think we should be able to add those assertions into an existing tests.

#2 still cleanly applied to 10.1 so removing credit for #16.

added tests to the existing function

This looks great, test coverage is comprehensive.

+++ b/core/lib/Drupal/Core/Render/Element/Link.php
@@ -89,7 +89,7 @@ public static function preRenderLink($element) {
-      $options = NestedArray::mergeDeep($element['#url']->getOptions(), $element['#options']);

I first thought this might have performance implications, and it does , but not because mergeDeep is replaced with mergeDeepArray, that is the same code. This introduces recursion and because of that it can be slower, but this is correct so I don't think we have to do performance testing there?

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

+++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
@@ -856,6 +856,20 @@ public function testNoLinkUri() {
+    $expected_link = (string) $this->container->get('link_generator')->generate('Test link', Url::fromUserInput('/example?parameter[89]=89'));
+    $this->assertStringContainsString($expected_link, $output);

Should we be explicit here and hard code the expected link? I don't think we'd need to allow for the basepath because this isn't a routed link.

My concern is that by using the link generator, we're assuming there's no similar bug in the link generator. Whereas if we hard code the expected URL, we can be sure that the SUT is working as expected.

I agree with #25, that would make the test more robust.

hardcoded the expected link

On local, the patch is not failing

The failure is legit though so can't be merged.

Take into account core tests run in subdirectory.

So this probably needs to be different

$expected_link = ' Test link → ';

Not quiet what I meant. This is being setup to only pass here. If ran anywhere else it will fail with that subdirectory line.

Instead of hard checking a URL should build a URL object.

Instead of hard checking a URL should build a URL object

is it like this?

    $expected_url = Url::fromUserInput('/example?parameter[89]=89')->toString();
    $expected_link = '<a href="' . $expected_url . '">Test link</a>';
    $this->assertStringContainsString($expected_link, $output);

I hope this is the correct way to hardcode the internal link

 $base_path = parse_url($this->baseUrl, PHP_URL_PATH) ?? '';
 $expected_link = '<a href="' . $base_path . '/example?parameter%5B89%5D=89">Test link</a>';
 $this->assertStringContainsString($expected_link, $output);

Definitely better. Lets see what the committer says.

Thanks for keeping with it!

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 2974555-20-test-only.patch, failed testing. View results →

The IS is brief but easy to understand what is happening here. I've added the standard template so remaining steps can be added.

All questions in the comments have been addressed.

I applied the patch and read the code. What caught my eye is that the test is added to testNoLinkUri which is for testing no links, as described in the doc block for that function. So can we move this test to a different existing method? I see that doTestLinkFormatter tests a variety of valid and invalid links. Maybe that would work, or is there a better option.

Setting NW to investigate putting the test elsewhere.