Establish standards for namespace usage [no patch]

Every PHP tutorial I've ever read in my life (such as http://www.php.net/manual/en/language.oop5.basic.php) has used the following syntax for instantiating an object:

$c = new ClassName();

So I would reject any of the $f = new Drupal*Subsystem*Foo();-type variants on their face for that reason.

Looking at PHP.net's documentation for namespaces http://ca.php.net/manual/en/language.namespaces.nested.php shows:

namespace MyProject\Sub\Level;

const CONNECT_OK = 1;
class Connection { /* ... */ }
function connect() { /* ... */  }

Nowhere is there reference to a \Foo\bar syntax, so let's reject that as well.

The "use" documentation I couldn't really make heads or tails of http://ca.php.net/manual/en/language.namespaces.importing.php but I think your proposal is inline with the suggestions there.

So in other words, I think the proposed resolution is fine, though I lament the additional complexity this pushes down to module developers. :(

I'm against ruling out the $f = new Drupal\Subsystem\Foo(); syntax because its handy for one off references and explicitly specifying the namespace where it might otherwise be confusing. Crell has voiced that this can be handled with aliases which if that's that solution need to be discussed here. I think that sometimes that could be useful but breaks the simplicity of a language construct provided to us.

I'd like to hear why Symfony doesn't use the leading /. That said, I think its probably fine and a good idea we stay consistent with them. I seem to remember it existing as a under-documented feature in C++ too that existed so you could say "using namespace Foo::std" but also reference "::std" so the same logic probably applies to PHP.

I think the more important we should provide guidelines and and leave some interpretation to the developer to leverage the language. Something like:
1) We don't use leading \'s
2) Only use namespace when defining namespace's as it must be the first line of the file(http://www.php.net/manual/en/language.namespaces.definition.php) and expect it has some different semantics.
3) Prefer use over explicit inline resolution except where it makes the code more clear.
4) Notes on aliases which I don't really have any ideas on ATM.

I'm unsure if anyone has noticed, but the day after webchick's comment, the PHP Nested Namespace documentation page was updated. It now reads:

<?php
namespace MyProject*Sub*Level;

const CONNECT_OK = 1;
class Connection { /* ... */ }
function connect() { /* ... */  }

?>

Where the *s are replaced with backslashes.

@Crell it has more then one namespace and clearly shows not using the leading \. I'm not sure it really affects the discussion in anyway though.

Crell: webchick specifically wrote "Nowhere is there reference to a \Foo\bar syntax, so let's reject that as well." , so I am assuming she didn't see the slashes.

@jhodgdon Right. My guess is that the PHP doc page might have been missing the slashes at the time Webchick looked at it. The next day the doc page was updated -- adding the slashes.

Sorry. Codefilter did indeed eat my slashes in #1. The page showed the same content as #4 when I read it.

What it did not show though, and still doesn't, is the \MyProject\Sub\Level variant with a leading \. Which is why I'm saying we shouldn't use it.

Can you provide a code snippet that demonstrates 1) ?

Ran across an example of needing(or at least a logical usage of) the leading \. Documenting it here for discussion: https://github.com/fabpot/Pimple/blob/master/tests/Pimple/Tests/Service.php

Basically, its a class in the Pimple\Test namespace extending a class in the global namespace. It might also be logical(if not necessary) for us to use this in situations like:

namespace Drupal*Foo;
class Bar extends *Symfony*Component*Baz {}

Bumping this to major, would be good to get this resolved before we add any more namespaced code.

The leading slash seems good to me, it definitely wipes out potential conflicts. (namespace A\B\A\B for example could create conflicts if you use A\B\Something within \A\B).

@Pounard same reason I'd prefer the leading \ too.

I'd like to suggest the following pattern:

namespace {module-name}\{module-name-of-api-being-implemented}\ClassNameSuffixedWithBaseClassName;

For example:
core/modules/entity/entity/Entity.inc

namespace entity\entity;
class Entity {}

core/modules/node/entity/NodeEntity.inc

namespace node\entity;
  use entity\entity;
class NodeEntity extends entity\Entity {}

Or:
core/modules/field/field/FieldWidget.inc

namespace field\field;
class FieldWidget {}

core/modules/text/field/TextareaFieldWidget.inc

namespace text\field;
  use field\field;
class TextareaFieldWidget extends field\FieldWidget {}

^{And a little bit off topic, I think blocks, pages, forms, input filters, etc, should be controller classes too:
core/drupal/page/Page.inc}

namespace drupal\page;
class Page {}

core/modules/node/page/AdminTypesPage.inc

namespace node\page;
  use drupal\page;
class AdminTypesPage extends page\Page {}

According to http://php.net/manual/en/language.namespaces.importing.php:

Note that for namespaced names [..], the leading backslash is unnecessary and not recommended, as import names must be fully qualified, and are not processed relative to the current namespace.

So yes, seems obvious to me.

Sorry for my previous post. Could you give me directions to that thread please?

Dangit, that's pretty clear. I guess no on the leading slash which is against my preference. :(

Sure, I'm comfortable with the current proposal. Thanks casey for finding a definitive resource on the leading slash question. Sorry, neclimdul!

Just so I don't open this from the commit queue again ;)

Yay, this issue is a quick win!
Let's write OO!

Let's do this actually.

Since the bulk of these standards are already established, calling this "normal" now.

But reopening based on discussions in #1321540: Convert DBTNG to namespaces; separate Drupal bits. There's a DX trade-off between the "use" keyword and absolute references to the class.

In Drupal 7, if you want to implement hook_query_alter(), as with every other hook, you go to its api.d.o page, copy the example, paste it into your module, and start hacking.

In Drupal 8, if DBTNG is moved to PSR-0 and we keep the "use" recommendation, this would change to, go to its api.d.o page, copy/paste the example into your module, then figure out where in the bowels of includes/Drupal the AlterableInterface class lies in, then scroll to the top and add a 'use' clause for that, then scroll back down to your pasted code and start hacking.

If we instead used absolute references like Drupal\Database\Query\AlterableInterface in the function signature, we'd retain the same DX as D7.

Additionally, David Rothstein notes (http://drupal.org/node/1321540#comment-5413124):

I really don't understand the benefit of use. I'd prefer the code to be more explicit inline, since it gives me a lot more context. For example, if I'm reading through code and see Drupal\Database\QueryAlterableInterface then I know immediately that it's part of Drupal's database API, whereas if I just saw QueryAlterableInterface I'd have to guess what it's for.

Thoughts?

I agree with reopening this discussion. I would tend to think that in pure OOP/PSR-0 code following a strict standard seems good (always use the "use" statement) because files will always be small enough to see the use not far; But in procedural code where some module files can be multiple thousands lines, we could use the fully qualified names in there, and use the "use" statement in those files only when we need to use the same class name more than once.

I think code examples that are designed to be pasted into modules should work without having to modify namespace imports at the top of the page. For this to work all classes / interfaces they refer to must be qualified, e.g. Drupal\Database\QueryAlterableInterface

However, if a file is referring to the same class multiple times, it makes sense to import the class to "optimize" the developer experience of the file. It's really just a shortcut to things that are used frequently.

So I think both should be permitted. It's okay to give developers a bit of flexibility.

On another note, there is a mistake in the standards:

Note that due to PHP's string handling the namespace separator must be escaped: 'Drupal\\Context\\ContextInterface'

This is not true - it is perfectly fine to have unescaped backslashes in single quotes - I do it all the time and it looks much better. The only exception is if the backslash is to be the last character in the string. In this case it must be a double backslash, otherwise it would be a literal single quote. This situation doesn't arise unless you are composing a qualified name of a namespaced resource via concatenation.

Yeah, I would be fine amending the coding standards to just say that things like hook implementations and theme functions should use the fully-namespaced name, else fall back to 'use' (particularly when something's used more than once in a file). That would get rid of most of my DX concerns, I think.

If the standard is "only for OO code" or "only for things that aren't hook implementations and theme functions" I think that would be a little confusing. Looking at the patch in #1321540: Convert DBTNG to namespaces; separate Drupal bits, that means places like node.module would not use "use" statements, but various simpletest files would.

How about instead, only use it for code that is already namespaced? In other words, if you have a namespace XYZ; statement at the top of your .php file, you should go ahead and use "use" statements also, since you're already living in a world where different rules apply. Because once you're in a namespace, you already have to put things like use Exception; at the top of your file (unless you want to type "\Exception" rather than "Exception" everywhere else, which is weird).

So in core, that would basically boil down to using this syntax in OO libraries within the includes/ directory, but using the full class names everywhere else.

Works for me!

While I agree with Crell, I also have a lot of experience in Java and a bit in C# development: by convention, people almost always uses "import" (for Java) or "using" (for C#) statement (as is "use" for us) but put intentionally the fully qualified names sometime, when they just use the class name once and/or it creates a name conflict (even if in PHP we have the "as" statement for this, sometime just for one usage, what the hell); It's like a commonly agreed convention infringement.

I would keep the "use" as standard, but accept to bypass it in minor cases when it doesn't cause any readability problems.

In the case of the node module query alter hook signature, for instance, I'd definitely let the fully qualified name because it appears only once and we don't really need the use statement whether for readability or DX, standard or not. A rule without exception is not a rule, and I know what I'm talking about I'm french and my native language is all about exceptions ;)

My feeling is that it's OK to break the rules sometime if we feel the readability is better this way: the only pre-condition is we have to agree at review time on a per-case basis.

"if a class name is used in a function or method signature only once in a file, it *may* be given in full form at the developer's discretion."

I think I could live with that. I'd like it even better if we appended ", for example if a class is used only once in a file." to it.

OMG. READING. I should try it sometime.

I mostly agree with the last 4 messages.

Works for me too.

Ship it! :)

Probably append a bullet to the end of the '"use"-ing classes' heading?

It probably goes without saying since I voiced this concern earlier but I totally support being more flexible for one off uses of fully qualified namespaces as well. Glad we found a use case and addressed it so quickly before namespaces where heavily adopted!

As written, this means that when adding certain new hook implementations to a module, I have to go back and audit some of the other hooks I already have (and possibly change their coding style in response to the mere existence of the new hook). I don't see what benefit that brings in terms of developer experience or code readability, and as coding standards go it doesn't seem like it's either simple or consistent.

I'd like to see more comments on #35.

From the point of view of a coder rule, I was thinking something like this:

"Fully qualified class names are allowed in function signatures, but not anywhere else."

When we get to the point where entities are all classed, and field API is implementing entity hooks, then it is going to be a lot of entity hook implementations in that file and it seems reasonable to allow a use statement there - even if entity.api.php is using the fully qualified class names, so I'm not sure about making it a strict requirement that you can never put a use statement outside of a namespaced file (although we might possibly want to do that for consistency in core).

That doesn't sound like what I proposed in #35... The proposal would require using the shortened class names + "use" in large parts of the codebase (in practice, probably most of the places that actually refer to such classes).

That said, I'm curious how much pain and agony there really could have been. Saving people from typing a few extra characters doesn't seem like a big deal to me when weighed against other considerations, and in general, our coding standards don't prioritize that - if they did we'd name all our variables like $a, $b, $c :) If the only benefit from namespaces were that less typing is required, I don't think it would be worth adopting; but fortunately there are lots of other benefits you described in #1240138: Adopt PSR-0 namespace convention for core framework classes (easy autoloading, compatibility with other libraries, etc), all of which are compelling regardless of how many characters we ask people to type.

My guess is we discussing all this as it was a really important detail, but I think it's only a matter of getting the right things close together in one place on the screen (quote from Joel on Software).

While having the strict "use" usage as convention is a good idea, we have, on per-case basis, to cool off about the convention when it makes the code harder to read. When I'm gonna use a class everywhere in the PHP file, I have to do a proper use of the "use" statement because I know that it will allow me more flexibility in term of readability and refactor; Nevertheless if I use my class name only once, I won't disagree breaking the convention if it keeps the right things close together in one place on the screen, because I don't want the next developper to have to scroll the whole file to find out where does it comes from.

Yes, I said it: break the convention, or doing something illegal, we probably should keep those exceptions illegal per the convention but agreed on a review basis and just not writing it into the convention, or just add a small asterisk in the convention saying but it's OK to use a fully qualified name if doesn't make the refactor harder but it clearly keeps the right things close together in one place of the screen.

Just to throw another idea into the mix, what do you all think about "using" only up until the subsystem / package name, and having code be explicit in resolving finer resolution than that?

For example, instead of:

use Drupal\Core\Database\Query\Condition;
use Symfony\Component\HttpFoundation\Request;
...
function (Request $request, Condition $condition) {
}

we instead standardize on:

use Drupal\Core\Database;
use Symfony\Component\HttpFoundation;
...
function (HttpFoundation\Request $request, Database\Query\Condition $condition) {
}

Is that the best of both worlds (reduce unnecessary clutter, but keep things clear) or the worst of both worlds (code not blindly copy/pasteable, but still containing ugly slashes)?

This seems wrong to use relative class name, it's even more confusing.

re: #53
What if at some point someone wants to replace Symfony\Component\HttpFoundation\Request with an API compatible Foo\Bar\Baz\Request?

I guess the standards should mention a special case on *.api.php files:
- classes/interfaces appearing in hook signatures type hinting
- classes/interfaces appearing in the sample functions bodies

Those should probably have their namespaces fully inlined - we can't rely on a bunch of "use" statements at the top of the file ?
(especially since the code in the functions bodies might come from several completely unrelated modules - or completely made up)
Will make copy/paste from existing code less straightforward, though.

I agree with yched about .api.php files.

So...

I'm looking at http://drupal.org/node/1353118 and there are no examples in there of how to do API docs for these namespaced classes. It's kind of mentioned at the top, but some examples would really help. And we should possibly get some examples onto the Doxygen reference at http://drupal.org/node/1354 ?

Also, I'm very concerned about how the API module should handle them -- how to display these namespaced classes on api.drupal.org, etc. There's an issue for that:
#1507476: Support namespaces in API module
I am really clueless as to what we would want the API module to do, much less how to do it, so if anyone has suggestions on what we want to happen, please clue me in. :)

Just a quick question: When using "instanceof" you have to use the class' full name with leading backslash (when you are inside a namespaced file). Should this be mentioned in the namespace documentation or is it too obvious?

Clean up comment regarding leading \.

Thx for the quick reply, Crell. I'm not knowledgeable on this topic - I'm just posting here based on recent experiments.

instanceof takes a class name directly, in which case it's subject to the same namespacing rules as any other class.

By this you mean that if the class name contains backslashes, it doesn't need a leading backslash? My testing indicates, the opposite. Example:

In core/modules/action/lib/Drupal/action/Plugin/views/field/BulkForm.php (which is a namespaced file), you'll find

$this->view->style_plugin instanceof \Drupal\views\Plugin\views\style\Table

Removing the leading baskslash here breaks the test. However, in other non-namespaced files, the leading backslash doesn't matter.

In a string, yes, a class name must be fully qualified and include a leading \, which I think is already in the coding standards, no?

The docs I pointed to says When specifying a class name in a string, use its full name including namespace, without leading \. (This isn't relevant for my example above, anyway, is it?)

This works:

use Drupal\views\Plugin\views\style\Table;
$this->view->style_plugin instanceof Table;

Crell: I have no idea, and I don't have time today to read through the comments here to see if there is a reason this thread is still open (something still missing from the namespace standards?). Sorry... my schedule should be better next week but I've been really busy all December and into this week.

What you're running into isn't coding standards, it's just the way the PHP engine handles namespace resolution. #64 is the Drupal-preferred way of handling namespaces.

Thx, Crell. So the answer to my question was, yes, it's too obvious and not worth mentioning. Sorry about the noise ;-)

OK, I skimmed through this issue. It was closed/fixed sometime before comment #29. Then webchick reopened it in #30, and a proposal was made in #35 for a standards change, which people have not agreed to or completely rejected... As far as I can tell, that is apparently what it is still open for.

I think the reason for this being open can be summarized by looking at the top of node.module right now in Drupal 8:

use Symfony\Component\HttpFoundation\Response;

use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\Database\Query\AlterableInterface;
use Drupal\Core\Database\Query\SelectExtender;
use Drupal\Core\Database\Query\SelectInterface;
use Drupal\Core\Datetime\DrupalDateTime;
use Drupal\Core\Template\Attribute;
use Drupal\node\Plugin\Core\Entity\Node;
use Drupal\file\Plugin\Core\Entity\File;
use Drupal\Core\Entity\EntityInterface;
use Drupal\entity\Plugin\Core\Entity\EntityDisplay;

Many of these only have a tangential relationship to what the node module actually is doing; they are referred to once in the entire file in some random part of the module's code thousands of lines down from the top. The concern people have raised about this is that this hurts readability and maintainability of the code in the rest of the file, without any real benefit.

I think the specific proposal in #35 will no longer work since namespaced classes are now being used in many more "higher-level" parts of the codebase than they were at the time I wrote that. Reading other comments above, there seem to be two possible solutions that remain:

1. Be more lenient about when "use" statements are required, and only use them when there's a real benefit. This would add some inherent ambiguity to the coding standards, but maybe not as much as we think. It could be something like: "If a class is of fundamental importance to the overall purpose of the file, 'use' it at the top." In the case of node.module, I think that means use Drupal\node\Plugin\Core\Entity\Node would remain and the others would go away.
2. Split up all files in Drupal core so that each file is relatively short and serves one main, focused purpose. (This mostly impacts procedural code, but there are some very long classes too that would need to be broken up.) This would mean extensive use of hook_hook_info() to put hooks inside many separate [module].[hook].inc files. The [module].module file itself would be left for API functions that the module provides (or that could be broken up further if it's still too big, like Field module does). This is likely a good direction overall, but would be a pain to actually implement.

I'm not sure either of these is a great solution on its own, and both have their problems. In an ideal world, we might do both; I think if node.module were a few hundred lines long instead of a few thousand, and had a single use Drupal\node\Plugin\Core\Entity\Node at the top, this would be both usable and sensible.

I can't remember why and when and for what reasons we rejected or objected to it, but importing namespaces instead of individual classes, as @Crell described in #70, has always been the best proposal from my perspective.

I'd even be tempted to potentially allow developers to scale it down even further by encouraging imports of "top-level"/"component" namespaces; i.e., preferring the primary namespace instead of a deeper one in almost all cases:

use Symfony\Component\HttpFoundation;

use Drupal\Core\Cache;
use Drupal\Core\Database;
use Drupal\Core\Datetime;
use Drupal\Core\Entity;
use Drupal\Core\Template;
use Drupal\node;
use Drupal\file;
use Drupal\entity;

function foo(Database\Query\AlterableInterface $query) {
  $blah = new Database\Query\SelectExtender();
}

// Assuming some entity plugin/namespace sanity, as discussed elsewhere:

function mymodule_node_insert(node\Entity\Type\Node $node) {
  foreach ($node->mymodule_files as $file) {
    if ($file instanceof file\Entity\Type\File) {
      $file->delete();
    )
  }
}

Importing individual classes works well in smaller files with a well-defined scope. For larger, de-facto scope-unlimited files, importing entire namespaces might work better (but not necessarily).

In any case, the maximum I'd recommend to do here is to provide a recommendation ("MAY" or possibly "SHOULD" in spec talk), but definitely not a standard (not "MUST"). That is, because I think decision should be based on human, logical sanity - not on pedantic standards. I.e., if it makes more sense to import a namespace, do it. If it's clear that you'll only ever need a single class from another namespace, just import that and be done with it - no reason to only import the namespace.

As a result, I'd recommend the following guideline:

Importing namespaced code

Namespaced code is imported via use statements at the top of a PHP file.

• use statements MUST be located after the initial @file phpDoc block, separated by a newline.
• use statements SHOULD be ordered alphabetically, and Drupal core namespaces should be imported before Drupal extension namespaces, e.g.:
  

  use Drupal\Component\Uuid;
  use Drupal\Core\Cache;
  use Drupal\file;
  use Drupal\node;
  use Symfony\Component\HttpFoundation;
  

• If it is clear and foreseeable that a particular file will only need a particular class of another namespace, you SHOULD import class directly; e.g.:
  

  use Drupal\Core\Cache\CacheBackendInterface;
  

• In case multiple resources are needed from another namespace, or the situation isn't clear yet, you MAY import the primary namespace; e.g.:
  

  use Drupal\Core\Cache;
  

  Note: For consistency and class name sanity, you MUST NOT import sub-namespaces of another namespace, since those can too easily lead to ambiguous/confusing but also conflicting class name references; e.g.:

  // Don't do this:
  use Symfony\Component\HttpKernel\Exception;
  
  // This would be weird:
  throw new Exception\AccessDeniedHttpException();
  
  // And this would conflict:
  use Drupal\Component\Plugin\Exception;
  
  throw new Exception\PluginException();
  

How's that?

Best part about the last couple comments? I think we've identified what we're looking for which is in my opinion "Discuss namespace importing in procedural code" which is not the original or summarized version of this issue. Standards have been setup but we can iterate on them in a new issue with the current state of the community.

I do not think this is an active discussion, so marking as "fixed".

If it is an active discussion, it should be moved into the Core queue again.