php-fig/per-coding-style

New PHP syntax worth considering

KorvinSzanto opened this issue · 17 comments

I've gone through the PHP versions since 7.3 which was the most recently released when PSR-12 was accepted and gathered a list of syntax that we may want to cover in this PER. This list is likely incomplete so please add a comment if there's more I should add.

PHP 7.3: https://www.php.net/manual/en/migration73.php

PHP 7.4: https://www.php.net/manual/en/migration74.php

  • #19 Class property types: public int $foo;
  • #17 Short functions: fn(int $foo) => $foo * 2
  • Numeric literal separator: $int = 1_000_000;
  • __serialize and __unserialize vs using Serializable
  • Nested ternaries require parenthesis in some situations post 7.4, we should consider using SHOULD NOT for nested ternaries.

PHP 8.0: https://www.php.net/manual/en/migration80.php

PHP 8.1: https://www.php.net/manual/en/migration81.php

  • Array unpacking with string keys: $array = [...$otherData, 'b' => 1];
  • Named arguments used after argument unpacking: $result = foo(...$args, namedArg: 123) https://3v4l.org/PN7kg
  • #7 Enums: https://3v4l.org/8Dljv
  • First class callables: foo(...) vs foo( ... )
  • #45 Intersection types: Foo&Bar vs Foo & Bar
  • new keyword in parameter initializers: https://3v4l.org/vcqT5
  • #19 Readonly properties

PHP 8.2: https://github.com/php/php-src/milestone/4

  • #41 Readonly classes

Extra potential things to cover:

  • Visibility keyword is not required when specifying readonly: public function __construct(readonly $foo)

__serialize and __unserialize vs using Serializable

Serializable is deprecated (see https://wiki.php.net/rfc/phase_out_serializable) and is meant as a compatibility layer for old implementations.

FILTER_VALIDATE_BOOL over FILTER_VALIDATE_BOOLEAN

FILTER_VALIDATE_BOOL was added as it is the canonical type name, therefore this one should be recommended IMHO

For composite types, no spaces IMHO, especially when mixing union and intersection types (soon inc.) having loads of spaces is going to be detrimental to readability IMHO.

Serializable is deprecated (see https://wiki.php.net/rfc/phase_out_serializable) and is meant as a compatibility layer for old implementations.

I wanted to include anything not removed just in case

FILTER_VALIDATE_BOOL was added as it is the canonical type name, therefore this one should be recommended IMHO

👍 We already require folks use bool over boolean so this makes sense.

For composite types, no spaces IMHO, especially when mixing union and intersection types (soon inc.) having loads of spaces is going to be detrimental to readability IMHO.

I'm not so sure on this one, PSR-12 currently prefers spaces around other binary operators and other languages that have similar syntax also tend to prefer the spaces. 1 2 3 I agree these declarations will get harder to read as they get longer especially since we don't have a great way to make larger union types portable like type FooBarBazType = Foo | Bar | Baz;. That said, I think we're going to have that issue whether we require spaces or not so we will probably need think about multiline declarations with something like:

function foo(): 
    null |
    SomeReallyLongClassName |
    SomeReallyLongOtherClassNameInterface |
    SomeReallyLongOtherOtherClassNameInterface
{
    return 1;
}

Footnotes

  1. https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html#unions-with-common-fields

  2. https://flow.org/en/docs/types/intersections/

  3. https://docs.python.org/3/library/typing.html#typing.Union

In regard to multiline declaration, IMHO we should suggest to put the operators at the start of the line, so it makes immediately clear that there's a continued declaration (or operation, in other contexts like with || and &&):

function foo(): 
    null
    | SomeReallyLongClassName
    | SomeReallyLongOtherClassNameInterface
    | SomeReallyLongOtherOtherClassNameInterface
{
    return 1;
}
Crell commented

From what I've seen, union types are generally written without spaces right now. At least that's what TYPO3 has been doing as we add types for v12.

IF going multiline, I agree the | should be at the start, as is typical for other similar constructs.

Looking at Typescript, union and intersection type declarations include spaces around operators (see https://github.com/microsoft/TypeScript/blob/main/src/lib/es5.d.ts#L174 for example).

Crell commented

PR for short closures: #17

Crell commented

PR for trailing commas: #18

Crell commented

PR for typed properties and readonly: #19

Crell commented

PR for attributes: #26

Crell commented

Things I think we can be silent on:

  • Numeric literal separator: Is there a guideline here to use?
  • __serialize vs Serializable: The latter is deprecated anyway, so I don't know that we have much to add beyond the language itself.
  • Throw as an expression: I really don't know what we'd even say here.
  • new in parameter defaults: Again, I'm not sure there's anything to standardize here that isn't already covered by other statements. (Eg, space on either side of the =.)
Crell commented

PR for FCC: #27

Numeric literal separator: Is there a guideline here to use?

I don't think there is a generally applicable guideline other than “yes, please make use of it”. The “correct” grouping depends on a context:

const ONE_EURO_IN_CENTS = 1_00;
const THOUSAND_EUROS_IN_CENTS = 1_000_00;
const HUNDRED_THOUSAND = 100_000;
const ONE_MILLION = 1_000_000;
Crell commented

Handles readonly classes: #41

Crell commented

I'm going to argue that CPP is already adequately covered by existing rules, so nothing new needs to be said.

Named arguments are here: #42

Crell commented

Nested ternaries: #43

Crell commented

OK, I have gone through and struck out any changes that I believe require no action, either because they're not worth it or existing rules are already sufficient. Korvin, if you disagree with any of those go ahead and un-strike them.

Everything else now has an active PR. Discussion on those PRs. 😄

Crell commented

I'll call this done at this point.