Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (seehttps://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
• Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Hi, thanks for the PR, as this is a new feature, please target 7.1

Hi, thanks for the PR, as this is a new feature, please target 7.1

Is it really a new feature or an oversight?
TheNotifier component documentation tells us that the transport use Event to interact in the lifecycle of the event

TheTransport class of the Notifier component allows you to optionally hook into the lifecycle via events.

Currently, the hooks aren't working with the FakeSms/FakeChat and Mercure as they should.

Not sure, from a technical POV it is a new feature, because they were never meant to dispatch those events, but I can understand your point.

cc@nicolas-grekas

I will give some argument in favour of a bugfix 😁

Those transports (at least the Fake* ones) are intended to be used in dev/test, but we cannot test the events behaviours, and it is not documented that they don't support those events.

Technically, the transports classdoes have everything to support the dispatch of those events, they correctly extends the AbstractTransport class and call the extended class constructor passing the two HttpClient and EventDispatcher argument.

The factories, in the other hand, don't handle the extended AbstractTransportFactory class constructor arguments. As all the other factories don't override the constructor, there is no precedent.
As those 3 specific ones need to override the constructor, I see no point not to pass them.

@fabpot , as you are the designer of the component and in order to determine if this is bug since 5.3 or a new feature for 7.x., do you have an opinion on that : does every TransportFactory need to pass EventDispatcher and HttpClient ?

I will target whichever you finally decide, tell me.

Thank you@rdavaillaud.

Can you also remove the use of named arguments? We don't use them in core except here and it looks like an oversight.

I will try to see what I can do with this, I need to dig in and understand why. I would open a PR then.

Can you please check on 6.4/7.1 if there are more bridges that would need a similar change and submit a PR if yes?

From what I can see, those three bridges are the only ones.

Behause we missed to add a conflict in FrameworkBundle 6.4.2 to require fake-chat and fake-sms at least in 6.4.2 ends up in the following error:

Invalid service "notifier.transport_factory.fake-sms": method "Symfony\Component\Notifier\Bridge\FakeSms\FakeSmsTransportFactory::__construct()" has no argument named "$client". Check your service definition.

The solution is to upgrade the notifier bridges to6.4.2 first

cc@xabbuh

The same issue should be observable too when updating FrameworkBundle from 5.4.33 to 5.4.34, 6.3.10 to 6.3.11 or from 7.0.1 to 7.0.2 without also upgrading these bridges.

Damn, I am not used to those dependencies conflicts, sorry. What are the rules I missed?
Can I do something to mitigate the issue ?

I suppose removing the use of named arguments as requested by@nicolas-grekas is the way to go, but I didn't worked on this until now. And this will maybe need some other things to do in composer that I don't know of.

@OskarStark, as you are the first contributor on these bridges, can you explain to me why was it necessary at the first place to put those lines in Framework Bundle? This may put me in the fast lane to understand.

Removing named arguments is the way to go, no need for anything else (no conflicts).

see#53341