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

Middleware attribute #93

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Middleware attribute #93

wants to merge 18 commits into from

Conversation

xepozz
Copy link
Member

@xepozz xepozz commented Feb 17, 2024

Q A
Is bugfix?
New feature? ✔️
Breaks BC?

It allows users to use middlewares like the following:

<?php

declare(strict_types=1);

namespace Yiisoft\Middleware\Dispatcher\Tests\Support;

use Nyholm\Psr7\Response;
use Psr\Http\Message\ResponseInterface;
use Yiisoft\Middleware\Dispatcher\Attribute\Middleware;

final class TestController
{
    public function index(): ResponseInterface
    {
        return new Response(200, ['test' => 'yii']);
    }

    #[Middleware([
        'class' => ResponseMiddleware::class,
        '__construct()' => [200],
    ])]
    public function error(): ResponseInterface
    {
        return new Response(404);
    }

    #[Middleware([
        'class' => SetHeaderMiddleware::class,
        '__construct()' => ['x-test1', 'yii1'],
    ])]
    #[Middleware([
        'class' => SetHeaderMiddleware::class,
        '__construct()' => ['x-test2', 'yii2'],
    ])]
    #[Middleware([
        'class' => SetHeaderMiddleware::class,
        '__construct()' => ['x-test3', 'yii3'],
    ])]
    public function severalMiddlewares(): ResponseInterface
    {
        return new Response(404);
    }
}

Copy link

what-the-diff bot commented Feb 17, 2024

PR Summary

  • Introduction of New Middleware Class
    A new file was added which introduces a new class called Middleware. This class has a public property $definition, likely defining key aspects of the middleware.

  • Enhancements to MiddlewareFactory Class
    Various improvements have been made to the MiddlewareFactory class. A method to set the $eventDispatcher property has been added, as well as a method for creating a new class that works with a callback and other properties. Both these new methods are then incorporated in existing ones to ensure improved functionality.

  • Addition of New Test Cases
    A new test case was added for creating middleware from a controller class and action name. This will allow us to more thoroughly validate the accuracy of these processes.

  • Creation of ResponseMiddleware and SetHeaderMiddleware Classes
    Two new classes were added that implement 'MiddlewareInterface'. These likely provide new functionalities related to handling responses and setting headers.

  • Additions to TestController Class
    The TestController class has been improved by adding new methods. The error method includes a middleware attribute, while the severalMiddlewares method incorporates multiple middleware attributes. This is anticipated to optimize the testing capabilities within the system.

Copy link

codecov bot commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (91d8895) to head (89a00da).

Additional details and impacted files
@@             Coverage Diff             @@
##              master       #93   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       109       116    +7     
===========================================
  Files              8         9    +1     
  Lines            291       300    +9     
===========================================
+ Hits             291       300    +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samdark
Copy link
Member

samdark commented Feb 18, 2024

Interesting approach.

@xepozz xepozz requested a review from a team February 21, 2024 20:09
@xepozz xepozz added the status:code review The pull request needs review. label Feb 21, 2024
Comment on lines +39 to +40
private ContainerInterface $container,
private ?ParametersResolverInterface $parametersResolver = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why drop "readonly"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was written before the rector changes

@@ -29,6 +29,9 @@ public function __construct(
private MiddlewareFactory $middlewareFactory,
private ?EventDispatcherInterface $eventDispatcher = null
) {
if ($eventDispatcher !== null && !$middlewareFactory->hasEventDispatcher()) {
Copy link
Member

@vjik vjik Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest major refactoring...

1) Mark MiddlewareFactory as internal
2) Create MiddlewareFactory in constructor and replace private MiddlewareFactory $middlewareFactory, to:

private ContainerInterface $container,
private ?ParametersResolverInterface $parametersResolver = null
  1. Pass currently instance of dispatcher to MiddlewareFactory constructor.

It's allow use MiddlewareDispatcher::withMiddlewares() into MiddlewareFactory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no reasons to make MiddlewareFactory internal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with internal. But 2 and 3 is actual. MiddlewareFactory methods withEventDispatcher() and hasEventDispatcher() looks strange.

@vjik vjik added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants