Skip to content

Conversation

@lyrixx
Copy link
Member

@lyrixx lyrixx commented Sep 8, 2014

Right now, I have the following configuration. And as you can see, the raven.dsn
is useless here, are the client_id service is not buildt by the extension
but by myself.

monolog:
    handlers:
        raven:
            type: raven
            # useless, because we set-up a custom client, but we have to...
            dsn:  "%raven.dsn%"
            client_id: sensiolabs.toolkit.monolog.raven_client

So with with patch, we can use:

monolog:
    handlers:
        raven:
            type: raven
            client_id: sensiolabs.toolkit.monolog.raven_client

BTW, I don't know the target branch of this PR.

@stof
Copy link
Member

stof commented Sep 8, 2014

tests are broken

@stof
Copy link
Member

stof commented Sep 8, 2014

And the right target branch is master. Older branches are not maintained (as the master branch supports Symfony 2.3+)

@lyrixx
Copy link
Member Author

lyrixx commented Sep 9, 2014

And the right target branch is master. Older branches are not maintained (as the master branch supports Symfony 2.3+)

I'm not sure to understand this comment. the target branch is master.

@stof
Copy link
Member

stof commented Sep 9, 2014

I'm not sure to understand this comment. the target branch is master.

It was an answer to your comment BTW, I don't know the target branch of this PR..
It is indeed right currently

@lyrixx
Copy link
Member Author

lyrixx commented Sep 9, 2014

Oh ;) I forgot that ;) Thanks.

@lyrixx lyrixx force-pushed the raven-and-custom-service branch 2 times, most recently from 23ce7bf to 25c8b67 Compare September 9, 2014 09:02
@lyrixx
Copy link
Member Author

lyrixx commented Sep 9, 2014

I fixed tests.

@stof
Copy link
Member

stof commented Sep 9, 2014

@lyrixx
Copy link
Member Author

lyrixx commented Sep 9, 2014

Hm, now I understand why I did that before. (remove the if not service-exist).

  1. If I keep the code like that (like in master), it's not possible to change the client service because the ContainerBuilder in the empty in this method. So the service will never exist. BUT if you register your bundle after MonologBundle, the definition in your bundle will override the definition created by monolog.
  2. If I change the monolog extension, like I did when I opened the PR (I changed the code), This is a very minor BC break, because monolog will no longer create a client definition with the specified service id. Any way, this behavior was wrong and not documented in this way.

So I think, I should update the code to focus on the second point. WDYT?

@lyrixx lyrixx force-pushed the raven-and-custom-service branch from 25c8b67 to 55aaefd Compare September 9, 2014 10:15
@lyrixx
Copy link
Member Author

lyrixx commented Sep 9, 2014

OK, I just implemented the solution N°2

@stof
Copy link
Member

stof commented Sep 9, 2014

Removing the service definition check is indeed the right thing to do. DI extensions cannot check services defined by others. Currently, client_id is unusable.
If you pass a client_id and you don't create the service, you will get an exception later saying the service is missing. It is fine IMO

@lyrixx
Copy link
Member Author

lyrixx commented Sep 9, 2014

So it's a +1 ?

@stof
Copy link
Member

stof commented Sep 13, 2014

Thanks @lyrixx.

@stof stof merged commit 55aaefd into symfony:master Sep 13, 2014
stof added a commit that referenced this pull request Sep 13, 2014
…ixx)

This PR was merged into the 2.6.x-dev branch.

Discussion
----------

Do not require raven.dsn when a client_id is specified

Right now, I have the following configuration. And as you can see, the raven.dsn
is useless here, are the client_id service is not buildt by the extension
but by myself.

```yml
monolog:
    handlers:
        raven:
            type: raven
            # useless, because we set-up a custom client, but we have to...
            dsn:  "%raven.dsn%"
            client_id: sensiolabs.toolkit.monolog.raven_client
```

So with with patch, we can use:

```yml
monolog:
    handlers:
        raven:
            type: raven
            client_id: sensiolabs.toolkit.monolog.raven_client
```

BTW, I don't know the target branch of this PR.

Commits
-------

55aaefd Do not require raven.dsn when a client_id is specified
@lyrixx lyrixx deleted the raven-and-custom-service branch September 14, 2014 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants