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

Add PHPConsoleHandler to debug in Google Chrome #533

Merged
merged 2 commits into from
Apr 23, 2015
Merged

Add PHPConsoleHandler to debug in Google Chrome #533

merged 2 commits into from
Apr 23, 2015

Conversation

barbushin
Copy link
Contributor

There is a PHPConsoleHandler that integrates Monolog with very popular Google Chrome extension PHP Console and its server library php-console.

There is also PHPConsoleHandlerTest with 25 tests that are passed on PHP 5.3, 5.4 and 5.5.

I keep code style compatible with PSR1/2 and added documentation notes.

Referenced to #416

Integrates Monolog with very popular Google Chrome extension "PHP Console" https://chrome.google.com/webstore/detail/php-console/nfhmhhlpfleoednkpnnnkolmclajemef and server library https://github.com/barbushin/php-console. PHPConsoleHandler and "PHP Console" server library code are 100% covered by tests.
@barbushin
Copy link
Contributor Author

It fails on PHP 7, with error in ruflin/elastica vendor. Seldaek, s there something that I can fix?

Fatal error: "bool" cannot be used as a class name in /home/travis/build/Seldaek/monolog/vendor/ruflin/elastica/lib/Elastica/Document.php on line 7

@xabbuh
Copy link
Contributor

xabbuh commented Mar 30, 2015

@barbushin You don't have to worry about that. The PHP 7 build was already failing before.

@barbushin
Copy link
Contributor Author

@xabbuh I want to make a big announce for ~80k users, that PHP Console is integrated with Monolog. Is it everything okay with this pull request, or I can improve something?

class PHPConsoleHandler extends AbstractProcessingHandler
{

protected $options = array(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this private (as well as the other protected methods). Then, you won't have to keep them for BC when refactoring in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

List of this options will not be changed(removed, renamed). May be new options will be added, but current will be keept as is, because if I change it, then many users configurations becomes failed. @xabbuh please understand it as configuration adapter interface for php-console setup methods, interface declaration will never change.

It must be protected because if somebody extend PHPConsoleHandler, it will defenetly need to access some options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I don't see the reason to change methods from protected to private because:

  1. If users extend PHPConsoleHandler they will be limited to extend protected methods behavior. There is no way to call parent::someMethod() for private methods. So they will just copy/paste the code.
  2. Making methods private does not protect it from refactoring, users can still override it and there is a chance that this methods will not be used after refactoring.
  3. Current protected methods implements very basic features, so I don't see why they should be refactored in future.

Actually I don't like idea to be paranoic and limit users in extending my class. Anyway, sometime they will need to extend something, and if you just do everything as private, they will just override private methods, or just do changes in vendor code. Alternatively, you can just think twice before designing your classes, so there will be no need to do refactoring(removing, renaming methods) in future.

But, anyway, if making this changes is required to accept pull request, then I'll do this. I have no choice. It's up to you to decide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I don't have anything to decide here (it's all up to @Seldaek and he wasn't that strict in the past IIRC :)). I just don't like to make anything changeable by making it protected in advance, but instead wait to see real use-cases and then think how to address them best (most of the times, IMHO inheritance is not the best solution). But if you don't agree, I suggest to leave it as is (because if @Seldaek agrees with you, you wouldn't have done any unnecessary changes then).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is some very easy to understand use case: imagine that you need to add some options that will customize php-console setup. If everything is protected, then users will do something like:

protected function initConnector(Connector $connector = null)
{
    $connector = parent::initConnector($connector);
    if(!empty($this->options['newOption'])) {
        // ....
    }
    return $connector;
}

Otherwise, if everything is private, users will need to override __construct, and it will looks very wierd.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the need. But why should one have to extend class at all? Having a factory method to which one can pass custom methods or a method that makes it possible to modify the options would be more handy I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Connector and all custon configurations must be made in __construct, because otherwise class will become more "heavy" and much more complicated in usage, like this:

$pcHandler = new PHPConsoleHandler($options);
$pcHandler->addOptionHandler('someOption', function($connector, $value) { ... });
$pcHandler->register(); // this method will be added to initialize Connector with new options handlers, and it must be called every time

There will be 5% users that will extend PHPConsoleHandler, and I don't want to ask 95% of other users to call $pcHandler->register() every time. And your issue is just about custom options, but what if user wants to customize behavior of PHPConsoleHandler::handleDebugRecord()? I should add something like $pcHandler->addDebugRecordPreHandler($callback)?

I don't see any problems in class extending - this is the basic OOP feature, and I like to keep balance between user-friendly class interface and customization ability.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather set all the protected stuff private unless they are clear extension points, because otherwise you can't refactor code safely without potential BC breaks.


protected function initConnector(Connector $connector = null)
{
$options =& $this->options;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see why this is assigned by ref here. Seems unnecessary? The whole variable is kinda unnecessary anyway it just shortens the code below in a minor way.

@barbushin
Copy link
Contributor Author

Updated everything as you guys ask. Tests results looks good in Travis CI, all warnings are not related to PHPConsoleHandler.

Now it's okay?

@Seldaek
Copy link
Owner

Seldaek commented Apr 23, 2015

Thanks @barbushin!

Seldaek added a commit that referenced this pull request Apr 23, 2015
Add PHPConsoleHandler to debug in Google Chrome
@Seldaek Seldaek merged commit 6d10786 into Seldaek:master Apr 23, 2015
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