Skip to content

[Hackday][Messenger] Add an alias for transport.symfony_serializer so SerializerInterface can be autowired #29517

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

Merged

Conversation

karser
Copy link
Contributor

@karser karser commented Dec 8, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

cc @thePanz

Use case:
Before:

    Pnz\Messenger\FilesystemTransport\FilesystemTransportFactory:
        arguments:
            - '@messenger.transport.symfony_serializer'
        tags: ['messenger.transport_factory']

After:

    Pnz\Messenger\FilesystemTransport\FilesystemTransportFactory:
        tags: ['messenger.transport_factory']

@stof
Copy link
Member

stof commented Dec 8, 2018

#SymfonyConHackday2018

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

👍 with minor comment.
Sounds more like a feature to me, better merge it on master I'd say.

@ogizanagi
Copy link
Contributor

I wondered about this alias before, but the serializer is configurable through the framework.messenger.serializer.id config.
So what would you expect to be injected when setting this option to a different value than the symfony_serializer and typehinting Messenger's SerializerInterface?
Shouldn't it alias messenger.transport.serializer instead?

@chalasr
Copy link
Member

chalasr commented Dec 8, 2018

Status: needs work
(#29517 (comment))

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Dec 9, 2018
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 9, 2018

I think we prefer defining in xml and removing in the extension: that makes the xml more useful to eg IDE plugins that want to use it.

@karser
Copy link
Contributor Author

karser commented Dec 10, 2018

@nicolas-grekas Did you mean this? please review

@nicolas-grekas nicolas-grekas modified the milestones: 4.2, next Dec 13, 2018
@nicolas-grekas nicolas-grekas changed the base branch from 4.2 to master December 13, 2018 10:40
@nicolas-grekas nicolas-grekas force-pushed the add-alias-to-messenger-serializer branch from 218ba84 to 2f0e948 Compare December 13, 2018 10:40
@nicolas-grekas
Copy link
Member

Thank you @karser.

@nicolas-grekas nicolas-grekas merged commit 2f0e948 into symfony:master Dec 13, 2018
nicolas-grekas added a commit that referenced this pull request Dec 13, 2018
…y_serializer so SerializerInterface can be autowired (karser)

This PR was submitted for the 4.2 branch but it was squashed and merged into the 4.3-dev branch instead (closes #29517).

Discussion
----------

[Hackday][Messenger] Add an alias for transport.symfony_serializer so SerializerInterface can be autowired

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| License       | MIT

cc @thePanz

Use case:
Before:
```
    Pnz\Messenger\FilesystemTransport\FilesystemTransportFactory:
        arguments:
            - '@messenger.transport.symfony_serializer'
        tags: ['messenger.transport_factory']
```

After:
```
    Pnz\Messenger\FilesystemTransport\FilesystemTransportFactory:
        tags: ['messenger.transport_factory']
```

Commits
-------

2f0e948 [Hackday][Messenger] Add an alias for transport.symfony_serializer so SerializerInterface can be autowired
@nicolas-grekas nicolas-grekas removed this from the next milestone Apr 30, 2019
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants