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

OskarStark · 2023-10-11T14:19:51Z

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

The PR

[Notifier] [Telegram] Extend options for location, document, audio, video, venue, photo, animation, sticker & contact #51717

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:

symfony/src/Symfony/Component/Notifier/Exception/MultipleExclusiveOptionsUsedException.php

Lines 17 to 31 in c868be4

    
           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)
``

src/Symfony/Component/Notifier/Exception/MultipleExclusiveOptionsUsedException.php

igrizzli · 2023-10-11T16:24:48Z

Good catch, thank you.

src/Symfony/Component/Notifier/Tests/Exception/MultipleExclusiveOptionsUsedExceptionTest.php

OskarStark · 2023-10-12T07:45:11Z

Ready to merge from my side

src/Symfony/Component/Notifier/Tests/Exception/MultipleExclusiveOptionsUsedExceptionTest.php

src/Symfony/Component/Notifier/Bridge/Telegram/composer.json

src/Symfony/Component/Notifier/Exception/MultipleExclusiveOptionsUsedException.php

stof · 2023-10-12T12:47:22Z

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

src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php

OskarStark · 2023-10-12T12:52:35Z

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 · 2023-10-12T14:32:57Z

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 · 2023-10-13T07:07:38Z

@igrizzli any idea?

OskarStark · 2023-10-13T07:32:51Z

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

[HttpClient] Support file uploads by nesting resource streams in body option #49911

is related....

nicolas-grekas

LGTM thanks, just minor details

src/Symfony/Component/Notifier/Tests/Exception/MultipleExclusiveOptionsUsedExceptionTest.php

nicolas-grekas · 2023-10-13T07:44:54Z

src/Symfony/Component/Notifier/Bridge/Telegram/composer.json

@@ -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",


why this bump?

we need #49911

and for this we need symfony/mime also

OskarStark · 2023-10-13T07:48:07Z

Thanks @xabbuh, looks like symfony/mime was missing

nicolas-grekas · 2023-10-13T07:52:53Z

src/Symfony/Component/Notifier/Tests/Exception/MultipleExclusiveOptionsUsedExceptionTest.php

+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Notifier\Exception\MultipleExclusiveOptionsUsedException;
+
+final class MultipleExclusiveOptionsUsedExceptionTest extends TestCase


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

nicolas-grekas · 2023-10-13T07:53:12Z

Thank you @OskarStark.

OskarStark added the Notifier label Oct 11, 2023

OskarStark self-assigned this Oct 11, 2023

carsonbot added Status: Needs Review Bug labels Oct 11, 2023

carsonbot changed the title ~~[Notifier][Telegram] Fix version~~ [Notifier] [Telegram] Fix version Oct 11, 2023

carsonbot added this to the 6.4 milestone Oct 11, 2023

OskarStark requested a review from nicolas-grekas October 11, 2023 14:23

OskarStark assigned fabpot Oct 11, 2023

igrizzli suggested changes Oct 11, 2023

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 11, 2023

OskarStark commented Oct 11, 2023

View reviewed changes

src/Symfony/Component/Notifier/Tests/Exception/MultipleExclusiveOptionsUsedExceptionTest.php Outdated Show resolved Hide resolved

OskarStark force-pushed the fix/composer-notifier-telegram branch from 9b069ad to c8701d8 Compare October 12, 2023 07:43

OskarStark changed the title ~~[Notifier] [Telegram] Fix version~~ [Notifier] [Telegram] Fix version and exception signature Oct 12, 2023

OskarStark requested a review from fabpot October 12, 2023 07:44

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 12, 2023

OskarStark commented Oct 12, 2023

View reviewed changes

src/Symfony/Component/Notifier/Tests/Exception/MultipleExclusiveOptionsUsedExceptionTest.php Outdated Show resolved Hide resolved

OskarStark commented Oct 12, 2023

View reviewed changes

src/Symfony/Component/Notifier/Tests/Exception/MultipleExclusiveOptionsUsedExceptionTest.php Outdated Show resolved Hide resolved

DiosaMorena mentioned this pull request Oct 12, 2023

HolaDone #52014

Closed

igrizzli approved these changes Oct 12, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 12, 2023

stof reviewed Oct 12, 2023

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Telegram/composer.json Show resolved Hide resolved

stof requested changes Oct 12, 2023

View reviewed changes

src/Symfony/Component/Notifier/Exception/MultipleExclusiveOptionsUsedException.php Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Exception/MultipleExclusiveOptionsUsedException.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 12, 2023

OskarStark commented Oct 12, 2023

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Telegram/Tests/TelegramTransportTest.php Show resolved Hide resolved

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

OskarStark requested a review from stof October 13, 2023 07:07

carsonbot added Status: Needs Review and removed Status: Needs Work labels Oct 13, 2023

OskarStark mentioned this pull request Oct 13, 2023

[Notifier] bump minimum required Notifier component version #52028

Closed

nicolas-grekas approved these changes Oct 13, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 13, 2023

[Notifier] [Telegram] Fix version and exception signature

74e0c9c

nicolas-grekas reviewed Oct 13, 2023

View reviewed changes

nicolas-grekas force-pushed the fix/composer-notifier-telegram branch from 6753c99 to 74e0c9c Compare October 13, 2023 07:53

nicolas-grekas merged commit 1f2ec3e into symfony:6.4 Oct 13, 2023

OskarStark deleted the fix/composer-notifier-telegram branch October 13, 2023 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

OskarStark commented Oct 11, 2023 •

edited

Loading

igrizzli commented Oct 11, 2023

OskarStark commented Oct 12, 2023

stof commented Oct 12, 2023

OskarStark commented Oct 12, 2023

OskarStark commented Oct 12, 2023

OskarStark commented Oct 13, 2023

OskarStark commented Oct 13, 2023 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Oct 13, 2023

xabbuh Oct 13, 2023

OskarStark Oct 13, 2023

OskarStark commented Oct 13, 2023

nicolas-grekas Oct 13, 2023

nicolas-grekas commented Oct 13, 2023

	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);
	}

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

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

Conversation

OskarStark commented Oct 11, 2023 • edited Loading

igrizzli commented Oct 11, 2023

OskarStark commented Oct 12, 2023

stof commented Oct 12, 2023

OskarStark commented Oct 12, 2023

OskarStark commented Oct 12, 2023

OskarStark commented Oct 13, 2023

OskarStark commented Oct 13, 2023 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Oct 13, 2023

Choose a reason for hiding this comment

xabbuh Oct 13, 2023

Choose a reason for hiding this comment

OskarStark Oct 13, 2023

Choose a reason for hiding this comment

OskarStark commented Oct 13, 2023

nicolas-grekas Oct 13, 2023

Choose a reason for hiding this comment

nicolas-grekas commented Oct 13, 2023

OskarStark commented Oct 11, 2023 •

edited

Loading

OskarStark commented Oct 13, 2023 •

edited

Loading