Problem/Motivation

Not exaclty sure what's the difference to PHP5, this might only be a follow-up problem, but PHP 7 doesnt like the $path[0] !== '/') check if $path is an empty string. Since that doen't seem to work on PHP 5 either, not sure what the real problem is?

One test that fails with this notice is Drupal\simpletest\Tests\BrowserTest

What might be a similar problem is that all installer tests have similar exceptions in template_preprocess_html(), where $path is an empty string as well but it doesn't except that. Somehow it doesn't detect that as frontpage I guess?

Proposed resolution

Add an empty check or prevent that it gets there..

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#6 interdiff-4-6.txt455 bytessdstyles
#6 php7-substr-2531042-6.patch1.1 KBsdstyles
#4 php7-substr-2531042-4.patch555 bytesBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented
Issue tags: +Novice

.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

I'm not sure this is novice. I would have uploaded the patch myself if I'd be sure that this i the right fix. But it doesn't make sense. if PHP 5 would get to the same place, then it really should result in the same notice. So why doesn't it?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

At least for the installer case this is why this is happening http://3v4l.org/iuojJ. This is caused by this behaviour change https://bugs.php.net/bug.php?id=62922 and resolved in https://github.com/symfony/symfony/pull/15282.

Where is our - closed upstream fix status?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review
Parent issue: » #2454439: [META] Support PHP 7
FileSize
php7-substr-2531042-4.patch555 bytes

Hm. That means 2.7.3 should have fixed this, right?

Drupal\system\Tests\Installer\InstallerTest doesn't fail anymore it seems with 2.7.2 and 2.7.3, but Drupal\simpletest\Tests\BrowserTest still does. I don't think that's related to symfony, but our own code.

      if (substr($path, 0, $length) === $base_path) {
        $path = substr($path, $length);
      }
      // Ensure that we have an absolute path.
      if ($path[0] !== '/') {
        $path = '/' . $path;
      }

I'm not sure why it only happens there, but what we're seeing is the weird behavior of $path[0] being fine if $path is FALSE but if it's an empty string then you get a notice. PHP7--.

Anyway, I think this means that this patch *is* a valid fix for this issue and I confirmed that BrowserTestBase passes then.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work

Yep I agree this fix looks good. I think we need to fix BrowserTestBase too as it has the same method with the same code.

sdstyles’s picture

sdstyles CreditAttribution: sdstyles at FFW commented
FileSize
php7-substr-2531042-6.patch1.1 KB
interdiff-4-6.txt455 bytes

Hi, fixed it in BrowserTestBase.php too.

sdstyles’s picture

sdstyles CreditAttribution: sdstyles at FFW commented
Status: Needs work » Needs review
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

It would be fixed by 2.7.3 if we where using it. #2548135: Upgrade to Symfony 2.7.3 could use a rtbc and will probably fix this.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

I don't think so, see #4. There are others that are fixed with that update, but those aren't, this code is completely unrelated to whatever symfony is doing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Yeah this won't be - it is our substr() that is causing this - see https://bugs.php.net/bug.php?id=62922. Committed 9c313e7 and pushed to 8.0.x. Thanks!

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

oh i see. +1 err fixed! awesome~!

  • alexpott committed 9c313e7 on 8.0.x 
    Issue #2531042 by sdstyles, Berdir: Undefined string index 0 in...

Status: Fixed » Closed (fixed)

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