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

Invalid StreamHandler Arguments #422

Merged
merged 3 commits into from Sep 29, 2014

Conversation

3 participants
@TheoKouzelis
Copy link

TheoKouzelis commented Sep 17, 2014

My application was incorrectly passing the StreamHandler a argument of

$stream = array('file://url');
$handler = new StreamHandler($stream);

And when the stream handler tried to handle a record it would cause the following error

Array to string conversion at /var/www/service-manager/vendor/monolog/monolog/src/Monolog/Handler/StreamHandler.php:82

I thought by adding an InvalidArgumentException it might show people where they are going wrong quicker

* @param Boolean $bubble Whether the messages that are handled can bubble up the stack or not
* @param int|null $filePermission Optional file permissions (default (0644) are only for owner read/write)
* @param Boolean $useLocking Try to lock log file before doing any writes
* @param Resource|string $stream

This comment has been minimized.

@stof

stof Sep 19, 2014

Contributor

resource should be lowercased.

public function testWriteInvalidArgument($invalidArgument)
{
$handler = new StreamHandler($invalidArgument);
$handler->handle($this->getRecord());

This comment has been minimized.

@stof

stof Sep 19, 2014

Contributor

handle is useless as it will not be reached. The exception is in the constructor

* @dataProvider invalidArgumentProvider
* @expectedException InvalidArgumentException
* @covers Monolog\Handler\StreamHandler::__construct
* @covers Monolog\Handler\StreamHandler::write

This comment has been minimized.

@stof

stof Sep 19, 2014

Contributor

it will not cover write either

* @param Boolean $bubble Whether the messages that are handled can bubble up the stack or not
* @param int|null $filePermission Optional file permissions (default (0644) are only for owner read/write)
* @param Boolean $useLocking Try to lock log file before doing any writes
* @throws InvalidArgumentException If stream is not a resource or string

This comment has been minimized.

@stof

stof Sep 19, 2014

Contributor

I suggest adding an empty line between the @param and @throws tags:

  • it will improve readability
  • it will make alignment look better (the PHP-CS-Fixer will actually align If stream is not a resource or string with $useLocking, not with Try to lock log file before doing any writes as it is in the third segment, while it won't align lines together if they are separated in diffeent blocks)
@TheoKouzelis

This comment has been minimized.

Copy link

TheoKouzelis commented Sep 20, 2014

Thanks for the corrections, hope this sorts it.

@Seldaek

This comment has been minimized.

Copy link
Owner

Seldaek commented Sep 29, 2014

Thanks!

Seldaek added a commit that referenced this pull request Sep 29, 2014

Merge pull request #422 from TheoKouzelis/issue/streamArgs
Invalid StreamHandler Arguments

@Seldaek Seldaek merged commit c2069d7 into Seldaek:master Sep 29, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment