Problem/motivation

PHP provides classes like stdClass or InvalidArgumentException. In namespaced code those can't be used directly. Also, putting "use" statements in code with these built-in classes causes problems (see #1766052: Importing of "global names" yields PHP warnings/fatal errors in (un)certain situations).

Proposed resolution

Amend the coding standards (on the Namespaces page http://drupal.org/node/1353118)

a) This bullet point:

In a file that declares a namespace, classes not in that namespace must be specified with a "use" statement at the top of the file. That includes global classes, including stdClass.

Never specify a class name in code with a \ in it. If it will be used, specify it at the top of the file with a "use" statement.

becomes:

Classes and interfaces with a backslash \ inside their fully-qualified name (for example: Drupal\simpletest\WebTestBase) must not use their fully qualified name inside the code. If the namespace differs from the namespace of the current file, put a use statement on the top of the file. For example:

namespace Drupal\mymodule\Tests\Foo;

use Drupal\simpletest\WebTestBase;

/**
 * Test that the foo bars.
 */
class BarTest extends WebTestBase {

Classes and interfaces without a backslash \ inside their fully-qualified name (for example, the built-in PHP Exception class) must be fully qualified when used in a namespaced file: for example new \Exception();.

Remaining tasks

1. [done] Agree upon and adopt the standards.
2. Patch core to remove "use Exception" and similar statements, and precede uses of Exception and other non-namespaced classes in namespaced code with a backslash.

User interface changes

none.

API changes

none.

Previous text from Niklas Fiekas and webchick

Possible solutions

  1. Use fully qualified namespaces inline, for "native" PHP classes, like:
    try {
      $foo = new ∖stdClass();
    }
    catch (∖InvalidArgumentException $e) { }
    

    Pros:

    • Symfony does this.
  2. Use use directives.
    use stdClass;
    use InvalidArgumentException;
    ...
    try {
      $foo = new stdClass();
    }
    catch (InvalidArgumentException $e) { }
    

    Pros:

    • Some PSR-0 patches that got in use this approach, already.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Tor Arne Thune’s picture

I think you left out the \ in the first example?

Niklas Fiekas’s picture

Personally, I am strongly in favor of inline slashes.

Niklas Fiekas’s picture

Oh no ... the <?php ... ?> filter doesn't support backslashes at all:

new \stdClass() new \stdClass()
use Drupal\foo;
use Drupal\foo;
new \\stdClass()
new \\stdClass()
Different issue. Using UTF-8 trickery for the issue summary, now.

Tor Arne Thune’s picture

I prefer inline slashes for classes in the global namespace as well. There is no gain that I can see in having use statements for them when all that is needed is to add a \ in front of them when they are used. The use statement is one extra line. A bonus is that another big project (and probably others) are doing this so developers can be exchanged without them having to change their code style thinking in between.

RobLoach’s picture

Issue tags: +Coding standards
FileSize
48.29 KB

Make it so. Inline slashes for global scope classes. I'm probably missing a few classes here, we might want to split them up into different issues.

Niklas Fiekas’s picture

Title: [policy, no patch] Coding standards for using "native" PHP classes like stdClass in namespaced code » [policy] Coding standards for using "native" PHP classes like stdClass in namespaced code

Love it. Thanks. Gonna check later, what else would be missing.

Niklas Fiekas’s picture

egrep -r '^use \\?[^\\\\)]+\;' . | grep -v "vendor/" | grep -v ".js" | grep -v ".css" says only 32 more instances.

Crell’s picture

I actually disagree with this.

We originally pushed all \ to the use statements for two reasons:

1) Consistency; no need to debate or bikeshed whether it's "appropriate" in this instance or not.

2) Having a full list of classes at the top of the file gives you a nice easily-accessible index of a class's dependencies.

While I agree it's annoying for \stdClass, I do not want to lose benefit 2. I also would prefer to keep benefit one, as we have too many things to bikeshed about as is. :-)

Niklas Fiekas’s picture

Well ... ok. I wouldn't say that \stdClass, \Exception, \InvalidArgumentException etc. are "dependencies", but \PDO is. So consistency is a good argument. Almost conviced. Won't fix?

Crell’s picture

I could see making an exception for stdClass, perhaps, since it's only kinda a class to begin with. And I might be convinced about Exception, because if you forget it you don't get a fatal but weird and hard to debug errors; but anything more than that, I'd prefer to be explicit. And even those two exceptions then beg for extra debate about PDO, DateTime, and on down the line, which leads to inconsistency... and frankly we have bigger fish to fry. :-)

sdboyer’s picture

i see Crell's reasoning in #8, and it makes sense...but it still makes me itch. i mean hey, it works for python, and there definitely are advantages to being able to scan that list. so i guess i just wanna say for the record:

ugh.

neclimdul’s picture

I still disagree and think we can give developers so level of judgement. And really even if we where going to set a 100% of the time rule, I think I'd still prefer to inline the global namespace calls. I've been shot down before though so I'm not holding my breath.

msonnabaum’s picture

I don't buy #8.

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. It's unfortunate that php makes us use \ for globally namespaced classes, but it would be even more unfortunate if we tried to "use" them all.

Also, if you delete the line that has Exception or stdClass, are you going to remember to remove the use statement? I've already found and fixed cases of this in core.

Let's either not standardize it or standardize on inline.

chx’s picture

Inline makes sense because of #13. Code rot-- One of the talking points for annotations was to keep metadata as close as possible to the class declaration, why not apply the same logic and keep path as close as possible to class usage?

sun’s picture

#1766052: Importing of "global names" yields PHP warnings/fatal errors in (un)certain situations actually provides a technical reason for not using use statements for global names.

(Not closing that one as a duplicate yet, because it is not only about coding standards, but about an actual functional bug [exception] being caused by a use statement referencing a non-compound name. So there's at least one patch already that had to be "un-corrected" [violating current coding standards] in order to pass tests and get committed.)

The list of affected use statements and files grew much bigger since the patch in #5.

chx’s picture

Assigned: Unassigned » Crell
Status: Needs review » Reviewed & tested by the community

webchick in the other issue said it's equally consistent

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

As for 2) we do not have a native php function dependency list anywhere so why a native class dependency list? And, anyways, just this dubious benefit is totally killed by the amount of hours already wasted on fixing catch (Exception $e).

I am naming webchick's quote the agreed-upon, moving it to Crell so he can see this and add his agreement :)

Crell’s picture

The implication of #16 would be that:

1) We have a standard of "sometimes there's a \, sometimes not".
2) People need to know what are internal classes vs. non-internal classes in order to know which to use.

If we're collectively OK with those implications, then I can accept #16.

tim.plunkett’s picture

To counter #17, the rule could be "if the namespace has no \ in it, don't `use` it and add a leading \".

chx’s picture

Assigned: Crell » Unassigned

#18 is good! I like that, it's very succint: if the class is namespaced, add a use statement, otherwise the code should contain \className. (Avoiding a use in the English meaning, here)

Crell’s picture

Assigned: Unassigned » Crell

Then you have silent dependencies against user-space global classes.

To be fair, in practice I don't know that we have many (or any?) of those left at this point, so that may be a moot distinction. But one of the reasons for "always use" was that dependency list (which doesn't apply so much for internal classes, unless you're using a PECL module which is just a whole other animal that most Drupalers don't deal with.)

msonnabaum’s picture

The vast majority of globally namespaced classes that we'll use will be built in php classes, so there's no sense in treating it like a dependency.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

This does not look RTBC yet to me.

catch’s picture

Status: Needs review » Reviewed & tested by the community

I think the code rot argument beats the consistency argument so I'm happy with inline \. Since this is still being discussed I'll leave it RTBC a bit longer.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Or CNR works too.

webchick’s picture

Or maybe it is, but please update the issue summary with whatever's actually reviewed and tested. ;)

webchick’s picture

Issue summary: View changes

Oh no, backslashes not supported.

jhodgdon’s picture

I have updated the issue summary with what I believe to be the proposal. Can someone (sun, catch, etc.) who knows what the proposal is please review and see if I've gotten it right, and if not, kindly fix it? Thanks!

I was also not sure if in code we need to do

$foo = new \stdClass();

or if it only applies to "real" classes like \Exception that have methods and such... so if that could be clarified in the issue summary, that might be useful. Just adding stdClass to the examples in the proposed standards would probably be quite helpful in clarifying this.

Crell’s picture

Well there's global classes, and there's internal classes. We've been discussing both. Right now all internal classes are global, but the signals from the PHP Internals team is that may change. We need to decide which we mean, since until #18 we were talking about internal classes, NOT global classes.

And yes, I believe you do need a \ on stdClass(). Which sucks, but meh, it's PHP. :-)

jhodgdon’s picture

I for one am not sure of what global/internal classes are... anyway can someone else edit the issue summary who understands what's going on here?

msonnabaum’s picture

I think this is what Larry means.

Global classes are classes declared in the global namespace (no namespace):

Twig_Template
views_plugin

Internal classes are classes that come predefined in php or in the php standard library:

stdClass
Exception
InvalidArgumentException

It's a confusing way to distinguish them, because they are both declared in the global namespace.

Crell’s picture

Correct. And the last dozen or so comments have been unclear about which one we're talking about saying "don't use, just inline the \".

The "automatic dependency list" argument goes away for internal classes, since they're, by definition, always present. That argument is still valid for user-space global classes (Twig_Template, etc.).

chx’s picture

The issue summary needs no update over this split of hairs. Read again the proposal:

In a file that declares a namespace, classes not in that namespace must be specified with a "use" statement at the top of the file. This only applies to classes that are themselves namespaced (classes with a backslash \ inside their fully-qualified name).

Twig_Template does not have a backslash, so there is no need to use Twig_Template.

Never specify a class name in code with a \ in it. If it will be used, specify it at the top of the file with a "use" statement. The exception is that global-namespaced classes (classes without \ inside their fully-qualified names) must always start with a \ -- such as the built-in PHP \Exception class.

Once again, Twig_Template is such that "classes without \ inside their fully-qualified names". However, I see a wording opportunity here, updating the issue summary.

chx’s picture

Issue summary: View changes

Update with current summary of what we need to do (I hope)

chx’s picture

Issue summary: View changes

reworded the rules

chx’s picture

Here's what I just added to the summary:

Classes and interfaces with a backslash \ inside their fully-qualified name must not use their fully qualified name inside the code. If the namespace differs from the namespace of the current file, put a use statement on the top of the file.

Classes and interfaces without a backslash \ inside their fully-qualified name (for example, the built-in PHP Exception class) must be fully qualified when used in a namespaced file: for example \Exception.

jhodgdon’s picture

+1 from me on the really clear text that is currently in the issue summary. Any dissenting opinions?

Tor Arne Thune’s picture

Looks good to me. We came to the same conclusion as Symfony, and in only 33 comments.

webchick’s picture

"Classes and interfaces with a backslash \ inside their fully-qualified name " could use a "(for example, ...)" like the "without a backslash" version. Otherwise, I have no complaints, and the rule makes sense to me. Thanks, Tim!!

webchick’s picture

Issue summary: View changes

slight fix

chx’s picture

Added an example too. We are getting there.

chx’s picture

Issue summary: View changes

added example

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I just made a slight edit to the summary (noticed a missing close paren and adjusted the example for Exception slightly).

It seems like we might be at RTBC for the issue summary... provisionally setting it there for a few days to see if anyone objects?

Crell’s picture

I would prefer that we limit the "inline the \" policy to internal classes, not just un-namedspaced classes, but if I'm the lone voice in that position then I will leave it be.

webchick’s picture

Yep, I think the issue summary is fine. The only thing I'd suggest is for the coding standards docs:

"If the namespace differs from the namespace of the current file, put a use statement on the top of the file. For example add use Drupal\Core\Config\FileStorage between the namespace declaration and the beginning of class and then inside the class just use new FileStorage($path);"

Can we just have a code example that shows that rather than me trying to create one in my head from a text description? :)

jhodgdon’s picture

RE #38 - correct me if I'm wrong, but I thought the policy on this applying to all classes in the global space was dictated by PHP errors/warnings rather than our preferences?

chx’s picture

I doubt that errors / warnings are relevant here, but I also think that the problems cropping up -- mostly that catch (ExceptionFoo $e) doesn't work -- are the same whether it's a non-namespacing library like Twig or a PHP built-in. It's not wise IMO to differentiate.

chx’s picture

Issue summary: View changes

slight edit

chx’s picture

#39 is done.

chx’s picture

#39 is done.

chx’s picture

Issue summary: View changes

Modified code example

jhodgdon’s picture

I made a tiny edit to the example -- I put the same class name in the text as in the PHP code example, just to make sure it's utterly clear. It's looking good...

webchick’s picture

Beautiful! :) Nice work, folks.

webchick’s picture

Issue summary: View changes

Make the example more self-consistent

catch’s picture

Status: Reviewed & tested by the community » Fixed

I think this is fixed now?

jhodgdon’s picture

Status: Fixed » Reviewed & tested by the community

I don't see the proposed edits at http://drupal.org/node/1353118 -- are we all in agreement?

Also, don't we need a patch afterwards?

jhodgdon’s picture

Issue summary: View changes

Clarified remaining tasks.

jhodgdon’s picture

Title: [policy] Coding standards for using "native" PHP classes like stdClass in namespaced code » [policy adopted, needs patch] Coding standards for using "native" PHP classes like stdClass in namespaced code
Assigned: Crell » Unassigned
Status: Reviewed & tested by the community » Active

Since it seemed we were all in agreement, I went ahead and edited http://drupal.org/node/1353118 -- see the diff:
http://drupal.org/node/1353118/revisions/view/2206618/2379078

The remaining task in this issue is a patch to remove "use Exception" and similar declarations from the code. Any takers, or have these already been removed? Or should it be a separate issue?

tim.plunkett’s picture

These are the use statements to remove, their files will need the leading slash added:

$ grep -r "^use [^\ ]*;" * | grep -v "vendor" | sed 's/.*://' | sort | uniq -c | sort -nr
  43 use Exception;
  18 use PDO;
  18 use InvalidArgumentException;
  14 use stdClass;
   9 use RuntimeException;
   7 use IteratorAggregate;
   5 use ArrayIterator;
   4 use SimpleXMLElement;
   4 use PDOException;
   4 use ArrayAccess;
   3 use Traversable;
   3 use Iterator;
   2 use ReflectionClass;
   2 use RecursiveIteratorIterator;
   2 use RecursiveDirectoryIterator;
   2 use DirectoryIterator;
   2 use DateTime;
   2 use DateInterval;
   2 use DOMDocument;
   2 use Countable;
   1 use ZipArchive;
   1 use SplFileInfo;
   1 use ReflectionObject;
   1 use ReflectionMethod;
   1 use ReflectionFunction;
   1 use PDOStatement;
   1 use LogicException;
   1 use DOMXPath;
   1 use Closure;

grep -r "^use [^\ ]*;" * | grep -v "vendor" will show which files to edit.

Eric_A’s picture

This part needs to be updated still? (EDIT: and the actual usage further in the code as well.)

namespace Drupal\Subsystem;

// Global classes must be imported into a namespace.
use DateTime;
xjm’s picture

The namespace doc should be linked from http://drupal.org/node/608152 probably; I couldn't find it (even though it's a sibling in the menu, duh) or this issue.

xjm’s picture

Also http://drupal.org/node/1353118 totally contradicts this at present...

jhodgdon’s picture

I fixed #51 (added link).

Not sure what you mean in #52 -- what contradicts what? The information from the summary is on http://drupal.org/node/1353118 unless I am mistaken?

xjm’s picture

Sorry, specifically, the full examples under the bulleted list have use DateTime;. Tried to fix it here: http://drupal.org/node/1353118/revisions/view/2379078/2402412

Edit: Those changes are still incomplete; the inline comment in the second example contradicts the usage below it, and the remark underneath the second example is also false.

xjm’s picture

jhodgdon’s picture

Looks good to me, thanks for the edits!

xjm’s picture

Filed #1992950: Stop use-ing Exception. Let's do separate patches for the top 4-5 for reviewability, and then we can lump the rest into one patch.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Rolling a patch.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
86.31 KB

Okay, this should do it.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks great - I've grepped the code base for use statements with no \ and there are none in Drupal code. And I've used --color-words to confrm that the only changes are the expected changes.

If #59 is green this is rtbc

webchick’s picture

Title: [policy adopted, needs patch] Coding standards for using "native" PHP classes like stdClass in namespaced code » Change notice: [policy adopted, needs patch] Coding standards for using "native" PHP classes like stdClass in namespaced code
Assigned: tim.plunkett » webchick
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. YAAAYYY!!

Needs a change notice. Working on it now.

webchick’s picture

Title: Change notice: [policy adopted, needs patch] Coding standards for using "native" PHP classes like stdClass in namespaced code » Coding standards for using "native" PHP classes like stdClass in namespaced code
Assigned: webchick » Unassigned
Status: Active » Fixed

Done. :)

webchick’s picture

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

Anonymous’s picture

Issue summary: View changes

Update issue status