10000 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

Messenger: validate options for AMQP and Redis Connections #34925

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 4, 2020
Merged

Messenger: validate options for AMQP and Redis Connections #34925

merged 1 commit into from
Feb 4, 2020

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

@chalasr done
8000

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

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 Jan 25, 2020

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

@nikophil
Copy link
Contributor Author

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
@@ -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))) {
Copy link
Member

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

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

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

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

@nikophil
Copy link
Contributor Author

yep i was doing that right now ;)

@nikophil
Copy link
Contributor Author

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

Nyholm
Nyholm requested changes Jan 29, 2020
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Just a small thing

src/Symfony/Component/Messenger/CHANGELOG.md Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@chalasr
Copy link
Member
chalasr commented Jan 29, 2020 •
edited
Loading

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

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

@Nyholm Right, thank you.

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

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

nicolas-grekas added a commit that referenced this pull request Feb 3, 2020
@nicolas-grekas
bug #35489 [PhpUnitBridge] Fix running skipped tests expecting only d…
1f053f9 
…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
chalasr approved these changes Feb 3, 2020
View reviewed changes
Copy link
Member
@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Just rebased and fixed tests. 👍

@fabpot
Copy link
Member
fabpot commented Feb 4, 2020

Thank you @nikophil.

@fabpot fabpot closed this in 981a771 Feb 4, 2020
@fabpot fabpot mentioned this pull request Feb 4, 2020
Closed
@fabpot fabpot merged commit bc4f7d7 into symfony:master Feb 4, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
@Tobion Tobion mentioned this pull request Jul 20, 2020
Closed
@Tobion
Copy link
Contributor
Tobion commented Jul 20, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weaverryan weaverryan weaverryan left review comments

@Nyholm Nyholm Nyholm requested changes

@fabpot fabpot fabpot approved these changes

@chalasr chalasr chalasr approved these changes

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Deprecation Feature Messenger Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
8 participants
@nikophil @chalasr @Tobion @Nyholm @fabpot @weaverryan @nicolas-grekas @carsonbot
0