8000 [Messenger] Fix PHPDoc template for `TransportFactoryInterface` by Ferror · Pull Request #51374 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] Fix PHPDoc template for TransportFactoryInterface #51374

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
Sep 5, 2023
Merged

[Messenger] Fix PHPDoc template for TransportFactoryInterface #51374

merged 1 commit into from
Sep 5, 2023

Conversation

Ferror
Copy link
Contributor
@Ferror Ferror commented Aug 13, 2023

Improved a little bit phpdocs when dealing with Transport & Transport Factory Interface.

@carsonbot carsonbot added this to the 6.4 milestone Aug 13, 2023
derrabus
derrabus requested changes Aug 13, 2023
View reviewed changes
Copy link
Member
@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

If you make the TransportFactoryInterface generic, you need to update the PHPDoc on every implementation accordingly.

@Ferror Ferror force-pushed the messenger-phpdocs branch from d1304cc to c738259 Compare August 13, 2023 19:32
@Ferror Ferror requested a review from kbond as a code owner August 13, 2023 19:32
@Ferror Ferror force-pushed the messenger-phpdocs branch from c738259 to 655d4fd Compare August 13, 2023 19:33
@Ferror Ferror requested a review from derrabus August 13, 2023 19:33
@Ferror
Copy link
Contributor Author
Ferror commented Aug 13, 2023

If you make the TransportFactoryInterface generic, you need to update the PHPDoc on every implementation accordingly.

Did as requested :)

@OskarStark OskarStark changed the title [Messenger] - phpdoc template for TransportFactoryInterface [Messenger] Fix PHPDoc template for TransportFactoryInterface Aug 14, 2023
@Ferror Ferror changed the title [Messenger] Fix PHPDoc template for TransportFactoryInterface [Messenger] [Scheduler] Fix PHPDoc template for TransportFactoryInterface Aug 14, 2023
@Ferror < 8000 /div>
Copy link
Contributor Author
Ferror commented Sep 4, 2023

@derrabus WDYT?

@carsonbot carsonbot changed the title [Messenger] [Scheduler] Fix PHPDoc template for TransportFactoryInterface [Messenger] Fix PHPDoc template for TransportFactoryInterface Sep 4, 2023
@fabpot
Copy link
Member
fabpot commented Sep 5, 2023

Thank you @Ferror.

@fabpot fabpot merged commit 2bac801 into symfony:6.4 Sep 5, 2023
@Ferror Ferror deleted the messenger-phpdocs branch September 5, 2023 11:34
@ro0NL
Copy link
Contributor
ro0NL commented Sep 6, 2023

wasnt updating the returntypes on createTransport sufficient? eg. SchedulerTransportFactory

@Ferror
Copy link
Contributor Author
Ferror commented Sep 6, 2023

wasnt updating the returntypes on createTransport sufficient? eg. SchedulerTransportFactory

Unfortunately not. SchedulerTransportFactory shares Transport Interface therefore by pipeline config it was also required to change to add that.

@ro0NL
Copy link
Contributor
ro0NL commented Sep 6, 2023

i mean why prefer templating a factory when the return type is already sufficient?

symfony/src/Symfony/Component/Scheduler/Messenger/SchedulerTransportFactory.php

Line 34 in 2bac801

public function createTransport(string $dsn, array $options, SerializerInterface $serializer): SchedulerTransport

or what does this PR solve? 🤔

@stof
Copy link
Member
stof commented Sep 6, 2023

If we never benefit from the generic types in places in which we inject a TransportFactory but only in places using a concrete factory explicitly, we could indeed avoid making it generic but using a refined return type...

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

@kbond kbond kbond approved these changes

@derrabus derrabus derrabus approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@Ferror @fabpot @ro0NL @stof @kbond @derrabus @carsonbot
0