Opening this as a critical followup from #1240138: Adopt PSR-0 namespace convention for core framework classes and #1178246: Add Symfony2 HttpFoundation library to core. Critical because we have a new coding standard and autoloader, and exactly zero Drupal code that complies to it yet ;)
Obvious candidates to start with are the database layer and cache system since they're already 99% OOP and also combine a mixture of interfaces and several classes - so if we do those it should be easy enough to copy that pattern for anything else.
Apart from converting the classes themselves, we need to register the Drupal namespace in bootstrap.inc so they get picked up.
It'd also be good to have a patch waiting somewhere that completely removes the registry file scan of includes, then when that passes we'll know we're done converting /includes as far as existing classes.
This also brings up the question of what to do with procedural leftovers - for example db_query()/db_select()/db_insert()/db_merge() or cache()/cache_clear_all().
I'd be very tempted to move these directly into bootstrap.inc (on the assumption that not much else will be in there by the time we're done), rather than having require once '/file/with/two/functions.inc'; in bootstrap.inc. But that's worth discussing here too.
Once this process is complete, #1320650: Stop scanning /includes with the registry should pass.
Related issues
The following issues need to be fixed before this issue can be closed:
#1321540: Convert DBTNG to namespaces; separate Drupal bits
#1323120: Convert cache system to PSR-0
#1323124: Convert file transfer system to PSR-0
#1468244: Convert DrupalQueue system to PSR-0
#1471368: Convert uuid.inc to PSR-0
#1469826: Convert DrupalCacheArray and SchemaCache to PSR-0
#1471376: Convert updater.inc to PSR-0
#1473600: Convert archiver.inc to PSR-0
#1475020: Convert stream_wrappers.inc to PSR-0
These are just related:
#1477446: Move lock backend to PSR-0 code
#1471364: Move mail system classes from system.mail.inc to PSR-0 classes in Drupal\Core
#1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch]
#1503184: Convert Graph.inc to PSR-0 (and make its documentation clearer)
Comment | File | Size | Author |
---|---|---|---|
#6 | 1320648-drupal-namespace.patch | 515 bytes | Crell |
Comments
Comment #1
Crell CreditAttribution: Crell commentedThe 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?
Comment #1.0
Crell CreditAttribution: Crell commentedUpdated to include link to extra followup.
Comment #2
pounardYou 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.
Comment #3
catchHmm, maybe factories.inc then? tbh I'm not too worried about bloating bootstrap.inc with them at this juncture, for database and bootstrap we'd be removing a require_once() and possibly even entire bootstrap phases so the main bloat would be comments in terms of loc.
@Crell - I'm fine with turning this into a meta issue if we open (at least major) tasks for each conversion. We also need to actually add the namespace to the autoloader - feels like we could just do that now whether any core classes are converted or not.
Comment #4
Crell CreditAttribution: Crell commentedfactories.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.
Comment #5
jherencia CreditAttribution: jherencia commented@Creel Git does not handle empty directories, just to confirm.
Comment #6
Crell CreditAttribution: Crell commentedWell, 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.
Comment #7
catchHere's the spin-offs:
#1323122: Convert database system to PSR-0
#1323120: Convert cache system to PSR-0
#1323124: Convert file transfer system to PSR-0
Comment #7.0
catchAdd link to the DB-namespacing issue.
Comment #8
Crell CreditAttribution: Crell commentedMoved 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?
Comment #9
catchWhoops, sorry for the duplicate.
Yeah let's get the namespace in, that's the initial patch I was hoping for. I'll just mark it RTBC now but give it a day or so to stew there.
Comment #10
catchOK, committed and pushed. Re-purposing as a meta-issue.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedAnyone have thoughts on how the Simpletest classes should be restructured? I'm referring to the tests themselves - the ones that ship with modules.
Comment #12
pounardCan be MyModulePrefix\Test\SomeTestCase
Comment #13
Crell CreditAttribution: Crell commentedMoshe: 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]
Comment #13.0
Crell CreditAttribution: Crell commentedAdd more spinoff issues.
Comment #14
sunComment #15
catchNope, definitely a task.
Comment #16
sunComment #17
pounardPlease also add the "PSR-0" tag on #1225404: Modern event based lock framework proposal.
Comment #18
webchickThis 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?
Comment #19
catchThe proper fix to bugs like #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap is to completely rip out the registry from core, anything we put in there can only be a nasty hack, so we need to refactor core so we can have an autoloader that doesn't result in fatal errors in quite common cases.
To do that, we need to replace it with another autoloader. We already put a PSR-0 autoloader in core (doesn't matter if it's Symfony's or not), so when all of the classes in /includes are using that, we can completely stop the registry from scanning /includes and only use it for module provided classes (or leave it scanning includes but not use that information). If we have any non PSR-0 classes in /includes we either can't do that, or would need to write another autoloader to handle the special cases.
There are currently only a handful of classes in /includes currently, so converting them all to PSR-0 is not that much, and that's what this issue tracks.
Then when #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch] is done we can stop using the registry for module-provided classes too, and git rm includes/registry.inc.
We may also want to convert some procedural code in /includes to PSR-0 (the cache system was made more OOP than it was in Drupal 7, there's a patch for lock.inc, other things are in the works). If that happens, they should be made PSR-0 compatible at the same time, but it's a different issue to this one - more the territory of #1239644: Define basic conventions for converting things into classes and [##1224666].
Comment #20
plachLarge 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).
Comment #21
webchickOk got it. So the scope is classes in core only, with the goal being to rip out our custom class registry.
Comment #22
sunI 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:
Do we really need to document the full namespace name in the phpDoc? I would have assumed that this:
would be sufficient?
We're already in the namespace after all...?
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.
Comment #23
plachTotally agreed.
Comment #24
pounard@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:
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.
Comment #25
sunNo, 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.
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.
Comment #26
pounardBackslash 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:
.
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.
Comment #27
yched CreditAttribution: yched commentedAgreed with sun.
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.
Comment #28
Crell CreditAttribution: Crell commentedThe 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.
Comment #29
sunAgain, "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.
:)
Comment #30
webchickOh, snap. ;)
Comment #31
catchI'm still not convinced we need to document this at all, since the $class implements $interface more or less covers it. However if we do, then having the full namespace seems useful.
Overall I agree with sun on this one, and especially if other projects also don't escape rather than the other way 'round. However while I don't really care, I'm not really keen on committing the other patch while it's actively disputed, so we should either sort it out quick, or explicitly rule this discussion out of scope for getting that patch in and be prepared to revisit it (or not) once this is done.
Comment #32
aspilicious CreditAttribution: aspilicious commentedCan 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?
Comment #33
tizzo CreditAttribution: tizzo commentedI'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.
Comment #34
Crell CreditAttribution: Crell commentedI'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.Comment #35
aspilicious CreditAttribution: aspilicious commentedSo 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.
Comment #36
aspilicious CreditAttribution: aspilicious commentedDoctrine:
https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Tools...
Symfony:
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/Securi...
I found only this while quicksearching but as Sun noted Symfony isn't consistent so I asked a symfony dev
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...
Comment #37
webchickOk, well that seems pretty clear. Single slashes it is.
Comment #38
aspilicious CreditAttribution: aspilicious commentedOk back to active because this is a meta. But it is said :)
Comment #39
Crell CreditAttribution: Crell commentedI have updated the coding standards page: http://drupal.org/node/1353118
Comment #40
webchickOops. 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.
I agree with this, unless I'm missing a cluestick?
Comment #41
catchThat seemed less controversial than the escaping so I glossed over it a bit for getting the other patch in, we should try to resolve though.
Thoughts on this:
- we're implementing the interface, so do we even need the extra doxygen duplicating what PHP itself enforces and knows about?
- 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 (for example it would not be impossible to have a cache DatabaseBackend as well as a field storage DatabaseBackend, in which case the comment is a bit of a shortcut compared to looking at the class definition + use statement.
- specifying the full namespace, especially for every method seems overly vebose, bug that brings us back tobthe first two points contradicting each other.
For me I think I'd either leave the full reference (as a shortcut) or remove it all together (as redundant).
Comment #42
pounardYes agree, this seems unnecessary. For hooks it was necessary because the language didn't knew about, but for interfaces implementation I think the
doxygenPHPdoc is necessary only if we need to document internal implementation details.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).
Agree.
Comment #43
Crell CreditAttribution: Crell commentedI 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.)Comment #44
tizzo CreditAttribution: tizzo commentedI, 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.
Comment #44.0
tizzo CreditAttribution: tizzo commentedUpdated issue summary.
Comment #44.1
amateescu CreditAttribution: amateescu commentedAdded link to the DrupalQueue system issue.
Comment #44.2
catchAdding DrupalCacheArray issue
Comment #44.3
catchUpdated issue summary.
Comment #44.4
catchUpdated issue summary.
Comment #44.5
catchAdding updater.inc
Comment #44.6
amateescu CreditAttribution: amateescu commentedLink to the Archiver issue.
Comment #44.7
amateescu CreditAttribution: amateescu commentedAdded stream_wrappers.inc issue.
Comment #45
pounardUpdated: added new lock backend to PSR-0 issue link.
Comment #45.0
pounardAdded lock backend minimal port issue to issue list
Comment #46
webchickCourtesy 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.
Comment #48
RobLoachAdded #1503184: Convert Graph.inc to PSR-0 (and make its documentation clearer)
Comment #48.0
RobLoachMoved to the lock issue to it's proper place.
Comment #48.1
RobLoachUpdated issue summary.
Comment #49
catchWe can now do the same thing for module-provided classes, should that be this issue too or a separate follow-up?
Comment #50
amateescu CreditAttribution: amateescu commented/me votes for new issue. We're at 50 comments here already and further discussions will be relevant only to module provided classes.
Comment #51
amateescu CreditAttribution: amateescu commentedThe 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!
Comment #52.0
(not verified) CreditAttribution: commentedUpdated issue summary.