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

Fixed example with priority in service manager. #82

Merged
merged 4 commits into from
Apr 9, 2018

Conversation

Daniel-KM
Copy link
Contributor

The priority in the queue and in the filter are mingled, so this fixes the example and add a remark.

Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

Please remove the entire notes.

Thanks in advance!

@@ -73,6 +82,21 @@ the configuration (`MyLogger`):
$logger = $container->get('MyLogger');
```

Notes:

- The keys of the writers are not required, but they make the merge of the
Copy link
Member

Choose a reason for hiding this comment

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

…they make the merge of the config files easier.

For whom? For the script or user or something else? Very confusing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I remove this part.

'name' => 'stream',
'priority' => Logger::DEBUG,
'priority' => 1,
Copy link
Member

Choose a reason for hiding this comment

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

Please use the correct constant with namespace here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, but it should be indicated that this priority is only a priority in the queue and it is inversed: so "1" (alert, default for the logger) will be triggered after the "7" (debug).

Copy link
Member

@froschdesign froschdesign Jan 9, 2018

Choose a reason for hiding this comment

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

@Daniel-KM
Please do not forget the topic of this chapter "Service Manager Integration". So this is the wrong place to explain this.

Don't get me wrong, this information is important, but not in this chapter of documentation.
If you are looking for this information, would you search in the chapter "Service Manager Integration"? I do not think so.

config files easier. The same remark can be made for the list of processors,
filters, etc.

- The key `priority` next to the key `name` should not be mingled with the
Copy link
Member

Choose a reason for hiding this comment

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

This is already explained in the introduction: https://docs.zendframework.com/zend-log/intro/#using-built-in-priorities

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, this is an application of this definition. And the difference between the priority in the queue and the priority as a severity filter should be explained.

Copy link
Member

Choose a reason for hiding this comment

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

@Daniel-KM
See my comment above.

'options' => [
'stream' => 'php://output',
'formatter' => [
'name' => 'MyFormatter',
],
'filters' => [
[
'name' => 'MyFilter',
'priority' => [
Copy link
Member

Choose a reason for hiding this comment

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

Better than the old version! 👍

'name' => 'stream',
'priority' => Logger::DEBUG,
'priority' => 1,
'options' => [
'stream' => 'php://output',
'formatter' => [
'name' => 'MyFormatter',
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace it with a concrete example like the filters? Thanks!

@Daniel-KM
Copy link
Contributor Author

I added a sample for the short format. In fact, I may be better to write the short format (single value) before the extended one (full array), but it is another subject.

@Daniel-KM
Copy link
Contributor Author

I moved the explanation into the page Writers.

@weierophinney weierophinney dismissed froschdesign’s stale review April 9, 2018 21:23

All feedback incorporated.

@weierophinney weierophinney merged commit 96f4af2 into zendframework:master Apr 9, 2018
weierophinney added a commit that referenced this pull request Apr 9, 2018
Fixed example with priority in service manager.
weierophinney added a commit that referenced this pull request Apr 9, 2018
weierophinney added a commit that referenced this pull request Apr 9, 2018
@weierophinney
Copy link
Member

Thanks, @Daniel-KM!

@Daniel-KM Daniel-KM deleted the fix/priority_example branch April 10, 2018 06:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants