Url generator will add '=' after query parameter when value is an empty string

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

At this time we will need a D10 version of the patch please.

Did not code review.

Patch for 10.1.x.

I'm not entirely convinced about this, it seems like it could be disruptive. Here's my two cents:

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -50,7 +50,7 @@ public static function buildQuery(array $query, $parent = '') {
   -      elseif (!isset($value)) {
   +      elseif (!isset($value) || strlen($value) == 0) {
   

   We should use a strict comparison here, either strlen($value) === 0 or $value === ''.

2. +++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
   @@ -423,7 +423,7 @@ private function verifyLinkEscaping() {
   -    $this->assertSession()->responseContains($link);
   +    $this->assertRaw($link->__toString());
   

   assertRaw is deprecated

3. +++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
   @@ -495,7 +495,7 @@ private function doUser() {
   -      $this->assertSession()->pageTextContains($message);
   +      $this->assertRaw($message->render());
   

   Same again here.

4. +++ b/core/modules/search/tests/src/Functional/SearchBlockTest.php
   @@ -93,7 +93,7 @@ public function testSearchFormBlock() {
   -      Url::fromRoute('search.view_' . $entity_id, [], ['query' => ['keys' => $terms['keys']], 'absolute' => TRUE])->toString(),
   +      Url::fromRoute('search.view_' . $entity_id, [], ['query' => ['keys' => ''], 'absolute' => TRUE]) . '=',
   

   Are we losing coverage for something else here?

5. +++ b/core/modules/views/tests/src/FunctionalJavascript/PaginationAJAXTest.php
   @@ -99,7 +99,7 @@ public function testBasicPagination() {
   -    $this->assertEquals('?status=All&type=All&langcode=All&items_per_page=5&order=changed&sort=asc&title=&page=2', $link->getAttribute('href'));
   +    $this->assertEquals('?status=All&type=All&langcode=All&items_per_page=5&order=changed&sort=asc&title&page=2', $link->getAttribute('href'));
   

   This seems like an odd side effect. I'd think in this case you would want to either leave it as it was, or remove the empty query param.

6. +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php
   @@ -24,6 +24,7 @@ public function providerTestBuildQuery() {
   +      [['foo' => ''], 'foo', 'Simple parameters are properly added.'],
   

   Why don't we explicitly mention empty strings here. Simply parameters is pretty meaningless.

Addressed 1,2,3,5 from #20.

Fixing the test failure with above patch. But we still need to address #20 completely

The last submitted patch, 22: 3040048-22.patch, failed testing. View results →

for the comment 4 we are not losing any test coverage only expected URL is sufficient to fix the failing test so, The = sign was removed because it caused the assertion to fail, but it did not affect the functionality being tested. other points mentioned in #20 has been addressed earlier.

Added a related issue for a real issue encountered in contrib-land.

Ugh...removed related issue -- noticed that it already exists in referencing issues. Need another coffee.

Url::fromRoute() was generating = if empty query key is passed.

      $this->assertEquals(
      $this->getUrl(),
      Url::fromRoute('search.view_' . $entity_id, [], ['query' => ['keys' => $terms['keys']], 'absolute' => TRUE])->toString(),
      'Submitted to correct URL.'
    );

Please review.

MR has a valid test failure.

Issue summary also needs some updating. What's the proposed solution.

@smustgrave, Could you please suggest what can be done here?

$terms = ['keys' => ''];
    $this->drupalGet('');
    $this->submitForm($terms, 'Search');
    $this->assertSession()->statusCodeEquals(200);
    $this->assertSession()->statusMessageContains('Please enter some keywords', 'error');

    // Confirm that the user is redirected to the search page, when form is
    // submitted empty.
    $this->assertEquals(
      $this->getUrl(),
      Url::fromRoute('search.view_' . $entity_id, [], ['query' => ['keys' => ''], 'absolute' => TRUE])->toString(),
      'Redirected to correct URL.'
    );

This test case is getting failed as we are passing the search term as empty. Here the search works on the GET request like ?filter='test' as the test case is passing the empty terms to query by default it takes ?keys=. Could you please suggest what am I missing here?