Skip to content

Amqp helper#90

Merged
Seldaek merged 14 commits intoSeldaek:masterfrom
pomaxa:master
Jun 19, 2012
Merged

Amqp helper#90
Seldaek merged 14 commits intoSeldaek:masterfrom
pomaxa:master

Conversation

@pomaxa
Copy link
Copy Markdown
Contributor

@pomaxa pomaxa commented Jun 12, 2012

adding ability to use rabbitmq as handler;

Comment thread src/Monolog/Handler/AmqpHandler.php Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the visibility is missing, and you need to add an empty line between the attributes and the constructor

@stof
Copy link
Copy Markdown
Contributor

stof commented Jun 12, 2012

And indeed, as mentioned in the commit message, tests are needed before merging

@pomaxa
Copy link
Copy Markdown
Contributor Author

pomaxa commented Jun 13, 2012

@stof thanks for your comments, i've update the code, but got a question about test... to be able to test all functions in this handler, i need to substitute AMQP classes, with mock objects... can you suggest the best way to test it? cuz i don't like to substitute classes in run-time using runkit..

Comment thread src/Monolog/Handler/AmqpHandler.php Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the visibility is still missing

@stof
Copy link
Copy Markdown
Contributor

stof commented Jun 13, 2012

@pomaxa a solution could be to inject the dependencies instead of creating them in the object. This way, you will be able to replace the deps easily.

@andrewtch
Copy link
Copy Markdown
Contributor

@stof , I've fixed and refactored a bit, but AMQP has a bug that segfaults all available to me php versions when trying to mock it, so... there's a bit of hardcode.

Since I've changed a constructor, I strongly advice @pomaxa to give a try to current version before any further review.

Comment thread src/Monolog/Handler/AmqpHandler.php Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typehint the argument instead of doing the check here

@pomaxa
Copy link
Copy Markdown
Contributor Author

pomaxa commented Jun 13, 2012

@andrewtch - thanks, I've just test it and it works.

@stof , i've made changes you suggest

Comment thread src/Monolog/Handler/AmqpHandler.php Outdated
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not too familiar with AMQP, but shouldn't the issuer be the composer channel that's in $record['channel']? I guess it depends if that's just used for information or if it's a machine-level setting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Seldaek It's used for information, just to create a routing key for rabbitmq(or over MQ server, i use it for rabbitmq), to easier manage logging messages. I'll check on $record['channel'], maybe it is better to use it.

@pomaxa
Copy link
Copy Markdown
Contributor Author

pomaxa commented Jun 15, 2012

@Seldaek - thanks, it was my mistake, i forgot about channel param in record;

Comment thread src/Monolog/Handler/AmqpHandler.php Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please remove the @return void as `void`` does not make any sense in PHP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

btw, you could use {@inheritdoc} for this method too

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@return void is commonly understood across multiple languages to mean that you're not returning anything. IMO it should stay.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed to @damianb , http://www.phpdoc.org/docs/latest/for-users/types.html - PHPDocumentor allows null / void @return statement.

But, still, it'll be better to use just @inheritdoc in this particular case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

returning nothing is the same than returning null in PHP. And anyway, Monolog follwo the Symfony2 CS which say to omit the @return tag when returning nothing

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

...and phpdoc's specification has been around far longer than symfony's, and matches that of other, older and more mature languages. return void has its place for understandability.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

5 participants