Component Upgrade for PHP 7.1

Prompted by a discussion in the Slack channel, we need some guidelines and ground rules for the conversion of components to type-hinted PHP 7.1 behaviour. The main point to make is that adding type hints is not going to be a simple case of just, well, adding the type hints. Refactoring is going to have to be done where methods are taking multiple types and these refactorings may be tricky. We need to make a decision on how and when to deprecate methods that take multiple typed arguments in order to clean up the API.

This is a unique opportunity to tidy up the API massively but we don’t want to break everything for everyone. For me we should strive to as close to 100% type hinted as possible and refactor out places where we can’t achieve that because of backwards compatibility, marking the mixed type method as deprecated.

The example used in the Slack chat was Zend\Forms’s add() method (you can find it here). Presently it takes an array/traversable or element interface. This should be split into 3 methods addFromArray, addFromTraversable and addElement (naming up for debate this is an example only) each having strictly typed parameters. The original add method would then have a mixed type and be marked deprecated and infer which call to proxy to depending on the type.

We should also remove @param docblocks where strict typing can happen in my opinion, but this is up for debate because of the usefulness of the comment about what the param does.

Anyway, this has rambled on enough, these are just my opinions and I’m dumping them here to open the discussion and hopefully get a set of guidelines in place so people can go along and convert components.

I missed the conversation on Slack, but yes there are cases where mixed types are definitely used. Don’t forget we’ve got iterable type as well (array|Traversable) but even that wouldn’t work fully as add() takes ElementInterface… so for that specific example, two methods addFromIterable(iterable, array) and addElement(ElementInterface, array) would be reasonable (sorry if I’m splitting hairs here).

Either way, yes I agree about removing @param and @return docs once converted. There’s always going to be weird edge cases where adding a type declaration will break something for someone (who was, perhaps, using a method call incorrectly for example), so we’d probably class them as BC breaks.

I have a naive question: What’s the reason for removing the @param docblocks? Is it simply that they would be redundant?

Yes, that, but also: single source of truth. It’s easy for docblocks to be out-of-sync with the code. If we have actual parameter typehints and return types, these are what the engine will actually follow, and those are what should be documented. Removing the docblocks ensures that IDEs and phpdoc will always be in sync.

The drawback is that we cannot document specifics about the arguments quite as easily. Having something of the form:

/*
 * @param array $options Key/value pairs, which may include the keys: "foo", "bar", and/or "baz".
 */

Provides even more contextual information when in IDEs or phpdoc. Those can be documented in the method docblock separately, but having the @param tag maps them sematically and binds them for tooling.

Most cases within the framework, this type of information isn’t terribly necessary, and some would argue that if it is, it’s likely an indication of poor argument naming and/or bad design. Minimizing the necessity for them is overall a good thing.

Would you like remove just @param tags or also @return?
IMHO we could keep all of them and use phpcs to sync PHPDocs and type-hinting.

@webimpress I’m guessing the argument is that @return SomeClassInterface doesn’t provide anymore information than the return typehint.

I would say that the need to add a description of what is returned (beyond the type) is probably a good hint that the method needs to be renamed to something more explicit.

@param is something I think would wind up be used sparingly. Again, if you can’t tell what it’s supposed to be from the type and variable name, some refactoring might be in order before just trying to explain in the comment. Of course this is coming form the guy that comments every class constant, class variable, parameter, and return type ad nauseam :slight_smile:

I’m against removing @params and @returns because they still allow us to express more things than type-hinting. The description is an obvious thing that would be lost and I don’t think it is reasonable to assume there isn’t a single parameter/return value that would benefit from a well written description in the entire ZF code base.

But there is also the documentation of array content that cannot be done with type hinting, only via phpdoc syntax. IMHO this:

function setOptions(array $options)

is way less descriptive than something like:

/**
 * @param int[] $options Array of options as defined in class constants
 */
function setOptions(array $options)

Also syncing between phdoc and type hinting could be automated, even with compatibility with the array syntax. Or at the very least checked, if not fixed.

Promoting the absence of comments really is not a good thing to do IMHO.

From what I can tell, we can provide @param annotations for specific arguments, which allows us to document only what differs from the tyephints. I think we should definitely provide descriptions when we can; however, if you look through the code base, the majority of parameter docs are simply the type and name, with no description. If there is no description provided, there’s no reason to add an annotation.

I do like the idea @webimpress provided of using phpcs to ensure that the annotations are in sync with the code; I’m just wary of providing annotations if they duplicate what’s in the code, without providing additional detail.

@moderndeveloperllc Please see How should we provide return type declarations for fluent interfaces?
There they suggest to use @return tag in some cases. So I am a bit worried that we will finish with messy PHPDocs - sometimes with some tags, sometimes without. I agree with @PowerKiKi, and his very good example with int[] and array type hint. As I mentioned before, and also @PowerKiKi did, we could use automated tools (like phpcs) to sync PHPDocs with type hints/return types.