Can you please add an entry toUPGRADE-5.1.md andUPGRADE-6.0.md?

@chalasr done

indeed, i had to update tests

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

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

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

Ok thanks, I'm looking at it.

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

ok, i'm gonna do that 👍

@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 atthis resolved comment). ButOptionResolver::resovle() is strict and only throws exception.

I wanted to replace myvalidateOptions() method by something like:

$resolver =newOptionsResolver();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?

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.

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

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 cInput options: a fDeprecated: 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)

haha, this seems terribly hacky!

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

i'll follow your decision ;)

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

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

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

Yeah. =)

Could you rebase the PR?

Most of these files has been moved to Messenger\Bridge

yep i was doing that right now ;)

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

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

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.

@Nyholm Right, thank you.

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::testDeprecationIfInvalidOptionIsPassedWithDsnFailed 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

Thank you@nikophil.

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