Skip to content

[MonologBridge] Added WebSubscriberProcessor to ease processor configuration #26832

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

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Apr 6, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Side note: I could be nice to have an interface to act as the marker to AutoConfigure processors

@lyrixx
Copy link
Member Author

lyrixx commented Apr 9, 2018

Actually, I don't need to add a new class to preserve the BC, right ?

4.1.0
-----

* Added `WebSubscriberProcessor` to be easily autoconfigured
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the meaning of this sentence. What is easily configured?

}

/**
* WebProcessor override to read from the HttpFoundation's Request and to be autoconfigured.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but I don't understand what that means as well (I see that you copy/pasted the one from WebProcessor, but the second part is quite cryptic to me).

@fabpot fabpot modified the milestones: 4.1, next Apr 22, 2018
@lyrixx
Copy link
Member Author

lyrixx commented Apr 23, 2018

I have update the PR:

  • I removed the new class
  • WebProcessor now implements EventSubscriberInterface in order to be easily autoconfigured

Before:

    Symfony\Bridge\Monolog\Processor\WebProcessor:
        tags:
            - { name: monolog.processor }
            - { name: kernel.event_listener, event: kernel.request, priority: 4096 }

After:

    Symfony\Bridge\Monolog\Processor\WebProcessor:
        tags: [ monolog.processor ]

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

minor CS comment

@lyrixx
Copy link
Member Author

lyrixx commented Apr 23, 2018

minor CS comment

@fabpot I don't see your comments :/

4.1.0
-----

* `WebProcessor` now implements EventSubscriberInterface in order to be easily autoconfigured
Copy link
Member

Choose a reason for hiding this comment

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

missing quotes around EventSubscriberInterface

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@fabpot
Copy link
Member

fabpot commented Apr 23, 2018

Thank you @lyrixx.

@fabpot fabpot merged commit b1287d6 into symfony:master Apr 23, 2018
fabpot added a commit that referenced this pull request Apr 23, 2018
…rocessor configuration (lyrixx)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[MonologBridge] Added WebSubscriberProcessor to ease processor configuration

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

---

Side note: I could be nice to have an interface to act as the marker to AutoConfigure processors

Commits
-------

b1287d6 [MonologBridge] Added WebSubscriberProcessor to ease processor configuration
@lyrixx lyrixx deleted the monolog-log-web branch April 23, 2018 12:19
@fabpot fabpot mentioned this pull request May 7, 2018
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.1 Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants