CommentFileSizeAuthor
#5 drupal-n2663752-5.patch555 bytesdamienmckenna
#2 2663752-2.patch561 bytestwistor

Comments

twistor created an issue. See original summary.

twistor’s picture

twistor commented
Status: Active » Needs review
StatusFileSize
new 2663752-2.patch561 bytes
ParisLiakos’s picture

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 TN, USA
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 TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-n2663752-5.patch555 bytes

Updated to match #2531042.

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community
twistor’s picture

twistor 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 commented
Issue tags: +Pending Drupal 7 commit
Parent issue: » #2656548: Fully support PHP 7.0 in Drupal 7
fabianx’s picture

fabianx 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 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.