Skip to content

Conversation

UtechtDustin
Copy link
Contributor

This PR will add a new event handler list for an "connected"-event (ref #143).
I also had to add an duplicate check for the subscriptions, to prevent duplicated execution of the subscriptions callbacks.
I'm not sure if this should be the goal, maybe it could make more sense to add option for it or maybe clear the subscriptions if the connection is lost.

@Namoshek Do you have an idea for the duplicated subscription issue ?

@UtechtDustin UtechtDustin force-pushed the add-connected-event-handlers branch from 0fc0929 to 7de08c4 Compare February 10, 2023 09:31
@Namoshek
Copy link
Collaborator

Namoshek commented Feb 10, 2023

@UtechtDustin Thank you for the PR!

Looking at the SUBSCRIBE/SUBACK logic, I think that re-subscribing is only possible if the broker accepts it and therefore it should be safe to replace the old subscription with the new one. That's what I've done now.

I also extended the new event handler by passing an $isAutoReconnect flag. This should give you an additional hint whether it is safe to resubscribe or not.

Additionally, I've added some sweet integration tests to make sure this stuff actually works.

Please let me know whether that looks good to you. 👍

@Namoshek Namoshek marked this pull request as ready for review February 10, 2023 13:10
@UtechtDustin
Copy link
Contributor Author

Looks good for me, thank you for the quick response :)

@Namoshek Namoshek merged commit 99967b4 into php-mqtt:master Feb 10, 2023
@UtechtDustin UtechtDustin deleted the add-connected-event-handlers branch February 10, 2023 19:38
@Namoshek
Copy link
Collaborator

Not sure why those builds all failed with a false-positive, but anyway, the feature is release as v1.8.0. 👍

@Namoshek Namoshek changed the title Add connected event (handler) Add connected event handler Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants