Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add PHP 8 support and configuration by attributes #3

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zluiten
Copy link

@zluiten zluiten commented Dec 16, 2020

Re #2

This adds support for the following:

  • PHP 8
  • Configure service definition by attribute (EventEngine\Discolight\ServiceDefinition).
  • Register multiples instances of the same type (but f.e. with a different factory method).
  • Register instances under more than 1 service id. This needs a second though because it behaves like aliases. It was inferred based on 1) attributes can be added more than once 2) PHP 8 (return) union types.
  • Register instances with a builtin return type (like callable)

Above list is available for PHP 8 only but doesn't break applications running PHP 7.

Example 1:

use EventEngine\Discolight\ServiceDefinition;
use Mezzio\Authentication\UserInterface;
use Mezzio\Authentication\DefaultUser;

final class ServiceFactory
{
    #[ServiceDefinition('Mezzio\Authentication\UserInterface')]
    public function userFactory(): callable
    {
        return $this->makeSingleton(UserInterface::class, function () {
            return function (string $identity, array $roles = [], array $details = []): UserInterface {
                Assert::allString($roles);
                Assert::isMap($details);

                return new DefaultUser($identity, $roles, $details);
            };
        });
    }
}

$serviceFactory = new ServiceFactory();
$container = new Discolight($serviceFactory);
$serviceFactory->setContainer($container);

$userFactory = $container->get(Mezzio\Authentication\UserInterface::class);

Example 2:

use EventEngine\Discolight\ServiceDefinition;
use Mezzio\Authentication\AuthenticationInterface;
use Mezzio\Authentication\UserInterface;

final class ServiceFactory
{
    #[ServiceDefinition('middleware.basic-auth')]
    public function basicAuthenticationMiddleware(): AuthenticationMiddleware
    {
        return $this->makeSingleton('middleware.basic-auth', function () {
            return new AuthenticationMiddleware($this->basicAuthenticationAdapter());
        });
    }

    #[ServiceDefinition('middleware.jwt-auth')]
    public function jwtAuthenticationMiddleware(): AuthenticationMiddleware
    {
        return $this->makeSingleton('middleware.jwt-auth', function () {
            return new AuthenticationMiddleware($this->jwtAuthenticationAdapter());
        });
    }
    
    protected function basicAuthenticationAdapter(): AuthenticationInterface
    {
        // return authentication adapter
    }
    
    protected function jwtAuthenticationAdapter(): AuthenticationInterface
    {
        // return authentication adapter
    }
}

$serviceFactory = new ServiceFactory();
$container = new Discolight($serviceFactory);
$serviceFactory->setContainer($container);

$basicAuthMiddleware = $container->get('middleware.basic-auth');
$jwtAuthMiddleware = $container->get('middleware.jwt-auth');

Fixed some unreachable code as well. The null check in the following code can not happen:

if ($returnType = $method->getReturnType()) {
    if (! $returnType->isBuiltin()) {
        $returnType = $method->getReturnType();
        if (null === $returnType) {
            throw new \RuntimeException(\sprintf(
                'The returnType of function %s is weird and breaks our system...',
                $method->getName()
            ));
         }

Oh, and I've removed the prooph/php-cs-fixer-config dependency for now, since it's not PHP 8 ready yet.

@zluiten zluiten changed the title Add PHP 8 support and configuration by attributes WIP Add PHP 8 support and configuration by attributes Dec 16, 2020
@zluiten zluiten changed the title WIP Add PHP 8 support and configuration by attributes [WIP] Add PHP 8 support and configuration by attributes Dec 16, 2020
@codeliner
Copy link
Contributor

I like the changes, but I'm not sure if we should merge it here or instead add PHP 8 attribute support to https://github.com/bitExpert/disco

As stated in the readme: Discolight is inspired by disco. Hence, the name ;)
We really like the DI container developed by @shochdoerfer, but wanted a light version without doc block annotations. Now with PHP 8 available the situation has changed. If disco will support PHP 8 attributes - and I'm sure Stephan is interested - we could switch back to disco as the default DI container for Event Engine.

@shochdoerfer , @Netiul , @sandrokeil What do you think?

@shochdoerfer
Copy link

That would be lovely. I had plans to create a version 1.0 of Disco supporting PHP 8 but I could not yet find the time to get started. I am more than willing to support this movement.

@zluiten zluiten marked this pull request as draft December 17, 2020 08:24
@zluiten
Copy link
Author

zluiten commented Dec 17, 2020

Ah, I came to this library via event-engine and I think it's really good. Never looked any further even though https://github.com/bitExpert/disco is indeed credited. I'm more than happy to move these changes and see if @shochdoerfer likes it.

@sandrokeil
Copy link
Contributor

Yes, we should go to that Disco, if they will play PHP 8 attributes. ;-)
@Netiul Would be nice if you can help @shochdoerfer with the implementation.

@shochdoerfer
Copy link

@Netiul feel free to open an issue in the Disco repo to discuss things. I assume we need to change quite a bit but that should not stop us ;)

@shochdoerfer
Copy link

ping @heiglandreas just in case you are bored during the Christmas holidays :P

…ons by using attributes.

This adds support for registering multiple instances of the same type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants