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

[FrameworkBundle] append instead of replacing potentially non-existent named-arguments #53341

Merged
merged 1 commit into from Jan 2, 2024

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jan 2, 2024

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52998 (comment)
License MIT

@carsonbot carsonbot added this to the 5.4 milestone Jan 2, 2024
@xabbuh xabbuh changed the title do not deal with named-arguments that may not exist [FrameworkBundle] do not deal with named-arguments that may not exist Jan 2, 2024
@xabbuh xabbuh requested a review from OskarStark January 2, 2024 10:15
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Can't we stop using named args at all?
So those args are already wired in the default config files?

@xabbuh
Copy link
Member Author

xabbuh commented Jan 2, 2024

Can't we stop using named args at all?
So those args are already wired in the default config files?

That's what we do now. Or do I misunderstand what you are asking for?

@rdavaillaud
Copy link
Contributor

The problem is that the bridges are wired via a factory that is declared via an abstract factory.

see

->set('notifier.transport_factory.fake-sms', FakeSmsTransportFactory::class)

As far as I have tested, if we remove the HttpClient and Dispatcher from the definition, they will not be injected in the constructor.

@xabbuh
Copy link
Member Author

xabbuh commented Jan 2, 2024

I mean we revert the framework extension changes made in #52998 which were not necessary because of the notifier.transport_factory.abstract definition: https://github.com/symfony/symfony/blob/5.4/src/Symfony/Bundle/FrameworkBundle/Resources/config/notifier_transports.php#L90-L92

@rdavaillaud
Copy link
Contributor

If we revert the changes, the DI will generate this code (at least in 6.3):

class getNotifier_TransportFactory_FakesmsService extends App_KernelDevDebugContainer
{
    /**
     * Gets the private 'notifier.transport_factory.fake-sms' shared service.
     *
     * @return \Symfony\Component\Notifier\Bridge\FakeSms\FakeSmsTransportFactory
     */
    public static function do($container, $lazyLoad = true)
    {
        include_once \dirname(__DIR__, 4).'/vendor/symfony/notifier/Transport/TransportFactoryInterface.php';
        include_once \dirname(__DIR__, 4).'/vendor/symfony/notifier/Transport/AbstractTransportFactory.php';
        include_once \dirname(__DIR__, 4).'/vendor/symfony/fake-sms-notifier/FakeSmsTransportFactory.php';

        return new \Symfony\Component\Notifier\Bridge\FakeSms\FakeSmsTransportFactory(($container->privates['mailer.mailer'] ?? self::getMailer_MailerService($container)), ($container->privates['monolog.logger'] ?? self::getMonolog_LoggerService($container)));
    }
}

The HttpClient and EventDispatcher are replaced, not injected.

@rdavaillaud
Copy link
Contributor

If we revert the FrameworkBundle, we must also change the signatures of the bridges Fake* constructor to put Mailer and Logger at the end.

@xabbuh xabbuh force-pushed the pr-52998 branch 2 times, most recently from 1737a93 to 9edd5fb Compare January 2, 2024 10:51
@xabbuh xabbuh changed the title [FrameworkBundle] do not deal with named-arguments that may not exist [FrameworkBundle] append instead of replacing potentially non-existent named-arguments Jan 2, 2024
@rdavaillaud
Copy link
Contributor

@xabbuh your last change seems to work with the old bridge, and the modified one. 👍

@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit 46ce601 into symfony:5.4 Jan 2, 2024
9 of 11 checks passed
@xabbuh xabbuh deleted the pr-52998 branch January 2, 2024 11:04
@xabbuh
Copy link
Member Author

xabbuh commented Jan 2, 2024

@rdavaillaud Thank you for testing it!

@fabpot fabpot mentioned this pull request Jan 30, 2024
This was referenced Jan 30, 2024
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.

None yet

5 participants