Skip to content

[Notifier] [Mobyt] Change ctor signature and validate message types #39587

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
Jan 25, 2021
Merged

[Notifier] [Mobyt] Change ctor signature and validate message types #39587

merged 1 commit into from
Jan 25, 2021

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Dec 21, 2020 •
edited
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? yes (validating the message type)
Deprecations? no
Tickets ---
License MIT
Doc PR ---

cc @Deamon as you provided the bridge

@OskarStark OskarStark force-pushed the rework-mobyt-5.x branch 2 times, most recently from e24af00 to 4c5f459 Compare December 22, 2020 19:50
@OskarStark
Copy link
Contributor Author

Ready to merge from my side 👌🏻

@nicolas-grekas nicolas-grekas added this to the 5.x milestone Dec 23, 2020
@OskarStark
Copy link
Contributor Author

Rebased

@OskarStark OskarStark force-pushed the rework-mobyt-5.x branch 2 times, most recently from e54f539 to f47c319 Compare January 4, 2021 07:22
@OskarStark OskarStark force-pushed the rework-mobyt-5.x branch 2 times, most recently from 9aecc44 to 533f7ab Compare January 15, 2021 10:35
@fabpot
Copy link
Member

fabpot commented Jan 18, 2021

I think I won't change my mind. Message type is way worse than quality. I don't any good reason to change it.

@OskarStark
Copy link
Contributor Author

Ok, I will rework the PR to validate the allowed types and remove the BC break

@OskarStark OskarStark force-pushed the rework-mobyt-5.x branch 2 times, most recently from a0218fc to cc33680 Compare January 22, 2021 07:31
@OskarStark OskarStark changed the title [Notifier] [Mobyt] [BC BREAK] Rename type_quality parameter to message_type + change ctor signature and validate message types [Notifier] [Mobyt] [BC BREAK] Change ctor signature and validate message types Jan 22, 2021
@OskarStark OskarStark requested review from jderusse and fabpot January 22, 2021 07:47
@OskarStark OskarStark requested a review from derrabus January 22, 2021 07:47
derrabus
derrabus reviewed Jan 24, 2021
View reviewed changes
src/Symfony/Component/Notifier/Bridge/Mobyt/MobytTransport.php
@@ -33,11 +33,15 @@ final class MobytTransport extends AbstractTransport
private $from;
private $typeQuality;

public function __construct(string $accountSid, string $authToken, string $from, string $typeQuality, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
public function __construct(string $accountSid, string $authToken, string $from, string $typeQuality = null, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to make that parameter nullable anyway?

Suggested change
public function __construct(string $accountSid, string $authToken, string $from, string $typeQuality = null, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)
public function __construct(string $accountSid, string $authToken, string $from, string $typeQuality = MobytOptions::MESSAGE_TYPE_QUALITY_LOW, HttpClientInterface $client = null, EventDispatcherInterface $dispatcher = null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would suggest that staying BC from the ctor signature is easier if the default value can be handled in the ctor itself, isn't it? 🤔

Copy link
Contributor Author

@OskarStark OskarStark Jan 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah and we need to know the default value in the transport factory too:

symfony/src/Symfony/Component/Notifier/Bridge/Mobyt/MobytTransportFactory.php

Lines 27 to 43 in 73509d9

public function create(Dsn $dsn): TransportInterface
{
$scheme = $dsn->getScheme();
if ('mobyt' !== $scheme) {
throw new UnsupportedSchemeException($dsn, 'mobyt', $this->getSupportedSchemes());
}
$accountSid = $this->getUser($dsn);
$authToken = $this->getPassword($dsn);
$from = $dsn->getRequiredOption('from');
$typeQuality = $dsn->getOption('type_quality', MobytOptions::MESSAGE_TYPE_QUALITY_LOW);
$host = 'default' === $dsn->getHost() ? null : $dsn->getHost();
$port = $dsn->getPort();
return (new MobytTransport($accountSid, $authToken, $from, $typeQuality, $this->client, $this->dispatcher))->setHost($host)->setPort($port);
}

By using null, only the transport sets and knows about the default value

@OskarStark OskarStark changed the title [Notifier] [Mobyt] [BC BREAK] Change ctor signature and validate message types [Notifier] [Mobyt] Change ctor signature and validate message types Jan 25, 2021
@OskarStark OskarStark requested a review from derrabus January 25, 2021 07:22
@fabpot
Copy link
Member

fabpot commented Jan 25, 2021

Thank you @OskarStark.

@fabpot fabpot mentioned this pull request Jan 25, 2021
Closed
@fabpot fabpot merged commit 0114d26 into symfony:5.x Jan 25, 2021
@OskarStark OskarStark deleted the rework-mobyt-5.x branch January 25, 2021 08:32
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
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

@jderusse jderusse Awaiting requested review from jderusse

@derrabus derrabus Awaiting requested review from derrabus

Assignees
No one assigned
Labels
BC Break Notifier Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@OskarStark @fabpot @derrabus @nicolas-grekas @carsonbot