CommentFileSizeAuthor
#5 drupal-n2663752-5.patch555 bytesDamienMcKenna
#2 2663752-2.patch561 bytestwistor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

twistor created an issue. See original summary.

twistor’s picture

twistor CreditAttribution: twistor as a volunteer commented
Status: Active » Needs review
FileSize
2663752-2.patch561 bytes
ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

makes sense, yes. essentially backporting #2531042: Undefined string index 0 in WebTestBase::getAbsoluteUrl() in PHP 7

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Reviewed & tested by the community » Needs work

This should be rewritten to check if(empty($path)) rather than if($path === ''), which is what the patch from #2531042: Undefined string index 0 in WebTestBase::getAbsoluteUrl() in PHP 7 does.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Needs review
FileSize
drupal-n2663752-5.patch555 bytes

Updated to match #2531042.

Crell’s picture

Crell CreditAttribution: Crell at Platform.sh commented
Status: Needs review » Reviewed & tested by the community
twistor’s picture

twistor CreditAttribution: twistor as a volunteer commented

<rant>
I would argue the 8.x code is wrong. empty() should never be used on strings since '0' is considered empty. In this case, it works, but only by sheer luck.

We know that $path is both set, and a string, from the surrounding code. There's no need for NULL/FALSE checking. Explicit is better than implicit.
</rant>

I don't want to change this to NW because it's the way 8.x does it, and it works. Just a pet peeve of mine.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue tags: +Pending Drupal 7 commit
Parent issue: » #2656548: Fully support PHP 7.0 in Drupal 7
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented

#7: You are 100% right. Can you open a follow-up issue to fix it in both versions?

Unfortunately we should use the same code as in 8.x - even if its wrong. However we might be able to get the fix into 8.x - really quickly - as core moves pretty fast these times.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Yeah, I agree, in theory empty() is wrong here but we use it in so many places already where it's technically incorrect (plus Drupal 8 did it this way too) that it is best to leave that to a separate issue.

Committed to 7.x - thanks!

I also credited all the people from #2531042: Undefined string index 0 in WebTestBase::getAbsoluteUrl() in PHP 7 in the commit message here.

  • David_Rothstein committed d100499 on 7.x 
    Issue #2663752 by twistor, DamienMcKenna, ParisLiakos, sdstyles, Berdir...

Status: Fixed » Closed (fixed)

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