PHP 8 and attributes

[zluiten]

With PHP 8 and having attributes support, configuring beans can be even more simpler. I don't expect to have a complete picture of what's needed, since I have only experience with discolight where I did this proposal (sort of). Anyway here's my take:

There are some options to walk this path:

1. Add support to the 0.x major versioning line
2. Replace configuration by annotation with attributes which of course would require a major version release: 1.x

I can't think of a good reason to go for option 2. Adding it to 0.x allows for a gradual upgrade path allowing to deprecate features which can eventually be removed in 1.x.

Adding this feature can be done by:

1. add support to current Annotation classes, or
2. creating new Attribute classes.

While new Attribute classes allows you to start with a clean slate, current Annotation classes can quite easily be adapted in order to be used as Attribute classes as well.

As option 1 is in my opinion the most valuable approach I'll go a bit further on that:

Add support to current Annotation classes

Example of how that can be achieved in case of the Alias class:

/**
 * @Annotation
 * @Target({"ANNOTATION"})
 * @Attributes({
 *   @Attribute("name", type = "string"),
 *   @Attribute("type", type = "bool"),
 * })
 */
#[\Attribute(\Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
final class Alias
{
    /**
     * @var string
     */
    private $name;

    /**
     * @var bool
     */
    private $type;

    /**
     * Creates a new {@link \bitExpert\Disco\Annotations\Bean\Alias}.
     *
     * @param array $attributes
     * @throws AnnotationException
     */
    public function __construct(array $attributes = [], ?string $name = null, bool $type = false)
    {
        // When configured by annotations the $attributes['value'] is not empty, so
        // in that case override the $name and $type variables from the $attributes['value'] array.
        if (!empty($attributes['value'])) {
            $attributes = $attributes['value'];
            // Assign local variables the value from $attributes meanwhile making sure the keys exist
            // with at least the default value as defined in the constructor.
            ['name' => $name, 'type' => $type] = $attributes + ['name' => $name, 'type' => $type];
        }

        if (!is_bool($type)) {
            $type = AnnotationAttributeParser::parseBooleanValue($type);
        }

        if ($type && $name) {
            throw new AnnotationException('Type alias should not have a name!');
        }

        if (!$type && !$name) {
            throw new AnnotationException('Alias should either be a named alias or a type alias!');
        }

        $this->type = $type;
        $this->name = $name;
    }

    public function getName(): ?string
    {
        return $this->name;
    }

    public function isTypeAlias(): bool
    {
        return $this->type;
    }
}

class Config
{
    /**
     * Using named arguments
     */
    #[Bean]
    #[Alias(type: true)]
    #[Alias(name: 'my.service.id')]
    public function myService(): Service
    {}

    /**
     * Using ordered arguments
     */
    #[Bean]
    #[Alias([], null, true)]
    #[Alias([], 'my.service.id')]
    public function myOtherService(): OtherService
    {}
}

As you can see named arguments allow for a clean usage of existing Annotation classes.

What about nested attributes?
In PHP's current implementation of attributes nesting is not supported, so explicitly configuring needs to be done on the same level, or instances of nested attributes/annotations should be able to be configured with scalars and instantiated in the "parent" attribute instance.

class Config {
    #[Bean(aliases: [
        ['type' => true],
        ['name' => 'my.service.id'],
    ])]
    public function myService(): Service
    {}
}

What does this means for each type of annotation?

Alias
Allow configuration of

• name
• type

Bean
Allow configuration of

• scope
• singleton
• lazy
• aliases discouraged, use (multiple) Alias attributes instead
• parameters discouraged, use (multiple) Parameter attributes instead

BeanPostProcessor
Allow configuration of

• parameters discouraged, use (multiple) Parameter attributes instead

Configuration
Just an identifier attribute

Parameter
The ordering of parameters is important since the method is called with the parameters in the order they are configured. Nesting the parameters inside the Bean annotation provides a natural way of respecting the ordering of the arguments in the methods signature.
But requiring attributes to be listed in a specific order feels a bit like a smell to me.

class Config {
    #[Bean(parameters: [
        ['name' => 'config.key1', 'default' => 'default value'],
        ['name' => 'config.key2', 'default' => 'other default value'],
    ])]
    public function myService($key1, $key2): Service
    {}

    // Correct ordering of the parameter attributes required, NOT IDEAL
    #[Bean]
    #[Parameter(name: 'config.key1', default => 'default value')]
    #[Parameter(name: 'config.key2', default => 'other default value')]
    public function myOtherService($key1, $key2): OtherService
    {}
}

Possible solution: When using the Parameter attribute, require a config key with the name of the argument so it can be used for calling the method with named arguments. The nicest would be something like this:

$parameters = [
    'config' => [
        'key1' => 'value of key1',
        'key2' => 'value of key2',
    ],
];
class Config {
    #[Bean]
    #[Parameter(name: 'arg2', key: 'config.key2', default => 'other default value')]
    #[Parameter(name: 'arg1', key: 'config.key1', default => 'default value')]
    public function myService($arg1, $arg2): Service
    {
        // $arg1 = 'value of key1;
        // $arg2 = 'value of key2;
    }
}

Unfortunately name is already in use as the name of the parameter key, but i.e. argname can perhaps be a good alternative.

How to incorporate this with the ConfigurationGenerator?

The doctrine AnnotationReader can be swapped out for a FallbackAttributeReader. The FallbackAttributeReader would prioritize attributes over Annotations.

I would not recommend mixing annotations and attributes on the same level.

Possible guidelines:

• When the Bean attribute is found on a method ignore any annotations.
• When the Bean attribute is found on a method look for Alias and Parameter attributes.
• When the Bean attribute has nested aliases prioritize these over configured Alias attributes.
• When the Bean attribute has nested parameters prioritize these over configured Parameter attributes.
• When the BeanPostProcessor attribute is found on a method ignore any annotations.
• When the Configuration attribute is found on a method ignore any annotations.

[shochdoerfer]

Thanks @Netiul for raising the issue. Your detailed analysis is awesome!

I'd be totally OK if we go the BC break way and introduce the next version to only support PHP 8 including attributes only. I don't think the current version would run on PHP 8 since we rely on ProxyManager which comes with some very strict PHP version constraints. If the next version is 0.11 or 0.20 or maybe 0.5 to signal the larger change, I don't favor anything.

Since we can't simply upgrade doctrine/annotations and have attribute support out of the box, I would recommend dropping doctrine/annotations support completely and "just" focus on the attributes. I would want to support both ways at the same time as this introduces not needed complexity.

From my point of view, we would need to upgrade all the dependencies first to support PHP 8 and adapt to possible changes in code plus get some basic work done like run the builds on GitHub actions and ideally add psalm & phpstan checks in the mix. There might be changes in ProxyManager that we would need to adapt to. Once we have that working we could start removing doctrine/annotations and change the code to support PHP 8's attributes. I like your attributes syntax proposal, that looks fine for me.

[shochdoerfer]

Hey @heiglandreas any thoughts you want to put into this?

[shochdoerfer]

The ProxyManager upgrade is mentioned in #130 already.

[zluiten]

Thanks @Netiul for raising the issue. Your detailed analysis is awesome!

Thanks for being positive :)

I don't think the current version would run on PHP 8 since we rely on ProxyManager which comes with some very strict PHP version constraints.

👍 Yes, looks like we are blocked by ProxyManager until it supports PHP 8. Besides that, a version of ProxyManager supporting both 7.4 and 8.0 might not be very likely.

I would want to support both ways at the same time as this introduces not needed complexity.

I think you mean "I would not want to support..."? Anyway I agree I guess, supporting both annotations and attributes at the same time might not be worth it.

From my point of view, we would need to upgrade all the dependencies first to support PHP 8 and adapt to possible changes in code plus get some basic work done like run the builds on GitHub actions and ideally add psalm & phpstan checks in the mix.

Sounds good. I noticed dependencies being out of date. PHP 7.4 is the only 7.x release series not being EOL and it's not even in the test pipeline. I would suggest (besides your suggestions) to bump the minimum supported PHP version to 7.4 in the next minor release. I can make a start if that's okay.

There might be changes in ProxyManager that we would need to adapt to. Once we have that working we could start removing doctrine/annotations and change the code to support PHP 8's attributes.

👍

[shochdoerfer]

Correct, I meant there is no need to support both Annotations and Attributes at the same time.

For ProxyManager there's a PR open to achieve PHP 8 compatibility: Ocramius/ProxyManager#628

Upping the minimum requirements to 7.4 and having all dependencies in the latest possible version sounds like a good start to me. Once we have that we could try to run Disco with PHP 8 and see if ProxyManager breaks things or not. I could imagine that things could work when we don't use PHP 8 features like union types.

[shochdoerfer]

Just came across this package, maybe it can help us: https://github.com/mbunge/php-attributes

[shochdoerfer]

Marco released ProxyManager 2.11.0 which supports PHP 8: https://github.com/Ocramius/ProxyManager/releases/tag/2.11.0

[shochdoerfer]

Upgrade to PHP 7.4 done via #132. Next step: Upgrade to PHP 8.0

[shochdoerfer]

@Netiul master branch is PHP 8.0 only now. Do you want to tackle the move to PHP's Attributes now?

[zluiten]

@shochdoerfer I definitely would like to help with that move, but I'm caught up in work for a couple more weeks. So if anyone else wants to pick up the slack in the mean time, go ahead... 😄

[shochdoerfer]

@Netiul I guess you are still busy with other stuff?

[shochdoerfer]

Does not look like anyone wants to drive this forward anymore ;(

There's a new kid on the block that might be interesting for those of you looking for a PHP8 version of Disco that makes use of attributes: https://github.com/giuseppe998e/syringe