8000 Messenger: validate options for AMQP and Redis Connections by nikophil · Pull Request #34925 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Comments

Messenger: validate options for AMQP and Redis Connections#34925

Merged
fabpot merged 1 commit intosymfony:masterfrom
nikophil:messenger/validate-amqp-options
Feb 4, 2020
Merged

Messenger: validate options for AMQP and Redis Connections#34925
fabpot merged 1 commit intosymfony:masterfrom
nikophil:messenger/validate-amqp-options

Conversation

@nikophil
Copy link
Contributor
@nikophil nikophil commented Dec 10, 2019 •
edited by chalasr
Loading
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

@nikophil nikophil requested a review from sroze as a code owner December 10, 2019 17:15
@nicolas-grekas nicolas-grekas added this to the next milestone Dec 10, 2019
@nikophil nikophil changed the title Messenger: validate options for AMQP Connection Messenger: validate options for AMQP and Redis Connections Dec 11, 2019
chalasr
chalasr reviewed Dec 16, 2019
View reviewed changes
@chalasr
Copy link
Member
chalasr commented Dec 16, 2019

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

@nikophil
Copy link
Contributor Author
nikophil commented Dec 16, 2019

@chalasr done

chalasr
chalasr previously approved these changes Dec 16, 2019
View reviewed changes
@chalasr chalasr dismissed their stale review December 16, 2019 15:27

Tests need to use @expectedDeprecation instead of expectedException

@nikophil
Copy link
Contributor Author
nikophil commented Dec 16, 2019

indeed, i had to update tests

@nikophil
Copy link
Contributor Author
nikophil commented Jan 4, 2020

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
Copy link
Member
chalasr commented 8000 Jan 25, 2020

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

@nikophil
Copy link
Contributor Author
nikophil commented Jan 25, 2020

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
Copy link
Member
chalasr commented Jan 25, 2020

Ok thanks, I'm looking at it.

Nyholm
Nyholm reviewed Jan 26, 2020
View reviewed changes
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated

private static function validateOptions(array $options): void
{
if (0 < \count($invalidOptions = array_diff(array_keys($options), self::AVAILABLE_OPTIONS))) {
Copy link
Member
@Nyholm Nyholm Jan 26, 2020

Choose a reason for hiding this comment

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

Why not use option resolver component?

@Tobion
Copy link
Contributor
Tobion commented Jan 26, 2020

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

@nikophil
Copy link
Contributor Author
nikophil commented Jan 27, 2020 •
edited
Loading

ok, i'm gonna do that 👍

@chalasr chalasr mentioned this pull request Jan 28, 2020
Merged
@nikophil
Copy link
Contributor Author
nikophil commented Jan 29, 2020 •
edited
Loading

@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
Copy link
Member
Nyholm commented Jan 29, 2020

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
Copy link
Contributor Author
nikophil commented Jan 29, 2020

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
Copy link
Member
Nyholm commented Jan 29, 2020 •
edited
Loading

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
Copy link
Contributor Author
nikophil commented Jan 29, 2020

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
Copy link
Member
chalasr commented Jan 29, 2020

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

@Nyholm
Copy link
Member
Nyholm commented Jan 29, 2020

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

@nikophil
Copy link
Contributor Author
nikophil commented Jan 29, 2020

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
Copy link
Member
Nyholm commented Jan 29, 2020

Yeah. =)

Could you rebase the PR?

Most of these files has been moved to Messenger\Bridge