RFC: Updated Default Code Style

Creating Neos & Flow Code
1

Hi there, fellow Neosians,

since the Coding Guidelines have been published, a lot has happened in the PHP world, most prominently a proper type system :partying_face:

As already mentioned in Slack, it might be time for a larger overhaul of our guidelines, and since I’m interested in this topic, I’d like to start the discussion with a basic proposal, based on PHP 8 and later on 8.1:

Base would be PSR-12 PSR-12: Extended Coding Style - PHP-FIG

Doc blocks

SHOULD only be present when adding information.
Comments should not describe what a method does or a variable is, that should be in the name. But they should add valuable information like why a method is implemented and how the result is achieved or what a variable is intended to be used for.
Type annotations should be used when and only when extending PHP’s type system to support static analysis.

Bad:

/**
 * @param array $value
 * @return string
 */
public function foo(array $value): string;

Good:

/**
 * This method foos an array to a string
 * to support general usage of the FooInterface in use cases A, B and C
 *
 * @param array<int,string> $value
 */
public function foo(array $value): string;

Testcase documentation

(PHP Coding Guidelines & Best Practices — Flow Framework 7.3.x documentation)

… should follow newer PHPUnit style:

public function testFooReturnsBarForQuux(): void
{
 ...
}

Additional to interface names etc:

Command Class Names

Commands are written in present imperative:

final class DoSomething {}

Event Class Names

Events are written in simple past:

final class SomethingHappened {}

Exceptions

Exceptions

  • are written in (negative) simple past (suffix “Exception” optional when deemed necessary)
  • should be created via named constructor to better describe the cause and centralize default messages and codes
final class SomethingFailed extends \DomainException {
    public static function becauseADomainInvariantWasNotSatisfied(string $attemptedValue): self {
        return new self('Message using ' . $attemptedValue, 1234567890)
    }
}

[to be continued…]

What do you think?

2 Likes
2

Absolutely! lets have it. I wouldn’t aim to change everything in one go, but having this as new guidelines and adjusting things over time as we come across them.

3

I don’t really like this ‘passive progress’ idea. I think it was the same idea to integrate php types this way, but they haven’t found their way into all neos and flow parts yet. (some files just haven’t been touched for a while - and then there’s no need to change types randomly ofc).

As I wrote in Slack, maybe we could use an automated tool like Rector to modernise large parts of the codebase.

4

Generally: full agreement!

Just a few additions/questions:

What do you mean? test-prefix rather than @test annotation?
Personally I would prefer:

/**
  * @test
  */
public function foo_returns_bar_for_quux(): void
{
 ...
}

(well, or with the prefix. I don’t really care)

I’m not sure whether commands and events should play a role in our Flow CGL since they are no core concept.
But if we decide to add it: In the Neos.EventSourcing package we even suggest “Present Perfect Tense for Domain Events because that reads better in conjunction with when*() handlers”

Regarding

I’m with @Marc on this one: If we change these things only partly, we’ll end up with an inconsistent state.
Instead I would suggest to automate the things we can automate (e.g. rename exception classes) and create one global PR for each individual type of change

5

Ah, yes, test-prefix over the @test annotation. Or maybe as an attribute, would be fine as well, I just love cleaner / no docblocks :slight_smile: I’m not too fond of snake case though, as it appears nowhere in PSR-12

Good point. Present Perfect it is then :slight_smile: I’m not sure whether it belongs here or just to the EventSourcing context, since in the (hopefully near :slight_smile: ) future people might get in contact to commands and events more, with the ES package just as an implementation detail… But I won’t be sad if we leave them out for now either.

6

You’re right, doc block just for the @test tag adds visual clutter.

It’s kinda weird that camel case won the case wars since this_is_easier_to_read than thisEquallyLongPhrase.

Obviously we can’t change that for the main code base. But I started to use snake case in my own tests and I don’t want to look back.

In general I don’t care too much about code style within tests though :wink:

7

It has the benefit that a human actually looks at the code at some point if they want types (or anytihng else modern) in there, which very often leads to a “oh we might as well do this and that”.

1 Like
8

Regarding test… vs @test – I doubt the prefix is the “newer” convention. We used the prefix for ages and at some point switched to the annotation IIRC. Also https://phpunit.readthedocs.io/en/9.5/writing-tests-for-phpunit.html still mentions both, with no real preference for one over the other:

The tests are public methods that are named test*. Alternatively, you can use the @test annotation in a method’s docblock to mark it as a test method.

Regarding exceptions that should be created via named constructor - I am really against that. I can hardly count the times the Doctrine codebase confused the hell out of me with this… You see the message, search the code for it only to end up in the exception. And from there find a dozen places the exception could have actually happened. Even if you have a stacktrace, this is useless indirection IMHO.

  • To centralize “default messages” is something one should not need, because if the same exceptional error happens in multiple places, why is the code causing this not centralized itself?
  • To centralize the codes is the complete opposite of what the reason for having those codes is! You have the code and can find the exact one place the exception was thrown! That’s what it is supposed to do and always did! One libe of code throwing an exception, one unique code for it.
2 Likes
9

Thanks for the initiative! I suggest that we take some coordinated action regarding the code style update, which means putting it into Github issues and working on them systematically.

Questions that I have:

  • do you agree that the CGL chapter should still be our definitive source of information for our coding style?
  • is it realistic to provide an extra section about setting up IDEs like Phpstorm? Ideally with configuration which could be used?
  • who knows Rector well and would offer to implement our standards accordingly?
  • at what point would we apply Rector to our codebase to bring Flow 9 and Neos 9 up to speed?

And a suggestion: If we stumble over a new topic, for example if global namespaces should be imported or not, let’s create a Github issue, discuss it there and then make sure that the result is implemented in the CGL document and Rector rules.

3 Likes