8000 [Messenger] Remove the Doctrine middleware configuration from the FrameworkBundle by sroze · Pull Request #26684 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] Remove the Doctrine middleware configuration from the FrameworkBundle #26684

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 4, 2018
Merged

[Messenger] Remove the Doctrine middleware configuration from the FrameworkBundle #26684

merged 1 commit into from
Apr 4, 2018

Conversation

sroze
Copy link
Contributor
@sroze sroze commented Mar 27, 2018 •
edited
Loading
Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26647
License MIT
Doc PR ø

As per @fabpot's request, remove the enabling feature of the DoctrineMiddleware from the FramworkBundle.

/cc @Nyholm

@sroze sroze force-pushed the remove-doctrine-middleware-configuration branch from 55a3ffc to 0705506 Compare March 27, 2018 13:34
@sroze sroze mentioned this pull request Mar 27, 2018
Closed
@Nyholm
Copy link
Member
Nyholm commented Mar 27, 2018

Im not convinced. It feels like all middlewares should be registered at the same place. Especially if we do #26652.

I do also imagine we could register middlewares like:

framework:
  messenger:
    middlewares: ~ # default middlewares
    bus_foo:
      type: foo
      routing:
        'App\Bar': ['sender.bar', 'sender.biz']
        'App\Foo': 'sender.foo'
      middlewares:  # bus specific middlewares
        - 'custom_service'
        - validation: true

@fabpot
Copy link
Member
fabpot commented Mar 27, 2018

@Nyholm There is a difference between being able to register all middleware services at the same place and hardcoding a specific middleware for Doctrine (third-party libraries won't be able to do that).

@sroze
Copy link
Contributor Author
sroze commented Mar 27, 2018

For the records, a bit of the discussion is happening here as well: #26647 (comment)

@Nyholm
Copy link
Member
Nyholm commented Mar 27, 2018

There is a difference between being able to register all middleware services at the same place and hardcoding a specific middleware for Doctrine (third-party libraries won't be able to do that).

Yes. I know. I just wanted to share this idea with you (everybody). Maybe I should put it on #26652.

@javiereguiluz javiereguiluz added this to the 4.1 milestone Mar 28, 2018
@sroze
Copy link
Contributor Author
sroze commented Apr 3, 2018

@fabpot at the end, WDYT about this one? Shall we close it or revert that on the FrameworkBundle (which will require a PR on the DoctrineBundle later)? 🤔

@fabpot
Copy link
Member
fabpot commented Apr 3, 2018

I haven't changed my mind on this one.

@sroze
Copy link
Contributor Author
sroze commented Apr 3, 2018

Fair enough. It's waiting for votes to go ahead then.

@fabpot
Copy link
Member
fabpot commented Apr 4, 2018

@sroze Can you rebase this one so that I can merge it? Thank you.

@sroze sroze force-pushed the remove-doctrine-middleware-configuration branch from 0705506 to 27a8b1d Compare April 4, 2018 12:53
@sroze
Copy link
Contributor Author
sroze commented Apr 4, 2018

Rebased. @Nyholm could you send a PR on the DoctrineBundle to port this feature there?

@fabpot
Copy link
Member
fabpot commented Apr 4, 2018

Thank you @sroze.

@fabpot fabpot merged commit 27a8b1d into symfony:master Apr 4, 2018
fabpot added a commit that referenced this pull request Apr 4, 2018
@fabpot
feature #26684 [Messenger] Remove the Doctrine middleware configurati…
572042c 
…on from the FrameworkBundle (sroze)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Messenger] Remove the Doctrine middleware configuration from the FrameworkBundle

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26647
| License       | MIT
| Doc PR        | ø

As per @fabpot's request, remove the enabling feature of the DoctrineMiddleware from the FramworkBundle.

/cc @Nyholm

Commits
-------

27a8b1d Remove the Doctrine middleware configuration from the FrameworkBundle
@sroze sroze deleted the remove-doctrine-middleware-configuration branch April 8, 2018 10:20
@javiereguiluz javiereguiluz mentioned this pull request Apr 20, 2018
Closed
@sroze sroze mentioned this pull request Apr 20, 2018
Closed
@ogizanagi ogizanagi mentioned this pull request May 2, 2018
Merged
@fabpot fabpot mentioned this pull request May 7, 2018
Merged
sroze added a commit that referenced this pull request May 14, 2018
@sroze
feature #27128 [Messenger] Middleware factories support in config (og…
f59ce97 
…izanagi)

This PR was squashed before being merged into the 4.1 branch (closes #27128).

Discussion
----------

[Messenger] Middleware factories support in config

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no  <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo

Following #26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references).

For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the `DoctrineTransactionMiddleware` reverted in #26684):

```yaml
framework:
    messenger:
      buses:
        default:
          middleware:
            - logger
            - doctrine_transaction_middleware: ['entity_manager_name']
```

where `doctrine_transaction_middleware` would be an abstract factory definition provided by the doctrine bundle:

```yml
services:

    doctrine.orm.messenger.middleware_factory.transaction:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory
      arguments: ['@doctrine']

    doctrine_transaction_middleware:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware
      factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware']
      abstract: true
      # the default arguments to use when none provided from config.
      # i.e:
      #   middlewares:
      #     - doctrine_transaction_middleware: ~
      arguments: ['default']
```

and is interpreted as:

```yml
buses:
    default:
        middleware:
            -
                id: logger
                arguments: {  }
            -
                id: doctrine_transaction_middleware
                arguments:
                    - entity_manager_name
        default_middleware: true
```

---

<details>

<summary>Here is the whole config reference with these changes: </summary>

```yaml
# Messenger configuration
messenger:
    enabled:              true
    routing:

        # Prototype
        message_class:
            senders:              []
    serializer:
        enabled:              true
        format:               json
        context:

            # Prototype
            name:                 ~
    encoder:              messenger.transport.serializer
    decoder:              messenger.transport.serializer
    adapters:

        # Prototype
        name:
            dsn:                  ~
            options:              []
    default_bus:          null
    buses:

        # Prototype
        name:
            default_middleware:  true
            middleware:

                # Prototype
                -
                    id:                   ~ # Required
                    arguments:            []
```

</details>

Commits
-------

f5ef421 [Messenger] Middleware factories support in config
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request May 14, 2018
@sroze
feature #27128 [Messenger] Middleware factories support in config (og…
c9d4a71 
…izanagi)

This PR was squashed before being merged into the 4.1 branch (closes #27128).

Discussion
----------

[Messenger] Middleware factories support in config

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no  <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo

Following symfony/symfony#26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references).

For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the `DoctrineTransactionMiddleware` reverted in symfony/symfony#26684):

```yaml
framework:
    messenger:
      buses:
        default:
          middleware:
            - logger
            - doctrine_transaction_middleware: ['entity_manager_name']
```

where `doctrine_transaction_middleware` would be an abstract factory definition provided by the doctrine bundle:

```yml
services:

    doctrine.orm.messenger.middleware_factory.transaction:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory
      arguments: ['@doctrine']

    doctrine_transaction_middleware:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware
      factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware']
      abstract: true
      # the default arguments to use when none provided from config.
      # i.e:
      #   middlewares:
      #     - doctrine_transaction_middleware: ~
      arguments: ['default']
```

and is interpreted as:

```yml
buses:
    default:
        middleware:
            -
                id: logger
                arguments: {  }
            -
                id: doctrine_transaction_middleware
                arguments:
                    - entity_manager_name
        default_middleware: true
```

---

<details>

<summary>Here is the whole config reference with these changes: </summary>

```yaml
# Messenger configuration
messenger:
    enabled:              true
    routing:

        # Prototype
        message_class:
            senders:              []
    serializer:
        enabled:              true
        format:               json
        context:

            # Prototype
            name:                 ~
    encoder:              messenger.transport.serializer
    decoder:              messenger.transport.serializer
    adapters:

        # Prototype
        name:
            dsn:                  ~
            options:              []
    default_bus:          null
    buses:

        # Prototype
        name:
            default_middleware:  true
            middleware:

                # Prototype
                -
                    id:                   ~ # Required
                    arguments:            []
```

</details>

Commits
-------

f5ef421474 [Messenger] Middleware factories support in config
symfony-splitter pushed a commit to symfony/messenger that referenced this pull request May 14, 2018
@sroze
feature #27128 [Messenger] Middleware factories support in config (og…
4ecffae 
…izanagi)

This PR was squashed before being merged into the 4.1 branch (closes #27128).

Discussion
----------

[Messenger] Middleware factories support in config

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no  <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo

Following symfony/symfony#26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references).

For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the `DoctrineTransactionMiddleware` reverted in symfony/symfony#26684):

```yaml
framework:
    messenger:
      buses:
        default:
          middleware:
            - logger
            - doctrine_transaction_middleware: ['entity_manager_name']
```

where `doctrine_transaction_middleware` would be an abstract factory definition provided by the doctrine bundle:

```yml
services:

    doctrine.orm.messenger.middleware_factory.transaction:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory
      arguments: ['@doctrine']

    doctrine_transaction_middleware:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware
      factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware']
      abstract: true
      # the default arguments to use when none provided from config.
      # i.e:
      #   middlewares:
      #     - doctrine_transaction_middleware: ~
      arguments: ['default']
```

and is interpreted as:

```yml
buses:
    default:
        middleware:
            -
                id: logger
         
8000
       arguments: {  }
            -
                id: doctrine_transaction_middleware
                arguments:
                    - entity_manager_name
        default_middleware: true
```

---

<details>

<summary>Here is the whole config reference with these changes: </summary>

```yaml
# Messenger configuration
messenger:
    enabled:              true
    routing:

        # Prototype
        message_class:
            senders:              []
    serializer:
        enabled:              true
        format:               json
        context:

            # Prototype
            name:                 ~
    encoder:              messenger.transport.serializer
    decoder:              messenger.transport.serializer
    adapters:

        # Prototype
        name:
            dsn:                  ~
            options:              []
    default_bus:          null
    buses:

        # Prototype
        name:
            default_middleware:  true
            middleware:

                # Prototype
                -
                    id:                   ~ # Required
                    arguments:            []
```

</details>

Commits
-------

f5ef421474 [Messenger] Middleware factories support in config
symfony-splitter pushed a commit to symfony/messenger that referenced this pull request Jan 28, 2020
@sroze
feature #27128 [Messenger] Middleware factories support in config (og…
7d3129a 
…izanagi)

This PR was squashed before being merged into the 4.1 branch (closes #27128).

Discussion
----------

[Messenger] Middleware factories support in config

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no  <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo

Following symfony/symfony#26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references).

For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the `DoctrineTransactionMiddleware` reverted in symfony/symfony#26684):

```yaml
framework:
    messenger:
      buses:
        default:
          middleware:
            - logger
            - doctrine_transaction_middleware: ['entity_manager_name']
```

where `doctrine_transaction_middleware` would be an abstract factory definition provided by the doctrine bundle:

```yml
services:

    doctrine.orm.messenger.middleware_factory.transaction:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory
      arguments: ['@doctrine']

    doctrine_transaction_middleware:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware
      factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware']
      abstract: true
      # the default arguments to use when none provided from config.
      # i.e:
      #   middlewares:
      #     - doctrine_transaction_middleware: ~
      arguments: ['default']
```

and is interpreted as:

```yml
buses:
    default:
        middleware:
            -
                id: logger
                arguments: {  }
            -
                id: doctrine_transaction_middleware
                arguments:
                    - entity_manager_name
        default_middleware: true
```

---

<details>

<summary>Here is the whole config reference with these changes: </summary>

```yaml
# Messenger configuration
messenger:
    enabled:              true
    routing:

        # Prototype
        message_class:
            senders:              []
    serializer:
        enabled:              true
        format:               json
        context:

            # Prototype
            name:                 ~
    encoder:              messenger.transport.serializer
    decoder:              messenger.transport.serializer
    adapters:

        # Prototype
        name:
            dsn:                  ~
            options:              []
    default_bus:          null
    buses:

        # Prototype
        name:
            default_middleware:  true
            middleware:

                # Prototype
                -
                    id:                   ~ # Required
                    arguments:            []
```

</details>

Commits
-------

f5ef421474 [Messenger] Middleware factories support in config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Bug Messenger Status: Reviewed
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
5 participants
@sroze @Nyholm @fabpot @javiereguiluz @carsonbot
0