Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Allow setting formatter for Zend\Log\Writer\Db via config options #5882

Closed
wants to merge 5 commits into from

Conversation

Martin-P
Copy link
Contributor

This PR fixes issue #5881 and makes it possible to set a formatter in Zend\Log\Writer\Db via the config options.

The last line in Zend\Log\Writer\Db::__construct() (line 90) always sets the formatter like this:

$this->setFormatter(new DbFormatter());

This PR has an additional check to see if there already is a formatter set which is provided via the config options. If there already is a formatter, that formatter has precedence over the default formatter. If no formatter is provided via the config options the default formatter Zend\Log\Formatter\Db (DbFormatter in the code above) will be used.

@weierophinney
Copy link
Member

Only issue I see with this PR is it is adding additional methods that are not defined in WriterInterface. Considering that the formatter is primarily an internal detail, I think the functionality of getFormatter and hasFormatter should be made protected visibility.

This will primarily have an impact on the test cases, as you'll need to use reflection to alter the visibility for purposes of the tests; alternately, we can rely on known behaviors to verify that the correct functionality is being invoked.

@weierophinney weierophinney added this to the 2.3.0 milestone Mar 3, 2014
@Martin-P
Copy link
Contributor Author

Martin-P commented Mar 4, 2014

I made the methods getFormatter and hasFormatter public on purpose to increase usablity. While debugging I missed the functionality to see if a formatter was set. Also the method setFormatter is public as well. I can alter the visibility and update the tests, but I hope my motivation is convincing enough to keep them public.

@weierophinney
Copy link
Member

@Martin-P The problem, though, is that (a) the methods are only visible in this single adapter, and (b) we cannot add methods to existing interfaces without breaking backwards compatibility.

I'll change the visibility during merge, and update the tests.

weierophinney added a commit that referenced this pull request Mar 5, 2014
Allow setting formatter for Zend\Log\Writer\Db via config options
weierophinney added a commit that referenced this pull request Mar 5, 2014
- `(get|has)Formatter()` are not part of the `WriterInterface`, and are
  primarily internal implementation details. Made them protected, and
  adjusted tests to use reflection to make them accessible in order
  to test internal state.
weierophinney added a commit that referenced this pull request Mar 5, 2014
weierophinney added a commit that referenced this pull request Mar 5, 2014
@weierophinney weierophinney self-assigned this Mar 5, 2014
@weierophinney
Copy link
Member

Merged to develop for release with 2.3.0.

@Martin-P
Copy link
Contributor Author

Martin-P commented Mar 5, 2014

The methods getWriter and hasWriter are declared in AbstractWriter. All other writers extend AbstractWriter so that shouldn't be a problem? I do see how altering the interface is a BC break. I didn't think of that.

@Martin-P Martin-P deleted the issue-5881 branch August 13, 2014 14:08
weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015
…ue-5881

Allow setting formatter for Zend\Log\Writer\Db via config options
weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015
- `(get|has)Formatter()` are not part of the `WriterInterface`, and are
  primarily internal implementation details. Made them protected, and
  adjusted tests to use reflection to make them accessible in order
  to test internal state.
weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants