Rollback type hinting for $node

Thanks for slogging through all those functions.

Committed to CVS HEAD. Thanks!

Committed to CVS HEAD. Thanks!

I am glad a 100+ kb patch saw adequate reviews and discussion.

chx - 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.

I 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)

yeah. type hinting with an interface, awesome. with a class? uh oh.

@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?

I 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

Renaming and bumping priority, as this is an API regression.

I'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.

Well, 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.

Quick, 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.)

Welp, 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.

Yes that's the consensus :)

chx 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. :-)

reroll for changes in head...

latest patch is half the size of the prior ones. probably not intended.

The 100K version had was likely rolled with -c 9 or something, look at it, there are 9 lines instead of 3 around the changes.

Here is another reroll. I have checked that it contains only function header changes:

chx@veyron:/var/www/drupal$ bzr diff > why_does_every_sort_of_crap_get_committed_to_drupal_7.patch
chx@veyron:/var/www/drupal$ grep '^- ' why_does_every_sort_of_crap_get_committed_to_drupal_7.patch |grep -v function
chx@veyron:/var/www/drupal$ grep '^+ ' why_does_every_sort_of_crap_get_committed_to_drupal_7.patch |grep -v function
chx@veyron:/var/www/drupal$

Looks 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.

Committed #28 to HEAD.

does not have any actual improvement of its own

That 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..

If 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.

Hrm, 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...