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
- Use fully qualified namespaces inline, for "native" PHP classes, like:
try { $foo = new ∖stdClass(); } catch (∖InvalidArgumentException $e) { }
Pros:
- Symfony does this.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#59 | use-statements-1614186-59.patch | 86.31 KB | tim.plunkett |
#5 | 1614186.patch | 48.29 KB | RobLoach |
Comments
Comment #1
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI think you left out the \ in the first example?
Comment #2
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedPersonally, I am strongly in favor of inline slashes.
Comment #3
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh 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.
Comment #4
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI 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.
Comment #5
RobLoachMake 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.
Comment #6
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLove it. Thanks. Gonna check later, what else would be missing.
Comment #7
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedegrep -r '^use \\?[^\\\\)]+\;' . | grep -v "vendor/" | grep -v ".js" | grep -v ".css"
says only 32 more instances.Comment #8
Crell CreditAttribution: Crell commentedI 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. :-)
Comment #9
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWell ... 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?
Comment #10
Crell CreditAttribution: Crell commentedI 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. :-)
Comment #11
sdboyer CreditAttribution: sdboyer commentedi 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.
Comment #12
neclimdulI 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.
Comment #13
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #14
chx CreditAttribution: chx commentedInline 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?
Comment #15
sun#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.
Comment #16
chx CreditAttribution: chx commentedwebchick in the other issue said it's equally consistent
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 :)
Comment #17
Crell CreditAttribution: Crell commentedThe 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.
Comment #18
tim.plunkettTo counter #17, the rule could be "if the namespace has no \ in it, don't `use` it and add a leading \".
Comment #19
chx CreditAttribution: chx commented#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)
Comment #20
Crell CreditAttribution: Crell commentedThen 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.)
Comment #21
msonnabaum CreditAttribution: msonnabaum commentedThe 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.
Comment #22
webchickThis does not look RTBC yet to me.
Comment #23
catchI 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.Comment #24
catchOr CNR works too.
Comment #25
webchickOr maybe it is, but please update the issue summary with whatever's actually reviewed and tested. ;)
Comment #25.0
webchickOh no, backslashes not supported.
Comment #26
jhodgdonI 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
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.
Comment #27
Crell CreditAttribution: Crell commentedWell 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. :-)
Comment #28
jhodgdonI 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?
Comment #29
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #30
Crell CreditAttribution: Crell commentedCorrect. 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.).
Comment #31
chx CreditAttribution: chx commentedThe issue summary needs no update over this split of hairs. Read again the proposal:
Twig_Template does not have a backslash, so there is no need to
use Twig_Template
.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.
Comment #31.0
chx CreditAttribution: chx commentedUpdate with current summary of what we need to do (I hope)
Comment #31.1
chx CreditAttribution: chx commentedreworded the rules
Comment #32
chx CreditAttribution: chx commentedHere'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
.Comment #33
jhodgdon+1 from me on the really clear text that is currently in the issue summary. Any dissenting opinions?
Comment #34
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedLooks good to me. We came to the same conclusion as Symfony, and in only 33 comments.
Comment #35
webchick"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!!
Comment #35.0
webchickslight fix
Comment #36
chx CreditAttribution: chx commentedAdded an example too. We are getting there.
Comment #36.0
chx CreditAttribution: chx commentedadded example
Comment #37
jhodgdonI 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?
Comment #38
Crell CreditAttribution: Crell commentedI 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.
Comment #39
webchickYep, 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? :)
Comment #40
jhodgdonRE #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?
Comment #41
chx CreditAttribution: chx commentedI 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.
Comment #41.0
chx CreditAttribution: chx commentedslight edit
Comment #42
chx CreditAttribution: chx commented#39 is done.
Comment #43
chx CreditAttribution: chx commented#39 is done.
Comment #43.0
chx CreditAttribution: chx commentedModified code example
Comment #44
jhodgdonI 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...
Comment #45
webchickBeautiful! :) Nice work, folks.
Comment #45.0
webchickMake the example more self-consistent
Comment #46
catchI think this is fixed now?
Comment #47
jhodgdonI 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?
Comment #47.0
jhodgdonClarified remaining tasks.
Comment #48
jhodgdonSince 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?
Comment #49
tim.plunkettThese are the use statements to remove, their files will need the leading slash added:
grep -r "^use [^\ ]*;" * | grep -v "vendor"
will show which files to edit.Comment #50
Eric_A CreditAttribution: Eric_A commentedThis part needs to be updated still? (EDIT: and the actual usage further in the code as well.)
Comment #51
xjmThe 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.
Comment #52
xjmAlso http://drupal.org/node/1353118 totally contradicts this at present...
Comment #53
jhodgdonI 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?
Comment #54
xjmSorry, specifically, the full examples under the bulleted list have
use DateTime;
. Tried to fix it here: http://drupal.org/node/1353118/revisions/view/2379078/2402412Edit: 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.
Comment #55
xjmAlright so here's the complete change:
http://drupal.org/node/1353118/revisions/view/2379078/2402430
Comment #56
jhodgdonLooks good to me, thanks for the edits!
Comment #57
xjmFiled #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.
Comment #58
tim.plunkettRolling a patch.
Comment #59
tim.plunkettOkay, this should do it.
Comment #60
alexpottLooks 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
Comment #61
webchickCommitted and pushed to 8.x. YAAAYYY!!
Needs a change notice. Working on it now.
Comment #62
webchickDone. :)
Comment #63
webchick.
Comment #64.0
(not verified) CreditAttribution: commentedUpdate issue status