Meta: start converting existing core classes to PSR-0 [policy, no patch]

The database layer I will be migrating sometime after 1 November, as there are some architectural changes we can/should make as part of that migration. 1 November is the "move everything around like crazy" date, after which backport patches will all be broken anyway. :-) I will open a new issue for that. I tentatively agree about moving the thin wrappers into bootstrap.inc, at least for the time being.

Cache system and File Transfer are the other key existing OOP systems in /includes. I would recommend the maintainers for those subsystem take point on the conversion. Both are welcome to contact me any time to discuss the best approach for doing so. (WSCCI is on the leading-edge of namespace usage in Drupal at the moment.)

Should this be refiled as a meta issue?

Updated to include link to extra followup.

You can safely move the procedural handlers to any file, but I would avoid to bloat boostrap.inc, that even itself could hypothetically being replaced by a full OO code.

factories.inc or wrappers.inc seems fine to me.

The WSCCI sandbox already has the \Drupal namespace defined. I'm not sure if Git can handle empty directories so I didn't include it in the initial patch, but it's a one line change to make /includes/Drupal the root for the \Drupal namespace. I suppose I could throw that into a patch and see what happens.

@Creel Git does not handle empty directories, just to confirm.

Well, let's see if the testbot likes this. This is the line that we have in the WSCCI sandbox to wire up Drupal namespace autoloading. Since nothing in core right now has a Drupal namespace this should have no effect on anything, but will "claim" the namespace.

Moved to the summary. Catch, I already created one for the DB. :-) How about #6? Do we want to just put that in on its own so it's ready-and-waiting?

Anyone have thoughts on how the Simpletest classes should be restructured? I'm referring to the tests themselves - the ones that ship with modules.

Can be MyModulePrefix\Test\SomeTestCase

Moshe: I'd say that's dependent on #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch]

Add more spinoff issues.

Please also add the "PSR-0" tag on #1225404: Modern event based lock framework proposal.

This issue is curently a critical task. Could someone clarify what scope of this issue is an actual release blocker? Does *all* of includes have to be converted? If so that seems a bit crazy. Or is this critical to make sure *something* uses symfony's auto loader? Or?

Large parts of language.inc will need to be converted to PSR-0 anyway, since they will have to be ported to the context system. I discussed with Crell the possibility to generalize and make globally available a chained context handler dealing with fallback values (i.e. what is currently done only for language negotiation).

Ok got it. So the scope is classes in core only, with the goal being to rip out our custom class registry.

I have two fundamental/general questions on the currently RTBC patch in #1323120: Convert cache system to PSR-0, which I'd like to see an agreement on before moving ahead:

1. +++ b/core/includes/Drupal/Cache/DatabaseBackend.php
   @@ -0,0 +1,250 @@
   +class DatabaseBackend implements CacheBackendInterface {
   ...
   +  /**
   +   * Implements Drupal\Cache\CacheBackendInterface::getMultiple().
   +   */
   +  function getMultiple(&$cids) {
   

   Do we really need to document the full namespace name in the phpDoc? I would have assumed that this:

   +  /**
   +   * Implements CacheBackendInterface::getMultiple().
   +   */
   +  function getMultiple(&$cids) {
   

   would be sufficient?

   We're already in the namespace after all...?

2. +++ b/core/includes/install.core.inc
   @@ -277,8 +277,7 @@
   +  $conf['cache_default_class'] = 'Drupal\\Cache\\InstallBackend';
   

   Why do we escape backslashes in single quotes? This was briefly discussed in #1323120-62: Convert cache system to PSR-0, but I do not think it is required, nor consistent, and also not good.

   As the PHP manual clarifies, no escape sequences work within single quotes. There are only two exceptions: the single quote character '\'' itself, and the escape character '\\'.

   However, the latter escape character escape sequence exception only exists for the actual purpose of using a backslash at the end of a single-quoted string; i.e., not having the purpose of escaping the single quote.

   Literal example of this purpose: print 'As a PHP developer you may escape characters using the backslash: \\';

   Without escaping the backslash, you get a syntax error.

   I really dislike the unnecessary escaping, because it looks ugly, and additionally disallows to grep the code base for a certain class or namespace.

I really dislike the unnecessary escaping, because it looks ugly, and additionally disallows to grep the code base for a certain class or namespace.

Totally agreed.

@sun

1. Documenting the full namespace allows editors that poorly parse PHP (all of them, in fact) to do proper code verification, completion and browsing. It also avoid to have contextual documentation. It is a topic that can be argumented in both ways, but I prefer using the qualified names in there. For what it worth, Symfony and Zend both do that too, it must exists a really good reason aside of that.

2. Escaping is not unnecessary. May I quote the PHP documentation:

To specify a literal single quote, escape it with a backslash (\). To specify a literal backslash, double it (\\).

Even if the single backslash works, it's documented to double it, I will do as the documentation suggest for clarity matters.

My personal opinion about this, I'm giving it because you gave yours, is that whenever I see a single backslash, my brain automatically assumes that it's meant to escape the character right after: even if it's not. Another side effect is that passing back and forth from single quote to double quote would create unattended behavior while doubling the backslash also make this safe.

For code consistency across the multiple use cases we may encounter, I strongly suggest to always double the backslashes because the backslash itself is the escape character: the fact that it is being interpreted as a literal backslash depending on the character after makes it usage dangerous and less predictable: let's avoid the confusion and always keep the double backslash.

The grep point is true, indeed, but it doesn't matter because when you grep the code, you grep for the real code, not configuration strings. And nothing tells you cannot grep with doubled backslashes too: when you are grepping, you know what you are looking for: it won't block you.

Even if the single backslash works, it's documented to double it, I will do as the documentation suggest for clarity matters.

No, the documentation does not suggest to double it. It states that you should double the backslash to get a literal backslash, after it explained that you may escape the single quote with a backslash.

Being programmers ourselves, we should be able to precisely understand the correlation there: If you weren't able to escape the single quote, then there would be no need to allow escaping of the backslash.

However, escaping of backslashes is in no way enforced, required, or suggested. The optional escape sequence only exists to accommodate for the edge-case that someone wants to not escape a single quote that follows a backslash.

the fact that it is being interpreted as a literal backslash depending on the character after makes it usage dangerous and less predictable

That is not the case. Especially not in this use-case. Fact is that single quoted strings have their own syntax and semantics, not only in PHP, and developers ought to know how they work. Associating semantics of double quoted strings or whatever else to a single quoted string is inappropriate.

Thus, the escaped backslashes disallow easy grepping for class names and namespaces throughout the code base for absolutely no reason.

Backslash remains the escape character, using single or double quotes. Thus when using it, it's more consistent to double it.

May I quote the documentation once again:

To specify a literal backslash, double it (\\).

.

We already had this discussion more than once, in different contextes, it's already been written in the PHP 5.3/namespaces convention draft following those.

Agreed with sun.

The optional escape sequence only exists to accommodate for the edge-case that someone wants to not escape a single quote that follows a backslash

That's how I understand it too. Therefore, I don't understand either the need to use this double escape in our 'namespaced class' strings, which do not fall in that edge case.

The reason to always use \\ is simple, IMO: Consistency. We will sometimes have double-quote strings that have them. I'd rather always know "if you've got a fully qualified class name in a string, \\ it" than have to think "wait, do I have to switch it back and forth now?" when making a new patch.

It's still just as greppable. Searching only for a \ is going to skip any double-quoted class name strings, so you have to do that anyway for completeness. And, actually, since it will be rare (function parameters only, and then not even always) to have a fully qualified class name that's not in a use statement that means most of the time you are grepping for string class names... in which case having all of them be \\ makes it easier to do, not harder.

Again, "consistency" is not an argument. Double-quoted strings and single-quoted strings are completely different things.

Apparently, Symfony, Doctrine, as well as Zend Framework 2 do not escape the backslash in single quotes. (Though Symfony in particular is very inconsistent, but at least within its own vendor code it is consistently not escaping backslashes.)

Thus, here's my killer argument (which I know you guys love):

Everyone does it. No need for another Drupalism.

:)

Oh, snap. ;)

Apparently, Symfony, Doctrine, as well as Zend Framework 2 do not escape the backslash in single quotes. (Though Symfony in particular is very inconsistent, but at least within its own vendor code it is consistently not escaping backslashes.)

Can we have links to the source code? That will stop this discussion. If it's true what sun says we shouldn't double escape it...

Don't we all love these namespace discussions?

I'm with @sun on this one, I think using single characters is more readable and more greppable. That's what I've been doing with the PSR-0 code I've been playing with and grepping yields all explicit references to my namespace without having to account for 2 cases.

@crell are there any use cases where we would *need* to place class names in double quotes? It seems like we could make avoiding that part of the coding standard and require dynamically building the class with concatenation for the few cases where that's necessary.

I'm doing this sort of thing a fair bit in the database PSR-0-ification issue: $class = "Drupal\\Database\\Driver\\{$driver}\\Insert"; return new $class(); I find that much more readable and grokkable than the concatenated equivalent. Plus, if you're dynamically building the string then a simple grep for a literal fully qualified class name won't find it either way.

So we are basicly discussing preferences between people. So we should look at other frameworks and see how they handle it. I have troubles finding examples.

There always be someone disagreeing in this case. I'll try to search for examples in the other frameworks.

Doctrine:
https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Tools...

private static $_exporterDrivers = array(
  'xml' => 'Doctrine\ORM\Tools\Export\Driver\XmlExporter',
  'yaml' => 'Doctrine\ORM\Tools\Export\Driver\YamlExporter',
  'yml' => 'Doctrine\ORM\Tools\Export\Driver\YamlExporter',
  'php' => 'Doctrine\ORM\Tools\Export\Driver\PhpExporter',
  'annotation' => 'Doctrine\ORM\Tools\Export\Driver\AnnotationExporter'
);

Symfony:
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/Securi...

$this->addClassesToCompile(array(
  'Symfony\\Component\\Security\\Http\\Firewall',
  'Symfony\\Component\\Security\\Http\\FirewallMapInterface',
  'Symfony\\Component\\Security\\Core\\SecurityContext',
  ...

I found only this while quicksearching but as Sun noted Symfony isn't consistent so I asked a symfony dev

<aspilicious> well if you put namespace paths in quotes do you use single or double escaped backslashes
<aspilicious> 'this//is//a//path' or 'this/is/a/path'
<aspilicious> you can find both
<aspilicious> in symfony
* indy-afk is nu bekend als indytechcook
<aspilicious> slash  != backslash srry for that
<lsmith> aspilicious: yeah

<lsmith> and we use single backslashes when possible

<aspilicious> lsmith,  so than this one: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
<aspilicious> is "not ok"
<lsmith> aspilicious: i dont think we are consistent on that

Conclusion: they are not consistent but they prefer a single backslash

Zend
Didn't found an example yet but I trust Sun on this. So I believe we should adopt the following rule:
"Use a single backslash in quotes when it's possible."

I don't think there will be a lot of cases in core where it isn't possble...

Ok, well that seems pretty clear. Single slashes it is.

Ok back to active because this is a meta. But it is said :)

I have updated the coding standards page: http://drupal.org/node/1353118

Oops. In reading through #1323120: Convert cache system to PSR-0 I notice we came to consensus about one, but not both, of sun's points.

+++ b/core/includes/Drupal/Cache/DatabaseBackend.php
@@ -0,0 +1,250 @@
+class DatabaseBackend implements CacheBackendInterface {
...
+  /**
+   * Implements Drupal\Cache\CacheBackendInterface::getMultiple().
+   */
+  function getMultiple(&$cids) {

Do we really need to document the full namespace name in the phpDoc? I would have assumed that this:

+  /**
+   * Implements CacheBackendInterface::getMultiple().
+   */
+  function getMultiple(&$cids) {

would be sufficient?

We're already in the namespace after all...?

I agree with this, unless I'm missing a cluestick?

we're implementing the interface, so do we even need the extra doxygen duplicating what PHP itself enforces and knows about?

Yes agree, this seems unnecessary. For hooks it was necessary because the language didn't knew about, but for interfaces implementation I think the doxygen PHPdoc is necessary only if we need to document internal implementation details.

doxygen isn't namespaced so looking at a code snippet out of context it might not be possible to know which class we're referring to

Yes and I saw a lot of external code doing this. I don't know if this is good or bad to write the qualified names here, but at least, it's explicit, and that's why in doubt I chosed the explicit way. EDIT: And also because my IDE (Eclipse) does a better automatic completion when type hinting with qualified names (there are some edge case where it can't do proper completion without it).

For me I think I'd either leave the full reference (as a shortcut) or remove it all together (as redundant).

Agree.

And also because my IDE (Eclipse) does a better automatic completion when type hinting with qualified names (there are some edge case where it can't do proper completion without it).

I think issues relating to IDE handling and api.module's capabilities are most important here, but that doesn't belong in this issue. :-) (Edit: Duh, forgot what thread I was even in.)

I, for one, would prefer the fully qualified name. I know that sometimes when extending views handlers it can be easy to loose track of exactly where in the inheritance chain this method you're overriding/implementing was originally defined and you have to slog back through several parents to find it. I think qualified names in comment blocks make complicated in heritence cases much easier to track down.

Updated issue summary.

Added link to the DrupalQueue system issue.

Link to the Archiver issue.

Added stream_wrappers.inc issue.

Updated: added new lock backend to PSR-0 issue link.

Added lock backend minimal port issue to issue list

Courtesy of core office hours, this issue now has as change notice: http://drupal.org/node/1479568

So when the last of the related patches get committed, this can be closed.

Added #1503184: Convert Graph.inc to PSR-0 (and make its documentation clearer)

Moved to the lock issue to it's proper place.

Updated issue summary.

/me votes for new issue. We're at 50 comments here already and further discussions will be relevant only to module provided classes.

The last relevant subtask has been committed (#1323124: Convert file transfer system to PSR-0), so I'm tentatively closing this task because our work on /core/includes is over. See you in #1513210: Meta: Start converting module provided classes to PSR-0!