Add RFC for Type Declarations

[jaswrks]

RFC: Type Declarations Proposal — an addition to WordPress Coding Standards.

cc @netweb @atimmer

[dingo-d]

... In a weakly typed language, you can do radical things like adding a string to an integer with no error.

Is this really radical? :D Maybe this should be reworded. Type juggling is a known thing in weakly typed languages

[jaswrks]

Good point. I agree that this word is a little too strong.

[jrfnl]

While I approve of the premise of this RFC, the RFC appears to be based on a incorrect understanding of type declarations.

Type declarations do not force the caller function to pass a certain type.
Their effect is that the function, on receiving a parameter of an unexpected type, will try and juggle whatever is passed to the expected type.
If successful, this is done silently.
Exceptions/errors will only be thrown if the type cast is unsuccessful.

This, in practice, means that array type declarations for parameters which only expect an array, can always be added safely as I've not yet come across any type juggle to array which would be unsuccessful.

Classname based type declarations can not always be added safely, though stdClass as a type declaration will work when appropriate. For other classes, type casting to the object can lead to unexpected results.

Other than that, the RFC needs updating:

• callable was introduced in PHP 5.4
• PHP 7.2 introduced the object type
• PHP 7.1 introduced nullable types
• PHP 7.1 introduced void (return type only)
• PHP 7.0 also introduced parent (return type only)

Regarding the formatting in the proposal, I think the current specs are incomplete.
I think a couple more things should be made explicit:

• expected case of the type declarations - I'd suggest lowercase except for class/interface names which should follow the name as declared.
• expected form of the type declarations - int, not integer, bool, not boolean etc.

I'd also like to see a mention of and possibly comparison with PSR 12 in so far as the proposed formatting would be different.

[ntwb]

Thanks for getting this project off the ground with the first proposal @jaswrks 💯

The specifics of the proposal are outside my skill set, I'll just add a few other references and pings:

Previous discussions: https://wordpress.slack.com/archives/C02RQBWTW/p1517605104000567

Ping @aaronjorbin @azaozz

[GaryJones]

Type declarations do not force the caller function to pass a certain type.
Their effect is that the function, on receiving a parameter of an unexpected type, will try and juggle whatever is passed to the expected type.
If successful, this is done silently.
Exceptions/errors will only be thrown if the type cast is unsuccessful.

If the declare(strict_types=1); is added, then it won't even try to type cast it - a wrong type will be a fatal error.

[jaswrks]

Type declarations do not force the caller function to pass a certain type.
Their effect is that the function, on receiving a parameter of an unexpected type, will try and juggle
whatever is passed to the expected type.

This is only true for Scalar Type Declarations in PHP 7+.
http://php.net/manual/en/migration70.new-features.php

i.e., Strict Mode changes the way Scalar Type Declarations behave.

Scalar type declarations come in two flavours: coercive (default) and strict. The following types for parameters can now be enforced (either coercively or strictly): strings (string), integers (int), floating-point numbers (float), and booleans (bool). They augment the other types introduced in PHP 5: class names, interfaces, array and callable.

So it's not the case for non-scalar types; e.g., arrays, objects, callable. These will always trigger a recoverable fatal error or throw an exception if the wrong type is passed.

[jaswrks]

@jrfnl Thanks for the great suggestions and corrections.
I can work on the items you listed in #3 (review)

[jrfnl]

If the declare(strict_types=1); is added, then it won't even try to type cast it - a wrong type will be a fatal error.

I didn't incorporate strict types in my statement as PHP 7 minimum is still ways away for WP, but all the same, that's still not true.

declare(strict_types=1) is file-based, which means it works as follows:

• It enforces return types for functions declared in a strict type file.
• It enforces parameter type declarations only when both the function call as well as the function declaration are in a strict typed file.

Otherwise, what I previously said still applies. It will juggle.

i.e., Strict Mode changes the way Scalar Type Declarations behave.

Correct 💯

So it's not the case for non-scalar types; e.g., arrays, objects, callable. These will always trigger a recoverable fatal error or throw an exception if the wrong type is passed.

Correct 💯 (to my surprise, I may add, as I'd expected that array casts would be made).

[jaswrks]

I believe I covered all of the great points raised by @dingo-d and @jrfnl. Revision 1 is now ready for review. Please see: Type Declarations Proposal (revision 1)

• Removed the word radical (props @dingo-d)
• Added to and corrected PHP version-specific details (props @jrfnl)
• Added a more detailed Formatting section and PSR-12 mention (props @jrfnl)
• Added a detailed explanation of Default Behavior vs. Strict Mode (props @jrfnl, @dingo-d)

[JDGrimes]

when both the function call and the function declaration are both in a strict-typed file

Only one "both" is needed here.

[jaswrks]

Great catch. Thank you!

[ntwb]

Thanks everyone for taking the time to review and comment 👍

I forgot to link to the original discussion that prompted this RFC

I believe we're now at the steps in the process:

"RFCs that are candidates for inclusion in the WordPress Coding Standards will enter a "final comment period" lasting 7 calendar days. The beginning of this period will be signaled with a comment and tag on the RFCs pull request."

Labelling, aka tagging and commenting as such 🎉

[jrfnl]

I've gone through it again and left a couple more remarks.

Additionally:

• The RFC contains a couple of references to "Void Functions". I believe it would be clearer if those references were changed to read "void return declarations" (or something along those lines) as that's what it really is all about.

• Question regarding the return types formatting: should a new line before the return type be allowed (tolerated) ? and if so, should the return type then be indented or not ?

  function some_function( $really, $long, $param, $list )
      : array {
  }

• Nitpick regarding the code samples used in the RFC: these appear to use space indentation while the WP coding standard prescribes tabs.
  As code in an RFC is often used as example/copy&paste code, I think it would be prudent to make sure all code samples comply with WPCS.

---

Regarding the next phase (implementation) - for the "lowercase type declarations" part, there is an open PR upstream, introducing a new sniff which checks just that: squizlabs/PHP_CodeSniffer#1685

[jrfnl]

I've ran some additional tests as parent is largely undocumented. Turns out to have been available and properly working since PHP 5.2 (5.0).

Note: while officially self and parent were available since PHP 5.0, they are only recognized properly since PHP 5.2. Before that, they would be seen as class names.

[jaswrks]

Nice! Thanks for researching this. I confirmed here: https://3v4l.org/TN3O6 and added parent as a valid type declaration.

[jrfnl]

I'm missing void in the list.

[jaswrks]

Fixed. Thanks!

[jrfnl]

➕ parent

[jrfnl]

Retrofitting public/protected functions/methods should be avoided 💯

However, what about retrofitting private methods ? These are not publicly accessible and should therefore be relatively safe to retrofit.

The only problem there would be plugins which extend a Core class and change the visibility of a private method to protected or public.
In that case, they would receive a Declaration of Class::method(...) should be compatible with Parent::method(...) warning. This will not cause WSODs though, so I think encouraging this would not be a bad thing.
See https://3v4l.org/VHoO6

[jaswrks]

should be avoided 💯

I've updated to this stronger wording you suggested.

---

However, what about retrofitting private methods ? These are not publicly accessible and should therefore be relatively safe to retrofit.

I agree. Not quite sure how we'd like to word this yet, but giving it some thought. Maybe others have some input they'd like to share on this? It seems reasonably safe to me.

[jrfnl]

@jaswrks I would remove the :100: from the RFC text though.....

[jaswrks]

@jrfnl writes...

Question regarding the return types formatting: should a new line before the return type be allowed (tolerated) ? and if so, should the return type then be indented or not ?

function some_function( $really, $long, $param, $list )
    : array {
}

Hi! 😄 No, not with the way it's currently written.

The colon and Type Declaration MUST be on the same line as a function's closing parentheses.

This matches PSR-12 (here).

When you have a return type declaration present there MUST be one space after the colon followed by the type declaration. The colon and declaration MUST be on the same line as the argument list closing parentheses with no spaces between the two characters.

So this is acceptable:

function some_function( $really, $long, $param, $list ): array {
	return array();
}

and so is this:

function some_function(
	$really,
	$long,
	$param,
	$list
): array {
	return array();
}

[schlessera]

As with most of the WordPress code, best practices can only be done half-way through because of compatibility reasons, and this causes them to not quite be best practices anymore.

I see a major issue in the above, as I suspect people will mostly just start putting array typehints everywhere (as they are often not using objects at all). This causes issues, as the array typehint does not behave as one would expect.

If you want to receive a collection of elements that you need to loop over, it might seem sensible to typehint against array. However, this will break on all other types that can be looped over: ArrayObject, Iterable, Traversable, ...

So, basically, what seems like a good idea to improve the quality in the WordPress code will probably just cause it to get tightly stuck in procedural world, as it completely kills all opportunities to have objects replace procedural constructs.

Note: I wholeheartedly support the effort to get typehints into the WordPress code. However, this should only ever be done with interfaces, and maybe abstract classes. Scalar typehints cannot be used (PHP 7+), class typehints eliminate all future flexibility and array typehint is just evil.

[jaswrks]

@jrfnl writes...

code samples [...] the WP coding standard prescribes tabs.

Fixed. Thank for catching that! 😄

[jrfnl]

So, basically, what seems like a good idea to improve the quality in the WordPress code will probably just cause it to get tightly stuck in procedural world, as it completely kills all opportunities to have objects replace procedural constructs.

@schlessera In those situations, wouldn't it be prudent to deprecate the original public function and add a new function interfacing with the new OO -oriented code instead ?

[schlessera]

@jrfnl Unfortunately, that's not how WordPress works... ;)
Whatever you throw in there will eventually cross the Ganges and stay eternally un-dead in BC-land.

[ntwb]

Just one note, how and where these RFCs end up is still a work in progress, adding this RFC as it currently stands to https://make.wordpress.org/core/handbook/best-practices/coding-standards/php would be somewhat out of place 😏

So please keep that in mind and not worry too much on the formatting of the markdown in this PR as if and when the time comes to implement and document this much of the formatting will require reworking to match existing documentation 😄

[schlessera]

I'd add an additional paragraph here along the lines of:

Moreover, as the wrong value does not throw an immediate error, it might get passed down to
another function, and yet another, and so on. Most often, you'll end up with an error that is
thrown in a completely unrelated part of the code, or in the worst case, no error being
thrown at all, while the frontend happily renders bogus data.

Basically emphasizing why throwing an immediate error is so important.

[schlessera]

The "no space between closing parentheses and colon" matches PSR-12. However, having a space in there might be closer to the "use extensive spacing" approach of WordPress.

[jrfnl]

WP coding standards is ambivalent about colons:

• for alternative control structures, a space is required before, (space or) new line after for () :
• ternaries require spacing on both sides of the colon
• for case $a: and default, the colon should not have a space before it

All together, the "no space between the parentheses and colon" for return types, seems to balance things out ;-)
(quite apart from making the divide between WP and the wider PHP a little smaller)

[schlessera]

This paragraph seems only partially related to type declarations. Although I agree that WP_Post should not be used, I think the most important reason is because it is a final class without an interface. Using that type declaration seals off any future extensibility. Using some other form of type checking is what throughout all of the WordPress code base anyway.

[schlessera]

Given the current PHP requirements, I think that type declarations should only be used right now with types that are:

• interfaces or classes
• considered to be part of the public interface
• extensible

All other type declarations should be discouraged, as they cause more problems than they solve:

• Using array immediately renders almost any future improvements using smart objects impossible. It makes it impossible for objects to stand in as collections of elements. Often times, when the doc-block states that a function needs an array, it actually means it needs a Traversable, because it wants to loop over it, but cannot do that as PHP 5.2 is missing that concept, or it needs an ArrayAccess because it wants to use indexed access. Using array should be avoided at all costs, unless there is a specific reason why it needs to be the actual internal type of array.
• Using classes that are not extensible means that future changes need to be done by changing an element of the public interface, instead of _ extending_ it. This is again an unneeded hindrance to flexibility. This is why segregated interfaces (i.e. Renderable or Taggable instead of WP_Post) should always be preferred, however WordPress is not really big on using interfaces.

[schlessera]

Due to the above, I'd also recommend changing the examples in this document to not so prominently show the array type declaration.

[azaozz]

Looking at all changes this reads more like a long-ish coding tutorial on how to use PHP type declarations. I'm not sure why "coding standards" have to include tutorials, off the top of my head that seems wrong.

Good coding standards should be very concise, logical and up to the point, or they become unusable. A major consideration when changing the coding standards for an existing project should be how the change would affect the existing code base and if it is going to introduce inconsistencies.

In addition, for an open source project (that attracts developers from "all walks of life") the main, most important requirement for the coding standards is to promote readability and consistency of the code base.

I completely agree with @schlessera's sentiment here. Using type declarations in core carries some risk of introducing fatal errors in existing plugins. I'm on the verge of proposing type declarations are marked as "not recommended for use" for core. Another very important reason is that they introduce yet another coding style that will make the existing code base more inconsistent.

I agree there should be some "linting rules" about type declarations in case plugins want to use them, but think they shouldn't be considered "best practice" for core unless they are used in a completely self-contained, separate "module" that is also released for general use outside of WordPress.

[GaryJones]

Looking at all changes this reads more like a long-ish coding tutorial on how to use PHP type declarations. I'm not sure why "coding standards" have to include tutorials, off the top of my head that seems wrong.

Unless I've misunderstood the process, this RFC is about getting to the point where the powers that be can make an informed vote about including an implementation as part of the WPCS Handbook and therefore WPCS sniffs. If that means there's some backstory / tutorial of the PHP feature, that's fine with me.

Good coding standards should be very concise, logical and up to the point, or they become unusable.

Agreed. I'd expect what would be added to the handbook, if the vote passed, would be considerably shorter in length, maybe no more than a couple of snippets to show how they should be formatted, and a paragraph or two about usage.

RFCs for PHP features don't directly end up on the userland php.net:

• https://wiki.php.net/rfc/scalar_type_hints_v5
  vs
• https://secure.php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration

What should be clear from an RFC, is:

1. How this syntax feature should be formatted. (space before colon or not, how it looks on multilines, etc.)
2. (And while we don't yet have a separate WPCS Handbook from Core Handbook): How WP Core may or may not use it (which this RFC does, at least to some extent)

Frankly, I don't care about how Core may or may not use it - if Core decides to avoid using array type declarations, or type declarations altogether, that's up to them. But WPCS is for everyone in the WP ecosystem, who can make their own decisions about whether to use a particular feature or not. What core, plugin, and theme authors want to know, is how should such a feature, if they use it, should be formatted.

I agree there should be some "linting rules" about type declarations in case plugins want to use them,

Yes! That's what I see this (and any RFC) to be about.

but think they shouldn't be considered "best practice" for core

These are two different points. Whilst WP is stuck on supporting PHP 5.2, it will always have exceptions to items in a more-rounded WPCS.

I would not want or expect (if even possible) for type declarations to become mandatory - as such, if a bit of code doesn't use them (such as Core), then no new violations would be reported. The violations would be if they were used, but with incorrect whitespace, for instance, or integer instead of int.

The Handbook doesn't cover how traits and interface file naming should be done, because Core doesn't use them, but that doesn't mean the Handbook can't agree on a format and publish that. The same goes for type declarations. Core is not the only consumer of the WPCS.

Moving forward, I'd ask:

1. Do you agree with the Formatting section of this RFC?
2. What paragraph or two do you want to see in the Core Handbook regarding usage in WordPress Core, related to restrictions due to PHP version or extensibility considerations?

[azaozz]

Unless I've misunderstood the process, this RFC is about...

Perhaps I misunderstood the process, sorry if I did :) IMHO having a nice tutorial on a PHP feature (as it relates to WordPress) should probably "live" somewhere, not just "roll off the screen" in a PR.

Whilst WP is stuck on supporting PHP 5.2, it will always have exceptions to items in a more-rounded WPCS.

Yes, unfortunately that is the reality (as dictated by web hosting needs for backwards compatibility).

Do you agree with the Formatting section of this RFC?

I tend to agree with @schlessera that there should be a space "between closing parentheses and colon"

protected function foo(): array {
	// ...
}

IMHO it makes the readability better (several non-letter characters in a row are always harder to read). The rest looks good.

What paragraph or two do you want to see in the Core Handbook

This for sure:

Retrofitting existing functions with Type Declarations should be avoided (as it may cause runtime errors).

However not sure about this:

However, new functions (particularly protected and private methods of a class) are encouraged to use Type Declarations.

In addition, if Type Declarations are added to the core coding standards, that will give the perception of their "approval" for use. Thinking the language should be chosen very carefully to avoid misunderstandings.

[JDGrimes]

I tend to agree with @schlessera that there should be a space "between closing parentheses and colon"

This would be my preference as well.

[GaryJones]

I tend to agree with @schlessera that there should be a space "between closing parentheses and colon"

This would be my preference as well.

While foo(): array has "several non-letter characters", foo( $args ) : array looks too spaced out to me.

PHP.net examples, and the rest of the PHP community IS the prior art here - and the key thing of being able to more easily adopt one way or another is support in IDEs, which will typically (at least first), follow the formatting from the community at large. @jrfnl has already highlighted that WP itself is inconsistent with colons (not that every colon has to be treated the same in every context), so my preference would be to have no space.

Aside: AFAIK, we do have a bug in a WPCS that means a space is required before the colon if the parameter has a type declaration, but otherwise the default behaviour of WPCS is to currently not to report a violation unless explicitly enabled.

[ntwb]

Thanks for the feedback @azaozz, this is the first time we are taking this RFC process for a drive 🚗 , I think it would be safe to say a test-drive 🚗💥🤕 even, I'm hopeful that as we continue to improve processes it becomes a valuable resource for all parties who ❤️ coding standards 😄

[jrfnl]

Regarding the space "between closing parentheses and colon":
Turns out there is still some discussion about this for PSR-12. I'd personally would very much like that discussion to be finished before a definite decision is taken about it for WP.

https://groups.google.com/forum/#!msg/php-fig/vZOpga3xoLg/_Q-CbeWMAQAJ

[ntwb]

Closing this and archiving this repository.