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

[Notifier] [Telegram] Fix version and exception signature #51994

Merged

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Oct 11, 2023

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets --
License MIT

The PR

introduced a new exception in the Notifier component. I tested this locally and faced the following error:

Class "Symfony\Component\Notifier\Exception\MultipleExclusiveOptionsUsedException" not found

The version bump of the notifier was missing.
I also added tests for the exception class.

I am wondering about the parameter signature, and I am not sure if the second parameter should be nullable:

class MultipleExclusiveOptionsUsedException extends InvalidArgumentException
{
/**
* @param string[] $usedExclusiveOptions
* @param string[]|null $exclusiveOptions
*/
public function __construct(array $usedExclusiveOptions, array $exclusiveOptions = null, \Throwable $previous = null)
{
$message = sprintf('Multiple exclusive options have been used "%s".', implode('", "', $usedExclusiveOptions));
if (null !== $exclusiveOptions) {
$message .= sprintf(' Only one of %s can be used.', implode('", "', $exclusiveOptions));
}
parent::__construct($message, 0, $previous);
}

@igrizzli is there anything I am missing or can e adjust the signature:

- public function __construct(array $usedExclusiveOptions, array $exclusiveOptions = null, \Throwable $previous = null)
+ public function __construct(array $usedExclusiveOptions, array $exclusiveOptions, \Throwable $previous = null)
``

@OskarStark OskarStark self-assigned this Oct 11, 2023
@carsonbot carsonbot changed the title [Notifier][Telegram] Fix version [Notifier] [Telegram] Fix version Oct 11, 2023
@carsonbot carsonbot added this to the 6.4 milestone Oct 11, 2023
@igrizzli
Copy link
Contributor

Good catch, thank you.

@OskarStark OskarStark force-pushed the fix/composer-notifier-telegram branch from 9b069ad to c8701d8 Compare October 12, 2023 07:43
@OskarStark OskarStark changed the title [Notifier] [Telegram] Fix version [Notifier] [Telegram] Fix version and exception signature Oct 12, 2023
@OskarStark
Copy link
Contributor Author

Ready to merge from my side

@DiosaMorena DiosaMorena mentioned this pull request Oct 12, 2023
@stof
Copy link
Member

stof commented Oct 12, 2023

The telegram tests are failing on CI with an error about an undefined key. Is it coming from that PR as well ?

@OskarStark
Copy link
Contributor Author

The telegram tests are failing on CI with an error about an undefined key. Is it coming from that PR as well ?

I will check later

@OskarStark
Copy link
Contributor Author

I cannot reproduce the missing key, I tried to composer update --prefer-lowest, but tests are still passing 🤔

Besides that, I think we should not write json and decode it, but write an array and encode it in the assertion. But that should be a floor up PR once green

@OskarStark OskarStark force-pushed the fix/composer-notifier-telegram branch from df41238 to d384f8f Compare October 12, 2023 18:12
@OskarStark
Copy link
Contributor Author

@igrizzli any idea?

@OskarStark
Copy link
Contributor Author

OskarStark commented Oct 13, 2023

As far as @xabbuh and I can find out so far, dropping symfony/http-client:^5.4 helps, so now we have the same errors in high and low deps test.

For the errors itself, maybe

is related....

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.

LGTM thanks, just minor details

@@ -17,8 +17,8 @@
],
"require": {
"php": ">=8.1",
"symfony/http-client": "^5.4|^6.0|^7.0",
"symfony/notifier": "^6.2.7|^7.0"
"symfony/http-client": "^6.3|^7.0",
Copy link
Member

Choose a reason for hiding this comment

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

why this bump?

Copy link
Member

Choose a reason for hiding this comment

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

we need #49911

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and for this we need symfony/mime also

@OskarStark
Copy link
Contributor Author

Thanks @xabbuh, looks like symfony/mime was missing

use PHPUnit\Framework\TestCase;
use Symfony\Component\Notifier\Exception\MultipleExclusiveOptionsUsedException;

final class MultipleExclusiveOptionsUsedExceptionTest extends TestCase
Copy link
Member

Choose a reason for hiding this comment

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

I'm removing final : a test case is not an API, we don't need extra visual debt (same as void) ;)

@nicolas-grekas
Copy link
Member

Thank you @OskarStark.

@nicolas-grekas nicolas-grekas merged commit 1f2ec3e into symfony:6.4 Oct 13, 2023
2 of 4 checks passed
@OskarStark OskarStark deleted the fix/composer-notifier-telegram branch October 13, 2023 07:53
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

7 participants