From #drupal-contribute:

FrobinRobin
12:18 hi all, in D8 - why is "Drupal::moduleHandler" sometimes escaped? i.e. "\Drupal::moduleHandler->someMethod()"
12:20 well, I guess its any class method but thats the one I'm looking at..https://drupal.org/files/drupal-2045931-replace-module-exists-calls-15.patch

berdir
12:21 FrobinRobin: it's not escaping. It means to use that class from the global namespace and not the one the class is in

FrobinRobin
12:23 thanks berdir, Why would it be used sometimes and not others? examples in that patch (Drupal::moduleHandler()->moduleExists)

berdir
12:23 FrobinRobin: because procedural code (.module etc.) is in the global namespace too, so it's not necessary)

FrobinRobin
12:23 ahhh nice one berdir, that was a very quick and easy answer!
12:24 berdir++

webchick
12:24 FrobinRobin: (love that name) It's good to hear the kinds of things that trip you up though as you're first encountering D8. I can definitely see why that would be confusing.

berdir
12:24 webchick: yep, not the first time..

Ok, so let's fix it. :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Although, as a counter-point:

berdir
12:28 webchick: well, not having to do that is the main reason we put Drupal in the global namespace and didn't make it something like Drupal\..\Service ;)

Though as a counter-counter point, that's great that it works even if you forget to put \ in front of it, but it'd still be less confusing if it were consistent throughout core, and the \ed version is the only one that can be.

webchick’s picture

Priority: Normal » Major

Seriously, now, this is getting absurd. From https://drupal.org/node/2082661:

Drupal 7:

$release = VERSION;

Drupal 8:

// When not in a namespace
$release = Drupal::VERSION;
// When in a namespace
$release = \Drupal::VERSION;

D7 developers are not going to know whether they're in a namespace or not, at least until much later and they understand that stuff. Let's just standardize on \ everywhere.

damiankloip’s picture

That is a good example of how things are at the moment!

Sounds good to me though. If we're going down the route of just one, that's our only choice.

mtift’s picture

I definitely think this is a good idea.

Imagine that you are new to object-oriented programming and you are wondering when to use "\Drupal" vs "Drupal." What do you do? Search the web for "Drupal \Drupal"? While that does produce some helpful results, it seems just as likely that someone might not know to search for that and would instead be wondering if they should use \Drupal::moduleHandle, \Drupal::entityManager, \Drupal::config, etc.

For whatever it's worth, I know for a fact that I have seen people ask this question in issue queues, and I have not been able to find many examples by searching. So not to pick on anybody, but here is one I remember: #1158322-152: Add backtrace to all errors

I'm not saying developers could not figure this out -- just that it might be complicated to know how to find the answer to the question. So if we can make one thing less confusing about Drupal 8, I'm all for it.

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +Coding standards
FileSize
513.79 KB

I wholeheartedly agree.

We either have

 Drupal::
(Drupal::
!Drupal::

or in authorize.php, Drupal:: at the beginning of the line.

So it was pretty easy to replace them all.

git diff --color-words showed that only \ was added.

It's trivial enough to reroll this patch, but often a "needs review" issue will get more attention than an "active" one, so why not.

longwave’s picture

The alternative of course is to use Drupal; in namespaced code where needed, and then drop the backslash everywhere.

Status: Needs review » Needs work

The last submitted patch, slash-drupal-2053489-5.patch, failed testing.

tim.plunkett’s picture

The alternative of course is to use Drupal; in namespaced code where needed, and then drop the backslash everywhere.

Nope, that leads to silly things like

use \stdClass;
use \Exception;
use \PDO;
use \Drupal;
use \Iterator;
use \ArrayAccess;
use \Serializable;
longwave’s picture

Why is that silly? Why have one standard for top-level classes, and another standard for namespaced classes? You also don't need the leading backslash in use statements.

Also note core/lib/Drupal/Core/Config/Schema/ArrayElement.php which already has five use statements like this.

webchick’s picture

We agreed over in #1614186: Coding standards for using "native" PHP classes like stdClass in namespaced code not to "use" anything without a backslash in their fully-qualified name. "Drupal" falls under this, so does "Exception" and "ArrayAccess." So this is merely bringing this part of the code in-line with the coding standard.

chx’s picture

Status: Needs work » Needs review
FileSize
579.84 KB
500.5 KB

This patch comes in two parts because I was getting a request entity too large otherwise.

Status: Needs review » Needs work

The last submitted patch, webchick_happiness+=1200.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
504.24 KB

Sorry, botched up the regex, the correct one is (?<!\\)Drupal:: and the replacement (in phpstorm at least) \\\\Drupal:: Hope this works.

Status: Needs review » Needs work

The last submitted patch, webchick_happiness+=1200.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
504.25 KB

OMG core changed since I started rolling it :P

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience), -Novice, -Coding standards

The last submitted patch, 2053489_15.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +Novice, +Coding standards

#15: 2053489_15.patch queued for re-testing.

webchick’s picture

Status: Needs review » Fixed

It's green! GREEEEENNN!!

Committed and pushed to 8.x. Thanks!

I'm going to combine the change notice for this one with #1614186: Coding standards for using "native" PHP classes like stdClass in namespaced code which, miraculously, doesn't fail to apply with this patch. So marking fixed.

webchick’s picture

Issue tags: +pants

.

tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
14.46 KB
andypost’s picture

klausi’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
15.54 KB
$ grep -rin '[^\\]Drupal::' *
core/modules/menu/menu.module:502:    '#access' => Drupal::currentUser()->hasPermission('administer menu'),
core/modules/statistics/statistics.module:61:    if (Drupal::currentUser()->hasPermission('view post access counter')) {

Added those two.

Verified that we are doing nothing else here but adding back slashes to Drupal::.

klausi’s picture

Gnah, wrong patch format in #22.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3742257 and pushed to 8.x. Thanks!

cosmicdreams’s picture

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