swarrot / SwarrotBundle

A symfony bundle for swarrot integration
MIT License
89 stars 59 forks source link

Allow the connection details to be given as a URL #124

Closed K-Phoen closed 6 years ago

K-Phoen commented 6 years ago

Quick attempt at implementing a way to configure the connection with a URL (as described in #50).

I'm not entirely satisfied with my solution as it allows to use simultaneously a URL and a list of attributes…but I'm not sure how I can improve it without BC breaks.

I open this PR early to have feedback :)

odolbeau commented 6 years ago

Hi!

Thanks for your PR.

Do you think it's possible to allow a configuration like this:

connections:
    fqdn: user:password@rabbitmq:5672
    list:
        user: user
        password: password
        host: rabbitmq
        port: 5672

I don't know the Config component enough to know if it's possible and unfortunately I don't have time to test it right now. :/

K-Phoen commented 6 years ago

I could do something like this but it would be a BC break as the configuration format would change.

Today, the each connection described by the user is configured like this, with a list of connection details:

connections:
    joelafrite:
        user: user
        password: password
        host: rabbitmq
        port: 5672

    another_joelafrite:
        user: another_user
        password: another_password
        host: another_rabbitmq
        port: 5672
        vhost: /another_vhost

The idea is to keep this configuration working and provide an alternative way to define the credentials: as a url.

connections:
    joelafrite:
        url: amqp://user:password@rabbit:5672/vhost

    another_joelafrite:
        user: another_user
        password: another_password
        host: another_rabbitmq
        port: 5672
        vhost: /another_vhost

This PR does exactly that. I tested it in one of my projects and it works. The only caveat, as indicated in the description, is that I didn't add any check preventing a user to write something like that:

connections:
    joelafrite:
        url: amqp://user:password@rabbit:5672/vhost
        user: another_user
        password: another_password
        host: another_rabbitmq
        port: 5672
        vhost: /another_vhost

But once we agree on how the URL should appear in the configuration, I can add this kind of check :)

P.S : don't worry if you don't have much time, it's the same for me ;)

olaurendeau commented 6 years ago

Hello @K-Phoen :) Thanks for this appropriate contribution ! I had a quick look and it seems possible to support the format proposed by @odolbeau without BC break : https://stackoverflow.com/questions/15922243/making-configuration-node-support-both-string-and-array-in-symfony-2-configurati

What do you think ?

odolbeau commented 6 years ago

In fact, I'm OK with your PR, I don't see the point of fqdn & list right now!

@olaurendeau what do you think? Does it looks like OK for you like this? Should we also make the old config deprecated? (could be done in another PR btw)

@K-Phoen can you please:

Thanks a lot. :)

odolbeau commented 6 years ago

Fix #50 #131

K-Phoen commented 6 years ago

Woops, I just saw your comment!

I'll update this PR soon (tomorrow or next week) :)

K-Phoen commented 6 years ago

I updated the PR :)

odolbeau commented 6 years ago

Perfect: :+1: