Messenger: validate options for AMQP and Redis Connections #34925

nikophil · 2019-12-10T17:15:28Z

Q	A
Branch?	master
Bug fix?	no
Ticket	#32575
New feature?	yes
Deprecations?	yes
License	MIT

This PR validates options for AMQP and Redis connections.

Regarding AMQP, I've merged symfony's specific options (exchange, delay...) with the ones available in the AMQP Extension and i've enhanced the phpdoc with the one found here

src/Symfony/Component/Messenger/Transport/RedisExt/Connection.php

src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/AmqpTransportFactoryTest.php

chalasr · 2019-12-16T15:16:30Z

Can you please add an entry to UPGRADE-5.1.md and UPGRADE-6.0.md?

nikophil · 2019-12-16T15:19:05Z

@chalasr done

Tests need to use @expectedDeprecation instead of expectedException

nikophil · 2019-12-16T15:51:10Z

indeed, i had to update tests

nikophil · 2020-01-04T12:44:40Z

I don't know where this error comes from in appveyor:

Uncaught Error: Call to a member function beStrictAboutTestsThatDoNotTestAnything() on null

i guess because redis is unreachable on app veryor, but, even if i add an assertion in the test, i still get the error

chalasr · 2020-01-25T15:12:29Z

@nikophil The mentioned failure seems to be gone. Can you rebase to see tests green?

nikophil · 2020-01-25T15:20:11Z

i've done that sooner this morning, but still see the failure on app veyor + some new errors in travis which are not related to this PR

chalasr · 2020-01-25T15:21:29Z

Ok thanks, I'm looking at it.

Nyholm · 2020-01-26T11:22:50Z

src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php

@@ -155,6 +217,30 @@ public static function fromDsn(string $dsn, array $options = [], AmqpFactory $am
        return new self($amqpOptions, $exchangeOptions, $queuesOptions, $amqpFactory);
    }

+    private static function validateOptions(array $options): void
+    {
+        if (0 < \count($invalidOptions = array_diff(array_keys($options), self::AVAILABLE_OPTIONS))) {


Why not use option resolver component?

Tobion · 2020-01-26T21:18:08Z

IMO the options resolver should indeed be used for this to not reinvent the wheel.

nikophil · 2020-01-27T13:59:57Z

ok, i'm gonna do that 👍

nikophil · 2020-01-29T07:42:41Z

@Tobion @Nyholm i've started to work on the usage of an option resolver, but it seems that's not the good solution, because we want to only deprecate the use of invalid options (look at this resolved comment). But OptionResolver::resovle() is strict and only throws exception.

I wanted to replace my validateOptions() method by something like:

        $resolver = new OptionsResolver();
        self::configureOptions($resolver);

        try {
            $amqpOptions = $resolver->resolve($options);
        } catch (OptionResolverInvalidArgumentException $exception) {
            @trigger_error($exception->getMessage()."\nPassing invalid options is deprecated since Symfony 5.1.", E_USER_DEPRECATED);
        }

but this would lead to $amqpOptions not to be instantiated.

WDYT?

Nyholm · 2020-01-29T08:26:32Z

You should still define deprecated options as valid. Then after you do the option resolver validation, you can check if the deprecated options were used.

nikophil · 2020-01-29T08:31:58Z

The actual problem is not to define some known options as deprecated, but to deprecate the use of any invalid option which we can't know in advance... i think this behavior does not exist in option resolver

Nyholm · 2020-01-29T08:46:23Z

Oh, sorry that is right.. We currently allow all options, and you want do deprecate all but a few named...

Hm... Could we do a hack? (Or do we want to do a hack?)
Like configure the option resolver after we look at the input.

Valid options: a b c
Input options: a f

Deprecated: array_diff (input, valid)
OptionResovler->setDefaults(valid)
foreach(deprecated as d)
   OptionResolver->setDeprecated(d)

That would always deprecate options that are not valid, right?
(I think it will work)

nikophil · 2020-01-29T08:53:38Z

haha, this seems terribly hacky!

that would definitely work, but because i'll do some array_diff on three different places, is it still relevant to use option resolver?

i'll follow your decision ;)

chalasr · 2020-01-29T15:01:11Z

Given the current implementation works, I’d not try to « hijack » the option-resolver here.

Nyholm · 2020-01-29T16:43:33Z

I agree with Robin.
Thank you for trying this out for us.

nikophil · 2020-01-29T17:22:03Z

it would have been nice to use the option resolver here. This have to be done in the 6.0 version!
in.... 2 years 😱

Nyholm · 2020-01-29T17:24:51Z

Yeah. =)

Could you rebase the PR?

Most of these files has been moved to Messenger\Bridge

nikophil · 2020-01-29T17:25:29Z

yep i was doing that right now ;)

nikophil · 2020-01-29T17:40:50Z

@Nyholm that's OK now,
i guess we'll have to wait for #35489 to be merged before having these tests green

Nyholm

Just a small thing

Nyholm · 2020-01-29T17:45:24Z

src/Symfony/Component/Messenger/CHANGELOG.md

@@ -7,6 +7,7 @@ CHANGELOG
 * Moved AmqpExt transport to package `symfony/amqp-messenger`. All classes in `Symfony\Component\Messenger\Transport\AmqpExt` have been moved to `Symfony\Component\Messenger\Bridge\Amqp\Transport`
 * Moved Doctrine transport to package `symfony/doctrine-messenger`. All classes in `Symfony\Component\Messenger\Transport\Doctrine` have been moved to `Symfony\Component\Messenger\Bridge\Doctrine\Transport`
 * Moved RedisExt transport to package `symfony/redis-messenger`. All classes in `Symfony\Component\Messenger\Transport\RedisExt` have been moved to `Symfony\Component\Messenger\Bridge\Redis\Transport`
+* Deprecated use of invalid options in Redis and AMQP connections.


You dont need to add this changelog here. The Redis and AMQP have a separate changelog.

i've split this one between amqp & redis' changelogs

chalasr · 2020-01-29T18:52:08Z

You dont need to add this changelog here. The Redis and AMQP have a separate changelog.

Wait! Now that we moved transports to separate packages, do we really care about providing an upgrade path here? I mean, can't we only patch the new transports and make them throw directly in case of unknown options instead of triggering deprecation notices?
You'll have to change your code in order to use the new packages anyways.
That would unlock the use of OptionResolver.

Note that this question applies to any change that has BC concerns for 5.1 features on messenger transports, do we want to introduce the new packages with deprecations?

EDIT: Not relevant as per #34925 (comment).

Nyholm · 2020-01-29T19:32:22Z

No, sorry.
There are class aliases so you can keep using the "old classes" until 6.0. Even though you reference old classes you will still be using new code.

chalasr · 2020-01-29T19:35:36Z

@Nyholm Right, thank you.

nikophil · 2020-01-29T20:04:09Z

it seems ok, but i don't know why the deprecation tests fails... where is coming from this weird %A char? 🤔

1) Symfony\Component\Messenger\Bridge\Redis\Tests\Transport\ConnectionTest::testDeprecationIfInvalidOptionIsPassedWithDsn

Failed asserting that string matches format description.

--- Expected

+++ Actual

@@ @@

 @expectedDeprecation:

-%A  Invalid option(s) "foo" passed to the Redis Messenger transport. Passing invalid options is deprecated since Symfony 5.1.

+  Invalid option(s) "foo" passed to the Redis Messenger transport. Passing invalid options is deprecated since Symfony 5.1.

it's on Travis, but i've got that in local as well

…eprecations (chalasr) This PR was merged into the 3.4 branch. Discussion ---------- [PhpUnitBridge] Fix running skipped tests expecting only deprecations | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - If a test class has unsatisfied `@requires` and contains test methods expecting deprecation only, you get: > Fatal error: Uncaught Error: Call to a member function beStrictAboutTestsThatDoNotTestAnything() on null in ./symfony/symfony-dev/vendor/symfony/phpunit-bridge/Legacy/SymfonyTestsListenerTrait.php:229 Spotted in #34925's build. Commits ------- 6b02362 [Phpunit] Fix running skipped tests expecting only deprecations

chalasr

Just rebased and fixed tests. 👍

fabpot · 2020-02-04T05:53:02Z

Thank you @nikophil.

Tobion · 2020-07-20T18:10:32Z

This does not seem to respect the options forwarded to the amqp ext, see #37618

nikophil requested a review from sroze as a code owner December 10, 2019 17:15

carsonbot added Status: Needs Review Feature labels Dec 10, 2019

nicolas-grekas added this to the next milestone Dec 10, 2019

nikophil changed the title ~~Messenger: validate options for AMQP Connection~~ Messenger: validate options for AMQP and Redis Connections Dec 11, 2019

chalasr reviewed Dec 16, 2019

View reviewed changes

src/Symfony/Component/Messenger/Transport/RedisExt/Connection.php Outdated Show resolved Hide resolved

chalasr reviewed Dec 16, 2019

View reviewed changes

src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/AmqpTransportFactoryTest.php Outdated Show resolved Hide resolved

chalasr added the Deprecation label Dec 16, 2019

chalasr previously approved these changes Dec 16, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 16, 2019

chalasr removed the Status: Reviewed label Dec 16, 2019

chalasr added the Messenger label Jan 25, 2020

Nyholm reviewed Jan 26, 2020

View reviewed changes

chalasr mentioned this pull request Jan 28, 2020

[PhpUnitBridge] Fix running skipped tests expecting only deprecations #35489

Merged

Nyholm requested changes Jan 29, 2020

View reviewed changes

carsonbot added the Status: Needs Work label Jan 29, 2020

chalasr approved these changes Feb 3, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Feb 3, 2020

fabpot approved these changes Feb 4, 2020

View reviewed changes

Messenger: validate options for AMQP and Redis Connections

bc4f7d7

fabpot closed this in 981a771 Feb 4, 2020

fabpot mentioned this pull request Feb 4, 2020

[Messenger] Validate options for AMQP and Redis Connections symfony/symfony-docs#13037

Closed

fabpot merged commit bc4f7d7 into symfony:master Feb 4, 2020

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

fabpot mentioned this pull request May 5, 2020

Release v5.1.0-BETA1 #36713

Merged

Tobion mentioned this pull request Jul 20, 2020

[Messenger] "login" falsely reported as invalid option #37618

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Messenger: validate options for AMQP and Redis Connections #34925

Messenger: validate options for AMQP and Redis Connections #34925

nikophil commented Dec 10, 2019 •

edited by chalasr

chalasr commented Dec 16, 2019

nikophil commented Dec 16, 2019

nikophil commented Dec 16, 2019

nikophil commented Jan 4, 2020

chalasr commented Jan 25, 2020

nikophil commented Jan 25, 2020

chalasr commented Jan 25, 2020

Nyholm Jan 26, 2020

Tobion commented Jan 26, 2020

nikophil commented Jan 27, 2020 •

edited

nikophil commented Jan 29, 2020 •

edited

Nyholm commented Jan 29, 2020

nikophil commented Jan 29, 2020

Nyholm commented Jan 29, 2020 •

edited

nikophil commented Jan 29, 2020

chalasr commented Jan 29, 2020

Nyholm commented Jan 29, 2020

nikophil commented Jan 29, 2020

Nyholm commented Jan 29, 2020

nikophil commented Jan 29, 2020

nikophil commented Jan 29, 2020

Nyholm left a comment

Nyholm Jan 29, 2020

nikophil Jan 29, 2020

chalasr commented Jan 29, 2020 •

edited

Nyholm commented Jan 29, 2020

chalasr commented Jan 29, 2020

nikophil commented Jan 29, 2020 •

edited

chalasr left a comment

fabpot commented Feb 4, 2020

Tobion commented Jul 20, 2020

Messenger: validate options for AMQP and Redis Connections #34925

Messenger: validate options for AMQP and Redis Connections #34925

Conversation

nikophil commented Dec 10, 2019 • edited by chalasr

chalasr commented Dec 16, 2019

nikophil commented Dec 16, 2019

nikophil commented Dec 16, 2019

nikophil commented Jan 4, 2020

chalasr commented Jan 25, 2020

nikophil commented Jan 25, 2020

chalasr commented Jan 25, 2020

Nyholm Jan 26, 2020

Choose a reason for hiding this comment

Tobion commented Jan 26, 2020

nikophil commented Jan 27, 2020 • edited

nikophil commented Jan 29, 2020 • edited

Nyholm commented Jan 29, 2020

nikophil commented Jan 29, 2020

Nyholm commented Jan 29, 2020 • edited

nikophil commented Jan 29, 2020

chalasr commented Jan 29, 2020

Nyholm commented Jan 29, 2020

nikophil commented Jan 29, 2020

Nyholm commented Jan 29, 2020

nikophil commented Jan 29, 2020

nikophil commented Jan 29, 2020

Nyholm left a comment

Choose a reason for hiding this comment

Nyholm Jan 29, 2020

Choose a reason for hiding this comment

nikophil Jan 29, 2020

Choose a reason for hiding this comment

chalasr commented Jan 29, 2020 • edited

Nyholm commented Jan 29, 2020

chalasr commented Jan 29, 2020

nikophil commented Jan 29, 2020 • edited

chalasr left a comment

Choose a reason for hiding this comment

fabpot commented Feb 4, 2020

Tobion commented Jul 20, 2020

nikophil commented Dec 10, 2019 •

edited by chalasr

nikophil commented Jan 27, 2020 •

edited

nikophil commented Jan 29, 2020 •

edited

Nyholm commented Jan 29, 2020 •

edited

chalasr commented Jan 29, 2020 •

edited

nikophil commented Jan 29, 2020 •

edited