RFC: Updated Default Code Style

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

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.

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.

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 suggestPresent 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

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.

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: