Re: Use behavior thoughts

Hi Anthony. This issue that has plagued me in the past, specifically with the use of traits: Error: http://3v4l.org/VFguK OK: http://3v4l.org/73b86 Although when combined with opcache does causes very confusing behaviour, I do worry that removing the error altogether may make it worse for developers. Take this example with constants: namespace Biz { const Bar = 41; } namespace Foo { const Bar = 42; } namespace Foo { use const Biz\Bar; echo Bar; } // Fatal error: Cannot use const Biz\Bar as Bar because the name is already in use http://3v4l.org/0pRZk An error here is far more useful that trying to exhibit the behaviour of when the namespaces are declared in reverse order: namespace Biz { const Bar = 41; } namespace Foo { use const Biz\Bar; echo Bar; } namespace Foo { const Bar = 42; } // 41 http://3v4l.org/E4PFX I suppose the question here is; Does the declaration order matter? Personally I think it should. /@leedavis81 On Thu, Mar 5, 2015 at 7:40 PM, Anthony Ferrara <ircmaxell@gmail.com> wrote: > 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >