Backwards compatibility break in PHP/Messages constructors

[ciaranmcnulty]

Relevant PHP concepts

PHP supports 'optional arguments' (arguments given an explicit default). This means a signature can change from:

foo(string $bar)

to

foo(string $bar, int $baz=0)

and the calling code foo('hello') still works as expected

Modern PHP additionally supports 'named arguments' so the same call can be written as foo(name: 'hello') but because this is a relatively recent addition, positional arguments are often used by developers.

The problem in Messages 19.0.0

For Messages, all arguments are optional by design and in the documentation we encourage using named arguments

The new KeywordType field has changed the constructor of Step from:

    public function __construct( 
        Location $location = new Location(),
        string $keyword = '',
        string $text = '',
        ?DocString $docString = null,
        ?DataTable $dataTable = null,
        string $id = '',
    ) {

to

    public function __construct( 
        Location $location = new Location(),
        string $keyword = '',
        ?KeywordType $keywordType = null,
        string $text = '',
        ?DocString $docString = null,
        ?DataTable $dataTable = null,
        string $id = '',
    ) {

This means that while named arguments still work:

$step = new Step(
    keyword: 'Given',
    text: 'I have an apple',
)

The equivalent with positional arguments is now a syntax error:

$step = new Step(
    new Location(),
    'Given',
    'I have an apple', # error because this is not a KeywordType
)

Solutions

We could

1. Explicitly state that positional arguments are not supported. This is easy for us but relies on the developer reading the documentation and will lead to some friction
2. Only ever add new fields at the end of the schema; this could be enforced by adding some tests that check the objects are all constructable

[ciaranmcnulty]

I'm interested to know whether this is an issue in other languages too

[ciaranmcnulty]

Discussed whether maybe we can mitigate this by having an official way of constructing the objects that isn't a constructor (and make the constructor private/internal?)

[mpkorstanje]

Java has the same problem but worse.

For Java adding any field will break the constructor. So generally speaking adding any new field is a breaking change for the API, though only adding required fields will break the json protocol.

A builder pattern to construct the messages would allow use to avoid this particular problem but to de-serialize messages from json the constructor would still have to be accessible. So we can't make it private either.

So while we can provide better ergonomics with builders (in java) and named arguments (in php) we can't avoid the problem entirely.

[mpkorstanje]

I think the better solution would be to decouple Gherkin from Messages. This would allow Cucumber implementations to align breaking changes in messages with breaking changes in their own implementation.

[ciaranmcnulty]

Decoupling Gherkin is probably a good idea, but the issue would still remain if we wanted to add a new field to something else