Problem/Motivation

On PHP 8 we need to ensure that the fragment is not set to an empty string. See the behaviour change https://3v4l.org/HsvX0

https://github.com/php/php-src/blob/master/UPGRADING#L430

Without this fix 

      // Ensure an empty fragment of # in the URI is discarded as expected.
      [
        'entity:test_entity/1#',
        [],
        'entity.test_entity.canonical',
        ['test_entity' => '1'],
        NULL,
        NULL,
      ],

from \Drupal\Tests\Core\UrlTest::providerTestEntityUris() will fail.

Proposed resolution

To maintain PHP 7 behaviour we need to check the value before setting it.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#2 3156883-2.patch654 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3156883-2.patch654 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented 
    // Discard empty fragment in $options for consistency with parse_url().
    if (isset($options['fragment']) && strlen($options['fragment']) == 0) {
      unset($options['fragment']);
    }

So we're doing things to match the PHP 7 behaviour. Interesting. I wonder what's the right thing to do here. The patch in #2 makes \Drupal\Core\Url::fromUri() behave the same way on PHP 7 & 8. I guess that's the best thing to do.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

Removing the fragment seems fine to me and preserves existing behaviour. If for some reason we wanted to change it, I think it'd be fine to just open a new issue (and that issue could deal with the PHP 7 vs. 8 compatibility, or not if we've dropped PHP 7 by then).

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

RTBC+1, an empty fragment makes no difference so removing it seems fine for now if it keeps the behaviour the same.

  • catch committed d3f654b on 9.1.x 
    Issue #3156883 by alexpott, longwave: \Drupal\Core\Url ensure fragment...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed d3f654b and pushed to 9.1.x. Thanks!

  • catch committed 355c54f on 9.0.x 
    Issue #3156883 by alexpott, longwave: \Drupal\Core\Url ensure fragment...

  • catch committed d4acf8d on 8.9.x 
    Issue #3156883 by alexpott, longwave: \Drupal\Core\Url ensure fragment...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: \Drupal\Core\Url ensure fragment is not an empty string » [backport] \Drupal\Core\Url ensure fragment is not an empty string
Version: 9.1.x-dev » 8.9.x-dev

Actually this is a straightforward one to backport, so even if we don't manage that for every PHP 8 issue, there's no reason not to. Cherry-picked to 9.0.x and 8.9.x too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: [backport] \Drupal\Core\Url ensure fragment is not an empty string » \Drupal\Core\Url ensure fragment is not an empty string

Status: Fixed » Closed (fixed)

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

solideogloria’s picture

solideogloria CreditAttribution: solideogloria commented

I'm curious, why was this changed? An href needs to be non-empty for a link to work, and in HTML an href with an empty fragment of # should be used if the link is something to open a modal, for example.

See these questions of people trying to find workarounds.

https://drupal.stackexchange.com/q/229877
https://drupal.stackexchange.com/q/231491