[PSR-12] Review phase review

237 views
Skip to first unread message

Larry Garfield

unread,
Dec 10, 2017, 4:00:48 PM12/10/17
to php...@googlegroups.com
*Puts on CC member hat*

4.2:
* Regarding trait whitespacing, it feels incomplete. Like, class bodies
should get the same "set of blocks with one line separating them" treatment as
the file header does. That would effectively expand to one line between
methods, and between the properties, too. I don't know if that's scope creep
but as is it feels lacking and incomplete.

* Related: Technically I see nothing that mandates properties-first, then
methods. That's virtually universal from what I've seen, though. Should that
be included in class organization?

4.5:
* I still hold that "function foo(): string" is silly and the colon should
have spaces on either side. This feels like an entirely unnecessary
inconsistency.

8:
* I know it's unresolvable without breaking the most controversial part of
PSR-2, but I am just going to call out the inconsistency in anonymous classes
between closures and explicit classes. That's messy.

Metadoc:

2:
* "PHP-FIG" is usually hyphenated, although Secretaries, please decide on
which the proper spelling is. :-)

* "we have taken a more prescriptive approach and defined the standards for new
language features as they are released" - eh, I know that was the intent, but
is that still true, given how long PSR-12 has been in progress?

* " it will have a better chance of being adopted but this is in the hope that
it will mean that projects." - That sentence is grammatically incomprehensible
and ends in the middle so I have no idea what it's saying.

3.1:
* PHP 7.1 and 7.2 should be mentioned, since there is mention of 7.1 syntax.
(I don't know that 7.2 has any new syntax we should care about.)

4.4:
* The description here of the vote, IIRC, is rather misleading. It wasn't a
"do you like this", but a "do you have a really really good reason to not do
it the way we've already written?" That's why the votes are so dramatically
biased in favor of the status quo: The survey specifically said to bias that
direction. The survey should be accurately described or omitted. As is, it
is misleading and false.

4.5:
* Typo, there's an extra ] in the text.

6:
* No need to bold names. I don't think any other specs do.

7:
* Formatting error on "Entrance Vote".

Generally:
* notice there's no mention of variadics. Was that deliberate? If the goal
is to cover "language features that didn't exist in 5.3 when PSR-2 was
written", variadics and the splat operator (which is still the coolest named
operator ever) seem like something that should be covered.

signature.asc

Adrien Crivelli

unread,
Dec 10, 2017, 10:42:58 PM12/10/17
to PHP Framework Interoperability Group
Since we are pointing out at inconsistencies, I would also like to mention how `declare` is treated as an exception for no apparent reason.

Why should we write:

declare(strict_types=1);

When we could write the more consistent variant:

declare(strict_types = 1);

This exception does not make sense to me. And I know it will be pointed out that `declare` is a construct, but I fail to see how it has anything to do with spaces around operators. "6. Operators" specifically state that all operators must be surrounded by exactly 1 space. Why do it differently for `declare` ?

Alexander Makarov

unread,
Dec 11, 2017, 3:30:21 AM12/11/17
to PHP Framework Interoperability Group
Thank you for the input. Meta should be definitely fixed. Some other comments:
 
* Related: Technically I see nothing that mandates properties-first, then 
methods.  That's virtually universal from what I've seen, though.  Should that 
be included in class organization? 

Generally it's true but there are exceptions. Private properties intended for caching object instances are kept close to  methods in many libraries I've seen and I can't say it's a bad practice. In fact, it makes it easier to read such code.

4.5: 
* I still hold that "function foo(): string" is silly and the colon should 
have spaces on either side.  This feels like an entirely unnecessary 
inconsistency. 

That would be very unnatural since in most natural languages there's no space before ":" and a space after ":".

* "we have taken a more prescriptive approach and defined the standards for new 
language features as they are released" - eh, I know that was the intent, but 
is that still true, given how long PSR-12 has been in progress? 
 
It is. Standards were mostly defined at that time. Then there was a period of validation with surveys.

Alexander Makarov

unread,
Dec 11, 2017, 3:38:30 AM12/11/17
to PHP Framework Interoperability Group


On Monday, December 11, 2017 at 12:00:48 AM UTC+3, Larry Garfield wrote:

Joe T.

unread,
Dec 26, 2017, 3:51:23 AM12/26/17
to PHP Framework Interoperability Group
i may be jumping into the end of a resolved discussion, but i'm new in the group and am looking for opportunities to offer my experiences to the PSR-12 review process.

Re: properties first
i fully support adding a recommendation to define all constants, properties, and methods declared with their own kind (preferably in the order listed). i can't recall ever finding a class that was more understandable by defining a property near its "relevant" functions midway down a class. If the point of a class-level member is to be accessible anywhere in the class, and for accessible members to be extensible/overridden by descendants, then organization is important. The use case specifies private properties, but i don't see the justification. If a descendant class needs to override the ancestor with its own private properties, there's an expectation set by the ancestor to define those properties in relatively close position to the original.

Shorter version: it would seem more appropriate that if a property "belongs" to only a limited scope of functions, they should be extracted to a separate structure (Trait or abstract class) and incorporated into the class that would use them.

i find it much cleaner to keep like-things together, and use PHPDoc comments with "@see" references if the member is designed for a narrow, limited purpose. My team's style guide requires not only member grouping, but grouping by modifier signature as well.


Re: 4.5 return type colon spaces...
Just to be a fly in the ointment, the argument that natural languages do one thing and code should follow suit could be used to support

public function foo ($bar, $baz)
// and
$this
->foo ($bar, $baz);

over the prescribed

public function foo($bar, $baz)
// and
$this
->foo($bar, $baz);

since most natural languages place a space before a parenthetical expression. Granted, the call format that includes a space looks silly, but it's legal and follows natural language. As such, i can see the contention that there's inconsistency to require removal the space before a ":" operator or to require collapsing white space inside declare(strict_types=1).

Spaces, in fact, have the appearance of several inconsistencies, particularly to potential new PHP coders:
  1. Name function declaration: no space between name and "("
  2. Anonymous function declaration: space between "function" and "(" - seems to contradict #1.
  3. All non-unary operators: space on either side of the operator.
  4. Return type operator: space after colon, not before - seems to contradict #3.
  5. Nullable argument / nullable return type: no space between "?" and type - seems to contradict #3.
  6. Cast expressions like (int) '123': no mention (follow #3, or is (int)'123' acceptable?)
  7. Declare strict types: no spaces at all, despite appearance as a "function" call (similar to list() or isset()) and an assignment operator. Seems to contradict #3.
  8. Spread/splat: no mention at all. Madness! ;-)
i realize a lot of these rules have been around for a long time, and come from surveys of programmers far more experienced than myself. But my understanding is part of the drive behind this updated PSR is to reexamine the existing guide and expand for new language features. Perhaps i'm mistaken. i have no strong opinion about where spaces are included or omitted, as long as they're done consistently, or at least with some justification for the rule. Simply saying "MUST" doesn't really help to explain why that's the recommended style. And as someone who has to convince a team of programmers to comply with style rules they will have to adapt to, justification goes a long way.

Okay, went a lot further with that than intended. Sorry.


Lastly, "splat/spread" was the best operator name until spaceship <=>, in my opinion. ;-) But splat is WAY more useful.

Thanks for the opportunity. Hope i haven't overstepped boundaries as a newcomer...
-jlt

Adrien Crivelli

unread,
Dec 26, 2017, 4:57:08 AM12/26/17
to PHP Framework Interoperability Group
I agree with pretty much everything you say. Predefined order for members is good. And eliminating inconsistencies in the spec is more important than adopting style of existing code base just for the sake of not editing that code.

FYI there is a PR dealing with `declare` statement over there: https://github.com/php-fig/fig-standards/pull/992
Reply all
Reply to author
Forward
0 new messages