Convert DBTNG to namespaces; separate Drupal bits

If it is to be a standalone library, then it shouldn't be in the same namespace as Drupal. Should it be in namespace DrupalDB?

e.g.

  // Register classes with namespaces.
  $loader->registerNamespaces(array(
    // All Symfony-borrowed code lives in /core/includes/Symfony.
    'Symfony' => DRUPAL_ROOT . '/core/includes',
    // All Drupal-namespaced code in core lives in /core/includes/Drupal.
    'Drupal' => DRUPAL_ROOT . '/core/includes',
    // All DrupalDB-namespaced code in core lives in /core/includes/DrupalDB.
    'DrupalDB' => DRUPAL_ROOT . '/core/includes',
  ));

I suggest DbTng, I don't like all uppercase names (sounds like yelling the name).

Question. Is it possible (or desirable) for Drupal to auto-include DBTNG in .module and .install and .test files as they're loaded? At least .install?

Copy/pasting this everywhere:

+use Drupal\Database\Database;

...is kind of annoying. As a module developer, I am confused why a full bootstrap doesn't give me access to DB functions without me having to do anything. It's also confusing because it's not in every .install and .test file. What's the secret sauce?

Then I'm super confused about this:

diff --git a/core/modules/node/node.module b/core/modules/node/node.module
index 57607cd..be9366f 100644
--- a/core/modules/node/node.module
+++ b/core/modules/node/node.module
@@ -1,5 +1,7 @@
 <?php
 
+use Drupal\Database\Query\AlterableInterface;

I take it that AlterableInterface already pulls in Drupal\Database\Database; .. so is this version required for any module implementing hook_query_alter()? Again, that's weird. Couldn't the code calling hook_query_alter() automatically call that in for module developers? What other "special" hooks like this are out there lying in wait?

Cross-post.

Tracking this as part of the Learn-ability initiative, since there pretty clear DX implications here.

Perhaps better to take the tag at face value; it does affect the learning curve even if it's just "the way it is." Maybe read it as "...and how can we mitigate that ?" rather than a condemnation?

@webchick Packaging is a feature used by a lot of other languages, and now a lot of PHP framework; even if new in Drupal I trust people to get used to it quickly (my guess is a lot of developers did code once or twice with other languages): it's mostly syntaxic and doesn't bring complexity as is.

This is a huge step in the right direction and something that Drupal has been missing for years. As far as the learning curve goes, it's better to teach global PHP standards like this, rather than weird Drupalisms that we currently have.

Could we use the fully qualified name for the class directly in the type hint, rather than the use statement? If it's only ever going to be referenced directly once in the file (which is extremely likely in this case), then I don't see the benefit of the up front import - it's not shorthand in this case, and if we did end up using this type hint twice it's not hard to go back to the use statement later.

One semantic usage of the "use" statement is to provide an index of component dependencies in the top of the file; But in this particular case since we have only one usage, I have to agree with catch that it's OK to use the fully qualified name instead.

@Crell. We just added those coding standards, so if we wanted to do this we could change them to reflect that.

I'm not sure we'd want to though. As Pounard says having all the use statements at the top could be a handy index of all classes used in the file (not really dependencies though, node uses classes here that aren't accessed directly, and just having a hook implementation with a type hint in it isn't a dependency either really). It'd also make the coding standard slightly more complex since you'd need to decide between use and fully qualifying the name depending on frequency.

So I spent some time grilling smart people about this again this morning, and I grok it now. For anyone else who might be confused by it, here's why there are use statements in node.module in the patch above, and what contrib module devs need to know.

Following the PSR-0 conversion, if your code ever says SomeClass, even in a type hint, you must:

1. Replace it with the full namespace, e.g., Drupal\Foo\Bar\SomeClass, or
2. Add use Drupal\Foo\Bar at the top of the file where the class is mentioned.

Edit: And, the "secret sauce" (the why) is actually pretty simple: Because we renamed them, and that's what their names are now.

Regarding the coding standards:

having all the use statements at the top could be a handy index of all classes used in the file (not really dependencies though, node uses classes here that aren't accessed directly, and just having a hook implementation with a type hint in it isn't a dependency either really)

Alright, so this (in bold) is the part that seems ishy to me. Is it more or less confusing to add the use statement when all I'm doing is implementing a query alter hook? (Especially since I can just omit the type hint, and I know our standards on type hinting are not very strict/complete.)

And so this will need a change notification when it goes in, to the effect that all the DBTNG classes are now namespaced, and including explicit instructions for module developers along the lines of my comment in #28.

As far as the learning curve goes, it's better to teach global PHP standards like this, rather than weird Drupalisms that we currently have.

I don't see where the patch is removing any Drupalisms? Rather, it's asking developers to use PHP-isms where previously their code would have worked on its own without anything extra. I think that's why some people are concerned about the learning curve.

That said, I tentatively agree that namespaces are the right way to go here (referencing #1240138: Adopt PSR-0 namespace convention for core framework classes, although it's not totally clear how many of those proposed benefits relate to this particular issue).

But looking at the patch, 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.

At first I thought that at least we'd want to avoid having to prefix everything with "Drupal" and could maybe employ use to get around that, but then I remembered this:

drupal_add_js()
drupal_add_css()
...
[and about a billion other functions]

I haven't heard anyone complain about having to type "drupal_" in front of a function name, so I don't see why typing "Drupal\" in front of a class name is any worse.

So overall I'd vote for not adding any use statements at all, anywhere, and just referring to all classes by their full name. Thoughts?

Also, I would suggest not overusing sub-namespaces here, but rather only using them to refer to pieces of code that are actually semi-standalone libraries of functionality. I think that would give the "\" more meaning to a developer reading through the code. For example, the patch currently has Drupal\Database\Query\AlterableInterface, but I don't see why "Query" is important enough to get its own namespace. I'd suggest Drupal\Database\QueryAlterableInterface instead, and the same goes for most (all?) other internal parts of the database API.

Yeah, given the choice between copy/pasting something kinda weird or having to add some code, then dig around in 20 sub-folders to figure out where the class I want to use is buried, then scroll to the top of the file and add a 'use' clause, then return *back* to my original code, I would much rather copy/paste the kinda weird thing.

Re-opened #1323082: Establish standards for namespace usage [no patch] so we can discuss this more.

And the "Increases learning curve" tag is just a statement of fact. It's not an admonishment, or saying we shouldn't do something. It's just raising awareness about it, especially among people who teach novice Drupal developers, as they often can offer insights as to how challenging (or not) proposed changes would be.

EDIT: Bleh. Sorry. Thought my first comment got eaten.

Ahah, patch frenzie has its own frustrations :) Good job thought for making work the first one upper.

Subscribe.

renames=copies ??

Try some incantations and a rain danse!

Ok, did some review, the overall looks good, here are some notes (they are not commit blocker thought):

• It's sad that drivers' namespaces are not CamelCased, I would prefer MySql instead of mysql
• There are a lot of Exception descendents, I would probably put them in ...\Database\Exception\ subnamespace, but it's a purely orgnization choice; It doesn't really matter
• I would add a DatabaseException discrete exception interface, for easy catching, and make all database related errors implementing it: this allows us to, for example, put transactional related exception being subclasses of RuntimeException and SQL syntax errors being InvalidArgumentException for example
• StatementBase has a bad name, I'd go either for making it abstract and call it AbstractStatement or lose the Base and just call the class Statement
• I would rename StatementEmpty as EmptyStatement instead
• Regarding the few comments upper, I'd probably go for a ...\Database\Statement\ namespace too
• As well as a ...\Database\Transaction\ namespace, but once again, it's not revelant technically, it's purely organisational and highly arguable

That's it, else it's almost RTBC for me, I would like some other people to review too. As Crell said, I used his sandbox in #4 to read the code, not the patch itself.

And yes, I don't care about the 80 chars comment limit, those are stuff that we can fix later easily. I'd say that it's RTBC and almost needs a "revisit later", but we may bloat the "revisit later" tag queue doing that.

#49: dbtngtng-review.patch queued for re-testing.

I didn't read this in extreme detail to see if the new classes have any meaningful changes from the old classes, or if they're just moved around. Assuming all they did is acquire namespaces, then this patch is +1 from me. I reviewed the changes being made to the users of the database system (i.e., core/includes (excluding core/includes/database) and core/modules), and the changes seem totally minor and reasonable to me.

I have some nits about the code comments in install_begin_request() and _registry_update(), but that's not worth holding up this patch: I can post that in a separate issue after this patch lands.

I'm tempted to RTBC this, but the comments in #33, #34, and #54 are totally valid. My own opinion is that those can all be deferred to follow-up discussion, and that it's better to commit this patch as-is in order to unblock other database issues than to hold this patch up on getting those points settled. I suggest we try to get this to RTBC within the next couple days, so if anyone thinks there's something in this patch that must be addressed before it lands rather than as a follow-up, please speak up quickly.

#56: dbtngtng.patch queued for re-testing.

Ok, time to get this in front of people who watch the RTBC queue.

#56: dbtngtng.patch queued for re-testing.

I think we still need to resolve the discussion at #1323082: Establish standards for namespace usage [no patch] but I agree it's not worth holding this mega-patch up for that. It's easier to deal with that later than it is to reroll this one.

My other question from #33 (why is Query separated out, i.e. why do we have things like "Drupal\Database\Query\AlterableInterface" rather than "Drupal\Database\QueryAlterableInterface"?) is somewhat answered by #56, although it feels like we might be setting an unclear standard here. Within the database API, drivers are the things that are obviously pluggable (and therefore logically can have separate namespaces), but I think people think of the rest as a single database API. Also, when you look at the patch there's a lot of lines like use Drupal\Core\Database\Query\Select as QuerySelect which suggests it would be cleaner to lump in Query with the rest... I'm not opposed to leaving it as is, but wanted to bring that up now, since changing it would involve moving lots of code around again.

Also, a few things I noticed while skimming the patch (I do not claim to have done anything more than skim it!). None of these are RTBC-blockers, just things to note for a followup cleaning:

 /**
  * Returns a new DatabaseCondition, set to "OR" all conditions together.
  *
- * @return DatabaseCondition
+ * @return Condition

(and a few other similar instances)... The documentation and @return are inconsistent. Plus, I think the @returns were normally using the fully-qualified class name?

+    // All Symfony-borrowed code lives in /core/includes/Symfony.
+    'Symfony' => DRUPAL_ROOT . '/core/vendor',

Comment and code mismatch.

+  // a PSR-0 class loader.  That avoids a fata circular dependency here, since

Typo.

  * @file
- * Database interface code for engines that need complete control over their
- * result sets. For example, SQLite will prefix some column names by the name
- * of the table. We post-process the data, by renaming the column names
- * using the same convention as MySQL and PostgreSQL.
+ * Definition of Drupal\Core\Database\StatementPrefetch

The old code comment had a lot more detail. Is it not worth keeping that as a second paragraph of the @file documentation?

This patch takes David's notes into consideration, and fixes an empty line ending in DatabaseException.php. I didn't touch the Query and DatabaseCondition -> Condition stuff as that might need some follow up documentation fixes.

Still not actually using git a year after adopting it is painful.

@Crell: Same goes for issue summaries, which should be actively used and updated to contain all the important/required information about a change proposal, so people don't need to read a plethora of comments in detail. I had to use browser search to find the hint about the branch and repo in the comments.

Add a todo followup for a blocked issue.

This is just a re-upload of #56, back to RTBC as per #60.

Doing a diff with #65, the only thing I could see that #65 added was #63.2 and #63.3, both of which are minor and can be deferred to follow-ups. @Crell, @Rob Loach, feel free to post new patches if you've committed stuff to Crell's branch in the meantime, and want that reflected here, but if Dries or catch are applying a 3-day minimum RTBC time, then please let's not reset that clock for trivial stuff.

Also, if you want committers to do something non-standard to their normal workflow, ideally that summary would have the exact git command that's to be copy/pasted.

The use ... as QuerySelect stuff is, I believe, all in *drivers*; in that case we would need to use that syntax anyway, since we have a class called Select in the driver (which is its own namespace) and the parent class, also called Select. Query as a separate namespace has no impact on that question.

Well, what I meant was if Query weren't a separate namespace, then instead of "Drupal\Core\Database\Query\Select" it would be "Drupal\Core\Database\QuerySelect", so there wouldn't be a need to do a "use ... as QuerySelect" because that would already be its name.

We still would have a "use", but my point was actually to avoid the "as". (I can see how that was confusing based on what I wrote; I should have put the "as" in bold or something.)

Like this:

namespace Drupal\Core\Database\mysql;
use Drupal\Core\Database\QuerySelect;
class Select extends QuerySelect {}

It's one less layer of indirection for people trying to see where a particular class is being extended in the codebase.

"as" statement is not confusing as soon as you just use it in the class definition (extends or implements for example) because the use statements are right on top of it anyway.

This is a non issue.

Like David, I don't understand why it's better to have Query as its own sub-namespace. SelectQuery, AlterableQueryInterface, and QueryCondition all sound like better class names to me than Query\Select, Query\AlterableInterface, and Query\Condition. However, I'm willing to go along with the Query sub-namespace for now, under the condition that one of the following happens as a follow-up:

• Either, Drupal\Core\Database moves to a separate github project (e.g., DbTng) and we bring it into Drupal under core/vendor, in which case stylistic decisions within that namespace become a concern for that project, and not Drupal.
• Or, if we leave it as code within Drupal core proper, then we have a follow-up issue to discuss the pros and cons of a Query namespace. Mostly, so that when we move other Drupal classes into namespaces, we have a clearer understanding of how fine-grained we want to be with namespaces.

I'm not sure how that relates to a namespace question, though...

Well, if the code is in core/lib, then it's Drupal core proper code (whether Drupal\Core or Drupal\Component), and therefore needs to conform to core style standards, and we still need to determine what those standards are with respect to when to make something within a subsystem/component into a separate sub-namespace. I don't think that question is settled, so we need a follow-up issue to discuss. OTOH, if the project moves out of Drupal entirely, then it's a style standards question for that project, not for Drupal. Just like none of us are debating the organization of classes within Symfony, or at least, we're not doing it in Drupal's issue queue.

In any event: Committers, please put this thread out of its misery!

Agreed. We're at 3 days from #60, so let's do it!

I'd like to delegate this to catch as has been more closely involved with some of the earlier namespace discussions. Catch, care to take this one on?

Would be good to have the follow-up issue for sub-namespaces opened before this goes in. I also don't see why we need a separate Query namespace, but since this is the first of possibly several patches re-organising the database layer we've got time to change that later depending on what we come up with in that follow-up issue, so don't feel like it needs to block this (and looks like no-one else does either).

I'm short on time at the moment so don't expect super-quick commits this week but I'll get to it when I can, and I won't commit any db layer patches until it's in to save re-rolls.

Ok, created that follow-up task at: #1460476: Figure out sub-namespaces for DBTNG, establishing precedent for other subsytems.

Committed/pushed to 8.x.

This will need a change notification.

Great.

However, the commit log/history looks very very awful now. The common practice (AFAIK also used in the Linux kernel) is to prepare a to-be-merged branch in a separate merge branch upfront - whereas that dedicated merge branch is rebased onto the latest core branch. Hence,

1. all change commits are known to be based on the latest code (which may be the case here now, but we factually don't know)
2. the git commit log/history is clean; i.e., all commits from the merged branch cleanly appear after each other, and there are zero merge commits involved

Additionally, pull request branch maintainers are asked to clean up the merge branch during the rebase; e.g., combine multiple related commits into one (there are plenty of "Add more/fix use statements" commits) and squash other needlessly verbose commits.

That is, because such commits are completely stupid and pointless with regard to the long-term history. Simply have a quick look at node.module's history now — next to a plethora of senseless merge commits, you also see commit messages like "Add more use statements and class renaming.", which are missing context and thus don't make sense in terms of Node module's history.

We should really follow that practice, because core's commit history makes little sense due to this merge now and it's close to impossible to determine and review the individual commits that belong to this change.

In fact, it made more sense to review the patch file, which ultimately contradicts the merge of the branch (and thus, it would have made more sense to commit the patch instead). A properly prepared merge branch avoids all of these issues.

Furthermore:

+++ b/core/modules/dblog/dblog.module
@@ -1,5 +1,7 @@
+use Drupal\Core\Database\Database;

+++ b/core/modules/node/node.module
@@ -1,5 +1,9 @@
+use Drupal\Core\Database\Query\AlterableInterface;
+use Drupal\Core\Database\Query\SelectExtender;
+use Drupal\Core\Database\Query\SelectInterface;

+++ b/core/modules/simpletest/tests/database_test.test
@@ -1,5 +1,14 @@
+use Drupal\Core\Database\Database;
+use Drupal\Core\Database\StatementEmpty;
+use Drupal\Core\Database\StatementInterface;
+use Drupal\Core\Database\TransactionOutOfOrderException;
+use Drupal\Core\Database\TransactionNoActiveException;
+use Drupal\Core\Database\Query\Merge;
+use Drupal\Core\Database\Query\InvalidMergeQueryException;
+use Drupal\Core\Database\Query\NoFieldsException;

ugh. Is this how Drupal code look in the future...?

That's only with one converted subsystem... :(

+++ b/core/modules/search/search.extender.inc
@@ -1,5 +1,8 @@
+use Drupal\Core\Database\Query\SelectExtender;
+use Drupal\Core\Database\StatementEmpty;

@@ -24,7 +27,7 @@
-class SearchQuery extends SelectQueryExtender {
+class SearchQuery extends SelectExtender {

@@ -433,7 +436,7 @@ class SearchQuery extends SelectQueryExtender {
     if (!$this->normalize) {
-      return new DatabaseStatementEmpty();
+      return new StatementEmpty();
     }

I agree with others that it's very hard to trace which class is using/extending which namespace, and imported classes are lacking context.

I can only guess that an IDE automatically digs out the context and shows it to you. But when merely jumping into this file and looking at new StatementEmpty(), it's entirely not clear whether that is a procedural function, a method within the namespace, or something completely different (as in this case).

I almost wonder whether most of the code wouldn't be much more readable if we'd use the fully-qualified name (e.g., Drupal\Core\Database\StatementEmpty) instead of use statements that are destroying context.

Change notices are critical issues, all of them.

Created follow-up issue: #1462472: [policy] Define standard for merging in branches from core sandboxes

Unassigning @catch.

Opened #1462712: Discuss best practices for managing core merges so that we don't derail this issue further. :)

This is the first time we've done this, and I'm glad that there's at least some measure of proper attribution and useful history for the specific changes that went into making a patch that's half a megabyte. :)

Wow. Okay. Much-delayed crosspost. Closing mine as a duplicate of sun's.

Back to active for the change notice.

Sigh: #1463564: Drupal 8 does not work with PHP 5.3.2 (the version shipped with Ubuntu 10.04 LTS).

Thnx crell

@Crell The code interpreter is still buggy, and for hooks implementation people could also use the fully qualified name if the class name only come up once in the code, but aside of those minor notes, sounds good to me.