Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
In #318016: Type hint array parameters it was suggested to use type hinting in function signatures, i.e. function foo(array $form, stdClass $node)
. Doing this across the whole code base is a huge task. This issue addresses a subset of that.
This patch adds type hinting for all (I hope) occurrences of $node. I also includes the patch from #592572: Always pass nodes as objects that is currently a requirement for this to work.
I haven't investigated whether the patch has a negative effect on performance.
Comment | File | Size | Author |
---|---|---|---|
#28 | why_does_every_sort_of_crap_get_committed_to_drupal_7.patch | 56.75 KB | chx |
#26 | why_does_every_sort_of_crap_get_committed_to_drupal_7.patch | 58.27 KB | chx |
#24 | type_hinting_rollback.patch | 56.54 KB | bcn |
#18 | node-type-hint-revert-3.patch | 104.88 KB | c960657 |
#12 | node-type-hint-revert-1.patch | 107.89 KB | c960657 |
Comments
Comment #1
c960657 CreditAttribution: c960657 commentedUpdated patch, because #592572: Always pass nodes as objects was just committed.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for slogging through all those functions.
Comment #3
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #5
c960657 CreditAttribution: c960657 commentedLooks like this wasn't committed to CVS.
Here is a fresh reroll.
Comment #6
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #7
chx CreditAttribution: chx commentedI am glad a 100+ kb patch saw adequate reviews and discussion.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedchx - your moody sarcasm is not at all helpful. state your objection if you don't like this patch. subscribe to the RSS feed for CNR or RTBC if you you want to review pending patches.
Comment #9
dmitrig01 CreditAttribution: dmitrig01 commentedI don't support this, because if someone wanted to override $node with a real class, now they can't because all the functions are expecting an stdClass. Arrays are fine but objects aren't for type hinting IMO (at least in most places)
Comment #10
chx CreditAttribution: chx commentedyeah. type hinting with an interface, awesome. with a class? uh oh.
Comment #11
chx CreditAttribution: chx commented@moshe and if i subscribe then i magically will have the time? why me? noone said a word about this patch and it got in just like that past freeze. It's not enhancing DX significantly, it's not enhancing pretty much anything, what's the point?
Comment #12
c960657 CreditAttribution: c960657 commentedI'm not sure how you would replace $node with a real class, but you could make that class extend stdClass.
I think type hints are particularly relevant for Drupal, because of the many arrays that are passed around. If you by accident pass the wrong argument, the error message may be very subtle, because the real error often occurred several levels above it in the stack trace. Adding type hints doesn't completely solve this, but it helps in some cases.
However, here is a patch for backing out the change.
Comment #13
Crell CreditAttribution: Crell commentedI don't know off hand if you can "extend" stdClass, or if all classes conform to stdClass. But I do know of one really crazy D5 project that added real classes for users, so there are certainly use cases.
OK, I just tested it and no, stdClass is not an implicit parent of all classes, and explicitly extending stdClass means that you cannot extend some other class. So this patch is a regression and should be rolled back.
As chx said, we should only type hint on interfaces or arrays, never classes. That's in the coding standards, too, although it's a recent addition: http://drupal.org/node/608152
Comment #15
Crell CreditAttribution: Crell commentedRenaming and bumping priority, as this is an API regression.
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedI'm fine with rollback if thats what people want though I have not heard of a single valid use case. "really crazy D5 project that added real classes for users, so there are certainly use cases". thats a non-sequitur for me.
Comment #17
Frando CreditAttribution: Frando commentedWell, it won't be possible without hacking core but in D7 with PDO and entity loading you'd only need to change very few LOCs to fetch entities into an actual class instead of an stdClass upon loading (I tried this, it works). So use cases are much more likely than in D5 or D6. At least it offers room for improvisation regarding using actual classes for entities, which this patch makes impossible without applying a 100k patch first (that does not have any actual improvement of its own). Therefore, I support the rollback.
Comment #18
c960657 CreditAttribution: c960657 commentedReroll.
Comment #19
Crell CreditAttribution: Crell commentedQuick, before it breaks again. :-) (I didn't look at every line, but it looks like all its doing is taking stdClass back out of function signatures, which is correct.)
Comment #20
Dries CreditAttribution: Dries commentedWelp, I was under the impression that stdClass was the parent class of all classes. Must be my Java background. I'm happy to rollback if that is the consensus.
Comment #21
chx CreditAttribution: chx commentedYes that's the consensus :)
Comment #22
Crell CreditAttribution: Crell commentedchx and I agree on something related to PHP 5 OO features. I don't think you can get a better sign that this should be committed. :-)
Comment #24
bcn CreditAttribution: bcn commentedreroll for changes in head...
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedlatest patch is half the size of the prior ones. probably not intended.
Comment #26
chx CreditAttribution: chx commentedComment #27
chx CreditAttribution: chx commentedThe 100K version had was likely rolled with -c 9 or something, look at it, there are 9 lines instead of 3 around the changes.
Comment #28
chx CreditAttribution: chx commentedHere is another reroll. I have checked that it contains only function header changes:
Comment #29
sunLooks good.
As chx mentioned, the difference comes solely from the increased diff context. And it's visible by reviewing it. Interesting.
Although this patch will most likely break many others of mine in the queue right now, I support it.
Comment #30
webchickCommitted #28 to HEAD.
Comment #31
PasqualleThat is not true. The improvement is that you can't pass an array for the $node function parameter. $node is an object.
In Drupal we (not me, someone else) had a tendency to allow all kind of variable types for the same function parameter. And not even allowing different types but giving different meanings for the same parameter:
http://api.drupal.org/api/function/node_load/6
http://api.drupal.org/api/function/hook_nodeapi/6
That is just insane, and should be fixed everywhere as it generates developer errors. It is simply a wrong code style which should be fixed and forgotten.
So is it possible to make the $node an object interface (in Drupal 8,9,10) and then use type hinting? I would not close this issue yet..
Comment #32
Crell CreditAttribution: Crell commentedIf we make a proper Interface out of nodes in Drupal 8, sure. We deal with that then. For now, $nodes are still anonymous objects and that's not changing for Drupal 7, so this issue remains fixed.
Comment #34
Dave ReidHrm, I don't get why this was rolled back without thinking the same logic could be applied to the *lot* of other places where stdClass is used as a typehint (see most of file.inc).
I hate PHP for not allowing a general 'object' type hint...