Skip to content
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.

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

RedisHandler support #262

Closed
wants to merge 5 commits into from
Closed

RedisHandler support #262

wants to merge 5 commits into from

Conversation

LukaszPiechowiak
Copy link

No description provided.

@@ -286,6 +286,13 @@
* - host: server log host. ex: 127.0.0.1:9911
* - [level]: level name or int value, defaults to DEBUG
* - [bubble]: bool, defaults to true
*
* - redis:

Choose a reason for hiding this comment

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

Could you add id here

@@ -699,6 +699,25 @@ private function buildHandler(ContainerBuilder $container, $name, array $handler
));
break;

case 'redis':
if(class_exists('\Redis')){

Choose a reason for hiding this comment

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

could you check for the id-parameter here?

@@ -699,6 +699,25 @@ private function buildHandler(ContainerBuilder $container, $name, array $handler
));
break;

case 'redis':
if(class_exists('\Redis')){
$client = new Definition('\Redis');

Choose a reason for hiding this comment

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

could you combine my pullrequest with yours? Then we have a monologHandler for Redis and Predis that works with services as with just redis-host, port, ... etc...
https://github.com/symfony/monolog-bundle/pull/225/files

Then I can close my pullrequest.

@ruudvdd
Copy link

ruudvdd commented Sep 10, 2018

What needs to be done to get this pr merged?

@jcerrada
Copy link

Hi! Will this be merged in a near future?

@MichielDilissen
Copy link

+1

2 similar comments
@gvanbeck
Copy link

+1

@JurRutten
Copy link

+1

@Seldaek
Copy link
Member

Seldaek commented Dec 26, 2018

I agree that this should be adjusted to also allow passing a service id for the redis/predis client as per @roelmonnens' comments. If an app already has a redis connection open for other purposes it's kinda silly to open a second one just for the logs.

@Seldaek
Copy link
Member

Seldaek commented Dec 26, 2018

Also if any of the +1 club wants to take this PR and do the required changes that'd be very helpful.

@Seldaek Seldaek added this to the 3.4 milestone Dec 26, 2018
@ruudvdd
Copy link

ruudvdd commented Jan 3, 2019

@Seldaek Since I don't know how to update this pr, I have created a new one with a branch created from a more recent version of master. #292

@lyrixx
Copy link
Member

lyrixx commented May 16, 2019

Closing in favor of #292

@lyrixx
Copy link
Member

lyrixx commented May 16, 2019

Thanks @LukaszPiechowiak for the initial work

@lyrixx lyrixx closed this May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants