Type hint array parameters

I think we should only use type hinting is the argument assumes a certain type for the argument. l() in Drupal 6, for example, assumes that the $options argument is array. If you don't pass it an array, it will result in a PHP error:

function l($text, $path, $options = array()) {
  // Merge in defaults.
  $options += array(
      'attributes' => array(),
      'html' => FALSE,
    );

This is one example where type hinting really benefits because then it cleanly results in a PHP argument exception rather then an ugly PHP error.... Subscribing.

There seems to be different opinions on whether to write function foo(array $bar) or function foo(Array $bar). I suggest using the former, as this is the style used in the PHP manual page on type hinting. Also, the array() construct and the array type string use the lowercase variant. The most prominent exception I can find is print_r() that outputs Array( ... ).

Likewise I suggest using stdClass (this is written StdClass a few places in Drupal).

I also agree with #3. Here's an initial patch to standardize the capitalization of the current type hinting before I start searching for parameters that need type hinting.

Looking at the diff, I find Array $foo more readable than array $foo in differentiating the type hinting from actual parameters, so I'm not sure I agree with the proposed change.

I think the lower-case "array" makes more sense for the type hinting then the upper-case "Array". When creating a new instance of an array, Drupal coding standards ask for the lowercase:

  $myarray = array(1, 2, 3);

So why not bring that standard over to the type hinting as well? Either that, or also make the creation require the uppercase as well:

  $myarray = Array(1, 2, 3);

I think we should keep both the instantiation and type the same case. Either have both the type and the creation use lowercase, or uppercase, not both. Would also be good to get Crell's thoughts on this.

I agree with Rob. It should be the same case as the coding standard. We shouldn't have two conflicting variable capitalization standard.

Yes, since Crell wrote most of those type hinting instances, I'd liek his input. As above - I don't claim it's consistent. Instead the difference as compared to instantiation (or casting) makes it more readable because it stands out.

I prefer the lowercase "array" for type hints. It's consistent with our widespread use of "array()".

The readability improvement (if worthwhile) would be better implemented with a more radical change to parameter variable naming. I see no reason why the type hints should stand out in function definitions instead of the actual parameter variable names:

function my_func(array $One, $Two, array $Three) {
}

versus Peter's suggestion:

function my_func(Array $one, $two, Array $three) {
}

I'm not saying we should change the standard, just that I think there would be more effective approaches than Peter's.

I'm also in favor of lower case. I committed Dave's patch. Thanks!

Setting this back to active to search for all the type-hinting needed now that we have a standard.

First pass with array type-hinting. My computer is too slow to run all tests, let's see how the testing bot fares.

common.inc changes did not pick up...

Another round, actually can install now.

I love how multiple people said that my input would be useful here and I didn't find out about the issue until after it was committed...

I discussed the case question with webchick months and months ago. I originally used capitals in DBTNG because in PHP the only things that can be type hinted are arrays and classes/interfaces. Classes/interfaces are always capitalized (in pretty much every convention, including Drupal core's now), so having all type hints be capitalized except for arrays didn't make much sense at all. It also made them stand out as different from array(), because they're a type hint, not a declaration. webchick agreed with that logic.

With that said, I don't care enough either way to make a huge fuss out of it. Lowercasing array is not wrong, just confusing when writing type hints. :-)

On the whole I am very +1 for adding more type hints, as they make the code more self-documenting and easier to debug since major errors will give a more useful error message. The caveats I will offer are:

1) Type hint all arrays iff we expect only an array, as is the case in l(). So by default don't type hint them unless you're sure. (I do plan to add more type hints to the DB layer once we get rid of the BC layer.)

2) Type hint classes/objects only with interfaces, not with actual class names. I know DBTNG violates that in a few places, but not many. :-) Type hinting to a class means you can only "fudge it" if you subclass from the parent class, which is not always possible. If you type hint to an interface, then you can pass in any object regardless of its hierarchy that implements that interface. That's much more flexible. See the changes made to DatabaseStatement in order to make the SQLite driver possible for a good example.

@Crell In standard C++ and Java code, primitives are lowercase and classes are camelcase. In PHP, arrays are primitives, so we would not be breaking with longstanding convention by keeping "array" lowercase yet preserving the camelcase of class names in hints.

Hm, I crossposted with the bot.

subscribing.

Add tag.

Lovely patch. Was this manually crafted or automated in some way? (re-roll?) :)

I think if this is going to happen it will have to be piecemeal. And I'm fairly sure the last patch here isn't even close to applicable anymore. :-)

Here's another piece for the meal. entity_load() does strange strange things if it's $ids isn't an array.

See also #839986: Enforce array arguments (Say good bye to Fatal error: Cannot use object of type stdClass as array errors).

This needs to be documented here: http://drupal.org/coding-standards#functdecl

@jbrown I've posted an issue here: http://drupal.org/node/1158720

@Crell if no interface exists, does it make sense to type hint with the class name?

I'd lean no. Hinting with the class name means that you cannot, ever, pass a variable that is not that class or a child of that class. There may be exceptions to that, but I'd prefer to never strong-type on a class.

You can, however, still specify the class type in the DocBlock. I don't know how many IDEs would pick up on that for auto-complete, but at least a human reader can pick up on it.

Hinting with the class name means that you cannot, ever, pass a variable that is not that class or a child of that class

Isn't that the desired result? If we expect a certain interface to be passed, then we expect a class that implements that interface to be passed in. An example of type hinting with interfaces is around, which shows that it works with interfaces, but gives you an error if you're trying to pass in a variable that does not implement the interface (desired result).

I think we should refine this issue to hit just functions where we expect arrays, and then a new issue where we add type hinting for classes. We don't want to implement that bike shed interface.

Good idea Rob!

Agreed on narrowing this issue's focus.

Rob: The problem with strong-typing on classes is if you want to have a class that implements two different interfaces (so it satisfies two different strong types), you're fine. If you want to have a class that extends from two different classes (because no interface is provided), you're screwed. It limits the amount of flexibility you have more than you probably intend. Strong typing in PHP should really only be used to say "I am going to use this object this way, so it better be ready for it". That's interfaces. Classes are for implementation details, which if you care about, you're doing it wrong. :-)

... for serious ;-) .

I think permitting flexibility without defining an interface is a bad idea.

We might want to split this into a different issue for each include file, but we'll see.

If we are adding type hinting to the function definitions, can we at the same time add the same type hinting to @param directives? Would you like me to roll an updated patch for that part?

@Rob I also noticed that you only added array type hinting to function variables with a default of array(). Shouldn't we also add array type hinting to the various form functions that use $form and $form_state?

Was concerned about the scope. Didn't want to do too much in one patch. Think the form changes should be a different issue, or hit those in here too?

If you wanna re-roll, be my guest! :-)

This is an untested patch that takes @rob's work from #41 and adds what I believe are other array declarations and @param type hinting that occurs in includes A through bootstrap.inc. I am curious what the bot and others think before I continue to common.inc and the rest of the includes directory.

It looks like patches in both #41 and #45 are having some of sort of test bot issue. Any ideas??

@Rob Loach and others - I created a patch in #1800330-2: Add missing type hinting to System module docblocks that adds type hinting to the *api.php files in the System module. There are considerable array definitions there like there are here. I would appreciate some reviews there to get that committed. Thanks.

Is there a plan to revisit this before release or should this get bumped to D9?

This needs to be done... Core committers did not want to touch issues like this that touch many files while there are a lot of potentially conflicting patches being worked on.

Is there any way to tag it to see that it is dealt with properly before launch? Should the priority be bumped? Or am I worrying about something that's already taken care of?

This should be done piecemeal. First, by ensuring that any NEW patches that touch code that should be type hinted get type hinted, and second by doing probably file-at-a-time or module-at-a-time patches. That's the only way this is going to happen.

The WSCCI initiative is working on the big task of converting all page callbacks to controllers at present, with some refactoring along the way. That's an excellent set of patches to review specifically for this issue.

Should this mark as fixed? Or the IS should be updated at least for me. Rolling patches per module is not applicable according to the scope police. https://www.drupal.org/core/scope#files

I agree with jungle that can be closed. Work on coding standards continues in #2571965: [meta] Fix PHP coding standards in core, stage 1.

An early version of this patch was committed on 2008, #13 and then reopened when a standard was agreed to. Now, coding standard changes are done by each sniff and there is an issue for type hints, #3107000: Fix Drupal.Commenting.FunctionComment.TypeHintMissing.

This patch contains coding standard fixes that apply to more than one sniff such as #2903911: Fix 'Drupal.Commenting.FunctionComment.ParamMissingDefinition' coding standard. There are also code changes to function user_role_permissions($roles = array()) { which look to be implemented in the meantime or otherwise handled because permissions are now config.

So, I think this can be closed as a duplicate and work continue in #3107000: Fix Drupal.Commenting.FunctionComment.TypeHintMissing.