Skip to content

Commit

Permalink
Refactoring regarding Monolog authors requirements
Browse files Browse the repository at this point in the history
  • Loading branch information
@barbushin
barbushin committed Apr 18, 2015
1 parent cd90150 commit 1c20abf
Showing 1 changed file with 33 additions and 35 deletions.
68 changes: 33 additions & 35 deletions src/Monolog/Handler/PHPConsoleHandler.php
View file
Expand Up @@ -40,7 +40,7 @@
class PHPConsoleHandler extends AbstractProcessingHandler
{

protected $options = array(
private $options = array(
'enabled' => true, // bool Is PHP Console server enabled
'classesPartialsTraceIgnore' => array('Monolog\\'), // array Hide calls of classes started with...
'debugTagsKeysInContext' => array(0, 'tag'), // bool Is PHP Console server enabled
Expand All @@ -64,7 +64,7 @@ class PHPConsoleHandler extends AbstractProcessingHandler
);

/** @var Connector */
protected $connector;
private $connector;

/**
* @param array $options See \Monolog\Handler\PHPConsoleHandler::$options for more details
Expand All @@ -83,7 +83,7 @@ public function __construct(array $options = array(), Connector $connector = nul
$this->connector = $this->initConnector($connector);
}

protected function initOptions(array $options)
private function initOptions(array $options)
{
$wrongOptions = array_diff(array_keys($options), array_keys($this->options));
if ($wrongOptions) {
Expand All @@ -93,56 +93,54 @@ protected function initOptions(array $options)
return array_replace($this->options, $options);
}

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

if (!$connector) {
if ($options['dataStorage']) {
Connector::setPostponeStorage($options['dataStorage']);
if ($this->options['dataStorage']) {
Connector::setPostponeStorage($this->options['dataStorage']);
}
$connector = Connector::getInstance();
}

if ($options['registerHelper'] && !Helper::isRegistered()) {
if ($this->options['registerHelper'] && !Helper::isRegistered()) {
Helper::register();
}

if ($options['enabled'] && $connector->isActiveClient()) {
if ($options['useOwnErrorsHandler'] || $options['useOwnExceptionsHandler']) {
if ($this->options['enabled'] && $connector->isActiveClient()) {
if ($this->options['useOwnErrorsHandler'] || $this->options['useOwnExceptionsHandler']) {
$handler = Handler::getInstance();
$handler->setHandleErrors($options['useOwnErrorsHandler']);
$handler->setHandleExceptions($options['useOwnExceptionsHandler']);
$handler->setHandleErrors($this->options['useOwnErrorsHandler']);
$handler->setHandleExceptions($this->options['useOwnExceptionsHandler']);
$handler->start();
}
if ($options['sourcesBasePath']) {
$connector->setSourcesBasePath($options['sourcesBasePath']);
if ($this->options['sourcesBasePath']) {
$connector->setSourcesBasePath($this->options['sourcesBasePath']);
}
if ($options['serverEncoding']) {
$connector->setServerEncoding($options['serverEncoding']);
if ($this->options['serverEncoding']) {
$connector->setServerEncoding($this->options['serverEncoding']);
}
if ($options['password']) {
$connector->setPassword($options['password']);
if ($this->options['password']) {
$connector->setPassword($this->options['password']);
}
if ($options['enableSslOnlyMode']) {
if ($this->options['enableSslOnlyMode']) {
$connector->enableSslOnlyMode();
}
if ($options['ipMasks']) {
$connector->setAllowedIpMasks($options['ipMasks']);
if ($this->options['ipMasks']) {
$connector->setAllowedIpMasks($this->options['ipMasks']);
}
if ($options['headersLimit']) {
$connector->setHeadersLimit($options['headersLimit']);
if ($this->options['headersLimit']) {
$connector->setHeadersLimit($this->options['headersLimit']);
}
if ($options['detectDumpTraceAndSource']) {
if ($this->options['detectDumpTraceAndSource']) {
$connector->getDebugDispatcher()->detectTraceAndSource = true;
}
$dumper = $connector->getDumper();
$dumper->levelLimit = $options['dumperLevelLimit'];
$dumper->itemsCountLimit = $options['dumperItemsCountLimit'];
$dumper->itemSizeLimit = $options['dumperItemSizeLimit'];
$dumper->dumpSizeLimit = $options['dumperDumpSizeLimit'];
$dumper->detectCallbacks = $options['dumperDetectCallbacks'];
if ($options['enableEvalListener']) {
$dumper->levelLimit = $this->options['dumperLevelLimit'];
$dumper->itemsCountLimit = $this->options['dumperItemsCountLimit'];
$dumper->itemSizeLimit = $this->options['dumperItemSizeLimit'];
$dumper->dumpSizeLimit = $this->options['dumperDumpSizeLimit'];
$dumper->detectCallbacks = $this->options['dumperDetectCallbacks'];
if ($this->options['enableEvalListener']) {
$connector->startEvalRequestsListener();
}
}
Expand Down Expand Up @@ -186,7 +184,7 @@ protected function write(array $record)
}
}

protected function handleDebugRecord(array $record)
private function handleDebugRecord(array $record)
{
$tags = $this->getRecordTags($record);
$message = $record['message'];
Expand All @@ -196,18 +194,18 @@ protected function handleDebugRecord(array $record)
$this->connector->getDebugDispatcher()->dispatchDebug($message, $tags, $this->options['classesPartialsTraceIgnore']);
}

protected function handleExceptionRecord(array $record)
private function handleExceptionRecord(array $record)
{
$this->connector->getErrorsDispatcher()->dispatchException($record['context']['exception']);
}

protected function handleErrorRecord(array $record)
private function handleErrorRecord(array $record)
{
$context = $record['context'];
$this->connector->getErrorsDispatcher()->dispatchError($context['code'], $context['message'], $context['file'], $context['line'], $this->options['classesPartialsTraceIgnore']);
}

protected function getRecordTags(array &$record)
private function getRecordTags(array &$record)
{
$tags = null;
if (!empty($record['context'])) {
Expand Down

14 comments on commit 1c20abf

@Xesenix
Copy link

Choose a reason for hiding this comment

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

WHY private methods now i need to rewrite this whole class to make fixes?

@stof
Copy link
Contributor

@stof stof commented on 1c20abf Feb 13, 2017

Choose a reason for hiding this comment

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

@Xesenix making things protected means we have to keep BC on these methods for people extending the class, which adds a big maintenance overhead (and can sometimes forbid doing fixes because it would require a BC break).
If you have a use case requiring an extension point, please open an issue describing your use case, so that maintainers can decide about the best way to provide such an extension point. Opening everything to inheritance is generally the worse extension point as far as maintainability is concerned.

@Xesenix
Copy link

Choose a reason for hiding this comment

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

I have made pull request with update to php console handler that fixes browsing logged data in chrome.
I wanted extend this class to fix one method but private methods required me to override additional two other methods. I have a little different opinion about responsibility about BC for classes that are extended (if you extend library class you should check if it breaks on update)

@barbushin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Xesenix I'm author of PHP Console extension, and I want say that I 100% agree with you. I changed methods to private because it was requirement of Monolog authors to merge my PR.

@stof Using private everywhere is good for vendors but extremely bad for users. You can make method final if you're worried that it will be overridden, but it will be still accessible in child classes.

IMHO, it's always users responsibility to test BC issues of overridden vendors after doing composer update.

OOP is not about using private everywhere, it's about being enough smart to find the balance.

@stof
Copy link
Contributor

@stof stof commented on 1c20abf Feb 13, 2017 •
edited

Choose a reason for hiding this comment

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

@barbushin making all methods protected final would not have helped @Xesenix, as he wanted to replace one of them

@Xesenix
Copy link

Choose a reason for hiding this comment

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

@stof it would help me becouse i would replace one that i wanted to fix and did not have to touch anything else because they would be inherited from parent class. And when there are private methods used by parent class that i need in child class i have to write them again in child class.

@stof
Copy link
Contributor

@stof stof commented on 1c20abf Feb 13, 2017

Choose a reason for hiding this comment

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

@Xesenix but if everything is final, you cannot replace them. This is the whole point of being final.

@Xesenix
Copy link

Choose a reason for hiding this comment

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

@stof ok my wrong im getting tired and read that as private instead of final

@barbushin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stof I'm not saying about making all methods final :D There must be the balance. But it looks like you guys just don't understand the difference of using protected/private/final. You just making everything private. Genius :)

@Xesenix
Copy link

Choose a reason for hiding this comment

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

Library should not restrict users ability to extend it. When users extends it, it is their responsibility to keep their code working.

@stof
Copy link
Contributor

@stof stof commented on 1c20abf Feb 13, 2017

Choose a reason for hiding this comment

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

@Xesenix with such a policy, users would be scared to upgrade your library, because any update could break their code. As a package maintainer, I hate people staying on old releases, because they then complain about bugs fixed ages ago (and they generally don't even tell us they use an outdated version).
Considering extension points as part of the API covered by semantic versionning makes the life of users much easier when dealing with upgrades. But to keep the maintenance possible, it then means designing extension points carefully.
And as said before, inheritance-based extension points are the worse ones regarding writing BC layers:

  • changing the arguments of a protected method breaks BC
  • changing the expected return value of a protected method breaks BC
  • stopping usage of the protected method (for instance because the logic got inlined, or moved to a different protected method due to previous points) is a BC break (as the modified behavior is not used anymore)
  • changing the cases where you call a protected method is a BC break (previous point is really just a special case of this one)

Opening everything to replacement by inheritance is not the point of OOP.

And I agree that a balance is a good thing. this is precisely why I asked you to open an issue describing your use case for an extension point, so that it can be opened.
But the maintenance work requires opening little by little to reach the appropriate balance, not opening everything from the start (because once it is opened, you cannot close it anymore until the next major version. And you probably don't even get any feedback about which part are actually useful and should stay open)

@barbushin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Soft

As a package maintainer, I hate people staying on old releases

And people hates when something is wrong with your package, and they need to extend it, but they can't. And there're many people, and you're the only one. And people can decide to stay on old version, or fix their package extension to BC with new version.

Users are not so stupid as you think. They can do everything on their own risk.

Not enough?

Check this out:

Protected methods in Laravel framework: 412
https://github.com/laravel/framework/search?utf8=%E2%9C%93&q=%22protected+function%22&type=Code

Private methods in Laravel framework: 7
https://github.com/laravel/framework/search?utf8=%E2%9C%93&q=%22private+function%22&type=Code

@stof
Copy link
Contributor

@stof stof commented on 1c20abf Feb 13, 2017

Choose a reason for hiding this comment

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

@barbushin do the same check on Symfony. you will find the opposite 😄

and Laravel is not the good example for my point: they explicitly don't advocate BC between minor versions and recommend against using semver constraints because of that (even when they write their dependency against libraries which do follow semver btw).

And there're many people, and you're the only one. And people can decide to stay on old version, or fix their package extension to BC with new version.

Staying on the old version implies using exact version constraints in the composer era (and depending directly on the package for that, instead of relying on your framework bringing in monolog btw, as the framework is likely to allow a version range).
And doing things "on their own risk" is precisely what leads people to not upgrade the package, which again hurts the package maintenance.

And again, if you have a valid use case for an extension point, there is nothing wrong with asking for it.
If it is just to fix a bug, providing a fix in the library itself is better for the community than using a child class in your project and keeping the code buggy for everyone else. A bugfix is not a justification for opening the class for inheritance.
Anyway, I will stop arguing here. There are different maintainer policy, and Monolog is on the side of following semver strictly (which is quite expected given that it is from the creator of composer...), which include extension points.

@Xesenix
Copy link

Choose a reason for hiding this comment

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

@stof you have 100% right about one thing if there waren't any private methods you wouldn't get feedback where they are annoying :)

lets make little analyze of methods and properties in this class that got private in this change:

options - if any class extends PHPConsoleHandler they wont be able to get anything set in options without overriding constructor and storing them in some other field for options for derived class.

connector - its ok i can get to it by using getConnector()

initOptions - its ok small chance that anyone would ever need to touch this

initConnector - its little inconvenient but i still can modify or use connector via getConnector()

handleDebugRecord - if i override this method it wouldnt be called in write method so i have to also override write method (this is my first problem) and when i override it i dont have access to getRecordTags so i have to recreate it in derived class (that is my second problem)

handleExceptionRecord and handleErrorRecord - those both have first problem from handleDebugRecord

PS Symfony is better example but I have checked privates are in minority and almost always they dont do any advanced logic (usually they call other methods or are empty so easy to recreate and small chance on cascading method recreation like in above example).

Please sign in to comment.