While porting modules to D7, I've seen enough "Fatal error: Cannot use object of type stdClass as array in includes/somewhere on line 666" for the rest of my live. Imho, this is a *huge* DX issue.
Reason is simple..
Without patch (without display_errors = 1 even a WSOD):
Fatal error: Cannot use object of type stdClass as array in ...includes/theme.inc on line 787
With patch (displayed with the drupal maintenance theme, controlled by drupal settings, not php settings):
Recoverable fatal error: Argument 2 passed to theme() must be an array, object given, called in ...sites/all/modules/user_relationships/user_relationship_blocks/templates/user_relationships-block.tpl.php on line 27 and defined in theme() (line 729 of ...includes/theme.inc).
When the array type is enforced, you don't see a fatal error somewhere deep in the function you're calling, but you see where you call it, which function it is and it's like 10x easier to fix.
The patch converts everything I've found in /includes. /modules is not yet converted. Not that I have not found everything - I've started with theme() (which is the worst one when porting modules to D7) and converted all which had an "= array()" since these were easy to grep for and a few other which I've stumbled over.
Sometimes, arguments can be both an array and a string, so it's possible that I added too much array's, the test bot should be able to tell us that... I need some sleep now.
Comment | File | Size | Author |
---|---|---|---|
#10 | add_array_type5.patch | 41.36 KB | Berdir |
#8 | add_array_type4.patch | 41.85 KB | Berdir |
#6 | add_array_type3.patch | 41.38 KB | Berdir |
#4 | add_array_type2.patch | 40.5 KB | Berdir |
add_array_type.patch | 40.52 KB | Berdir | |
Comments
Comment #1
BerdirComment #2
BerdirComment #4
BerdirReverted the change to entity_load() and related functions for $ids since many functions in core pass NULL or FALSE to them. This "works" because $ids is checked with empty(). Not clean, but enforcing an array there can be considered as an api change.
New try.
Comment #6
BerdirWrong watchdog call.. we might want to allow NULL there too, I fixed the call as that's more correct for now...
Comment #8
BerdirAnother watchdog fix.
Comment #10
BerdirAlmost there ;)
Fixed another watchdog() call and removed the array type as it's not technically required for it to be an array (as opposed to other functions like theme()). So wrong watchdog() calls won't produce errors anymore.
I guess we can enforce D8 array arguments for entity loading, watchdog and other things...
Comment #11
BenK CreditAttribution: BenK commentedOkay, I'll use the patch in #10 in combination with our User Relationships D7 testing...
Comment #12
retester2010 CreditAttribution: retester2010 commented#10: add_array_type5.patch queued for re-testing.
Comment #14
klausiSubscribing, I like the idea of more type safety.
Comment #15
binford2k CreditAttribution: binford2k commentedYes, this is a HUGE issue when porting someone else's module!
Comment #16
RobLoachYes please!!!! Think we should split this patch into a per-module basis, plus core? A 41K patch is quite difficult to review ;-) .
Comment #17
johsw CreditAttribution: johsw commentedI wanna try to help help out here.
Should I make a new issue for each module, or should I just start making per-module patches and post them in this issue?
Comment #18
jbrown CreditAttribution: jbrown commentedSee also #318016: Type hint array parameters.
Comment #19
jbrown CreditAttribution: jbrown commented