Please correct this summary where it is technically wrong.

Problem

  1. core/modules/simpletest/lib/Drupal/simpletest/TestBase.php contains:

    namespace Drupal\simpletest;

use Drupal\Core\Database\Database;
use Drupal\Core\Database\ConnectionNotDefinedException;
use ReflectionMethod;
use ReflectionObject;
use Exception;

  2. core/includes/install.inc contains (or was patched for current coding standards to contain):

    use Drupal\Core\Database\Database;
use Drupal\locale\Gettext;
use Exception;

  3. PHP Warning: The use statement with non-compound name 'Exception' has no effect.

    ...but only when running tests. Not reproducible when manually testing the Drupal installer.

    Furthermore, @jthorson manually ran the patch on scratchtestbot on PHP 5.3.3, and actually got a PHP fatal error instead of a warning there.

  4. Removing one of the use Exception; statements (doesn't matter which) resolves the error.

Cause

  • TestBase::prepareEnvironment() loads

        include_once DRUPAL_ROOT . '/core/includes/install.inc';

Proposed solution

  1. Change the coding standards to not import "global names"; i.e., native PHP classes.

Related resources

CommentFileSizeAuthor
#8 use-global-names.txt8.43 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes

Updated issue summary.

chx’s picture

chx CreditAttribution: chx commented

I would love doing that especially for the Exception classes (LogicException esp) because a forgotten use statement does not cause any problems aside from the catch (Exception $e) not functioning at all. Let's justdo catch (\Exception $e) and be happy 'bout it.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

It is still unclear to me though what exactly falls under "global names"...

Is it only Exception? Or is every native PHP class name; i.e., including stdClass, ReflectionClass, PDO, PDOException, etc.pp.?

The PHP manual doesn't really state or clarify that, AFAICS.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Just a question... if the use statement said

use \Exception;

would the PHP warning/error go away?

chx’s picture

chx CreditAttribution: chx commented

While #3 is correct, https://bugs.php.net/bug.php?id=60022 is filed so if we go that route then we need to report on that PHP bug so that they don't "fix" it.

chx’s picture

chx CreditAttribution: chx commented

As for #2, every native PHP class name needs to be rooted yes. It's crystal clear IMO: they are not in a namespace so if you are in a namespace then you need to specify fully every class outside of that namespace either by fully specifying them or using a use statement. It says http://www.php.net/manual/en/language.namespaces.rules.php

To reference any global class in the global namespace, its fully qualified name new \C() must be used.
Crell’s picture

Crell CreditAttribution: Crell commented

Background: We originally went with "use all the things!" as a standard for 2 reasons:

1) Consistency, so that you never need to worry about or use those ugly \ statements inline. (We were concerned about scaring people not used to namespaced code, and the \ was not exactly a popular decision.)

2) Automatic list of used classes, which serves as an informal dependency map.

There's another issue or two around already (I couldn't find them quickly and I'm in a rush right now, but they're there) about making exceptions for things like stdClass and DateTime, and the consistency issue came up again there.

The PHP bug (I mean really, people!) is new. I'd recommend finding one of those existing issues, adding that info there, and closing this as a duplicate.

chx’s picture

chx CreditAttribution: chx commented

I think the only reason the installer doesn't whine about it because the installer has a special error handler and eats errors. I've seen that several times. The include statement is a red herring because namespacing is per file and does not affect included files.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
use-global-names.txt8.43 KB

I stopped counting the people who'd just like to get rid of the use statements for global names, anyway; i.e., not even knowing about this problem.

Doing so

- resolves this problem,

- is forward-compatible with https://bugs.php.net/bug.php?id=60022 (which seems likely to happen, since it essentially removes complexity/special-casing from the engine and makes it adhere to what the manual states),

- follows what Symfony, Doctrine, and others are doing (which leads me to the impression that we've introduced a coding standard without the necessary experience; not knowing what we're doing) [That said, they're not ~100% consistent, but I guess that's rather caused by not applying and ensuring adherence to coding standards to all new code as aggressively as we do.]

- and is easy to explain.

So I'd like just simply get rid of this major issue and problem space entirely, adjust the coding standards and the code, and be done with it.

Attached is a list of affected files + use statements.

Crell’s picture

Crell CreditAttribution: Crell commented

Prior art: #1323082: Establish standards for namespace usage [no patch] and #1614186: Coding standards for using "native" PHP classes like stdClass in namespaced code

This discussion should not be forked; global classes have been discussed at length in other issues and we shouldn't make a decision in this one with only 4 people. Add this information to one of those issues (probably the second) and continue the discussion there.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I cross-referenced this issue in #1614186: Coding standards for using "native" PHP classes like stdClass in namespaced code (the other one seems unrelated)

Not closing as a duplicate yet, because this is about a functional bug. It took me at least an hour to debug and figure out what's wrong and how to resolve the PHP warning.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

FWIW, I completely agree with this, having spent the better part of an hour trying to figure out why my exception was getting eaten, only to discover I was missing a "use" statement for something I presumed to be "built in" to PHP natively. This is an enormous DX issue, and I predict that it will hit every single Drupal developer coming into D8 fresh (and even not-so-fresh, since this bit sun too, apparently).

In addition to the arguments laid out by sun in #8, another compelling argument is msonnobaum at #1614186-13: Coding standards for using "native" PHP classes like stdClass in namespaced code:

"It tells me absolutely nothing to see "use Exception" or "use stdClass" at the top of a file. I dont have to do this in any other language and feels very unintuitive here."

In terms of the consistency argument, IMO it's just as consistent to say "When you're using native PHP classes, such as Exception and stdClass, always prefix the classnames with backslash. Otherwise, put 'use' statements at the top of the file so that people coming in can get a sense of what classes are being used within." And we set good patterns that people copy/paste in their contributed modules and don't get bit by this. Everyone wins.

I do agree that these discussions need to be consolidated, though. 2+ threads is too much.

chx’s picture

chx CreditAttribution: chx commented
Status: Active » Closed (duplicate)

Duplicate then; copying webhick's argument ovre.

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes

Updated issue summary.