Problematic order of optional constructor parameters in AsContentElement/AsFrontendModule

[m-vo]

Affected version(s)

4.13

Description

Our AsContentElement attribute currently looks like this:

contao/core-bundle/src/DependencyInjection/Attribute/AsContentElement.php

Line 23 in 3691e1d

public function __construct(string $type = null, string $category, string $template = null, string $method = null, string $renderer = null, array ...$attributes)

Having the first non-optional parameter at the second place forces you to define at least two parameters (https://3v4l.org/Y7N4G#v8.1.2), but with the content element attribute, you typically only define the category, which then results in:

Contao\CoreBundle\DependencyInjection\Attribute\AsContentElement::__construct(): Argument #1 ($type) not passed

(Similar reasoning for AsFrontendModule.)

@aschempp What's the reason for this unusual order? Can we change this?

[m-vo]

I found a discussion about this very topic #3619 (comment). But I don't agree with the outcome. 🙂

[aschempp]

This is not a bug. As mentioned in the comment, this is intentional and makes sense to be consistent with annotations. Always use named parameters with (our) attributes.

[m-vo]

It does not work, though, even with named parameters unless you pass in at least two. So this is clearly a bug.

[aschempp]

What does not work?

[m-vo]

Did you read anything of what I wrote? You cannot construct the object with a single parameter, no matter if named or not.

Having the first non-optional parameter at the second place forces you to define at least two parameters (https://3v4l.org/Y7N4G#v8.1.2), but with the content element attribute, you typically only define the category, which then results in:

[m-vo]

Just so that we are on the same page:

=

[fritzmg]

I agree that this needs to be fixed. Only having to define the category and nothing else should be the minimum for the AsContentElement attribute, just as it is already the case with the ContentElement annotation.

[ausi]

Can this be fixed by making category optional like
public function __construct(string $type = null, string $category = null, …
and throw an InvalidArgumentException if $category is null?

Or use an actual default value like string $category = 'texts' instead?

[m-vo]

Can this be fixed by making category optional like
public function __construct(string $type = null, string $category = null, …
and throw an InvalidArgumentException if $category is null?

Yes, this would work. https://3v4l.org/TArPo#v8.1.2

But we could IMHO also just change the order - especially, if you should use named arguments anyways.

[ausi]

As this parameter order was not released as stable yet, I agree we should change the order.

[m-vo]

By the way, having optional parameters before mandatory ones is deprecated since PHP8.0: https://php.watch/versions/8.0/deprecate-required-param-after-optional

[m-vo]

see #4065