Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:84350
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.223.175 as permitted sender)
MIME-Version: 1.0
Date: Thu, 5 Mar 2015 14:40:39 -0500
Message-ID: <CAAyV7nERc=GWfP0GrUkjjvMHgLBjDno3_=0nw-y7JzLSZ3ZEHQ@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Use behavior thoughts
From: ircmaxell@gmail.com (Anthony Ferrara)

All,

Currently, use has a few error cases that are rather fragile. For example:


namespace Biz {
    class Bar {};
}

namespace Foo {
    use Biz\Bar;
    $a = new Bar;
    var_dump($a);
}

namespace Foo {
    class Bar {};
}
http://3v4l.org/IPTiQ

That works 100% of the time, on 5.3.0 -> present.

But if we change the order of the two Foo blocks:

namespace Biz {
    class Bar {};
}

namespace Foo {
    class Bar {};
}

namespace Foo {
    use Biz\Bar;
    $a = new Bar;
    var_dump($a);
}

http://3v4l.org/KlXUq

We get a Fatal error that "Cannot use Biz\Bar as Bar because the name
is already in use.

Basically, zend_compile.c is doing a check of the current symbol table
to see if the class is already defined when doing the use.

This is problematic not just for the example above, but when combined
with opcache. Opcache nulls out the symbol tables for file
compilation. So that means that file inclusion order matters when it's
disabled, but not when it's enabled.

This was discovered last night by Drupal 8 developers, when they
noticed that some developers claimed fatal errors, while they weren't
able to reproduce. After some debugging, it turned out that disabling
opcache caused the fatal errors. It was because of this symbol table
change.

The error is fragile. And IMHO it's not really protecting anything
significant, since it's clear within the file what each symbol refers
to (you need to look at use declarations for that anyway).

So, I created a PR to remove this error:
https://github.com/php/php-src/pull/1149

Note that there is no BC break here, as it's removing an error condition today.

This results in a weird edge case (which is 100% valid, but feels odd):

<?php
class Test {}
use Bar\Test;
var_dump(new Test); // class(Bar\Test)

That's perfectly valid, if not a bit weird.

The reverse would error mind you:

<?php
use Bar\Test;
class Test {}

Because you've already defined the symbol Test in the file.

To get around this, I've created another PR with a BC break, adding a
compile error if code comes before `use`:
https://github.com/php/php-src/pull/1150

This requires use to immediately follow namespace declarations:

namespace Foo {
    use Bar; //valid
}
namespace Bar {
    use Foo; //valid, second namespace in file
}
namespace Baz {
    echo "Hi!";
    use Foo; // Invalid
}

This did break approximately 30 internal tests, but they were all
testing this specific code path (or bugs related to it).

What are your thoughts about both PRs? The first seems like a clear
win, the second is a nice cleanup but also a bit of a change to
behavior... I'm not sure if an RFC is required for either, but if
people want one (for both, for one, etc) I can draft something up
quickly.

Remove symbol check: https://github.com/php/php-src/pull/1149
Pin use to top: https://github.com/php/php-src/pull/1150

Thanks

Anthony