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

Supporting v2 handlers and removing removed. #321

Merged
merged 8 commits into from Nov 13, 2019

Conversation

inverse
Copy link
Contributor

@inverse inverse commented Aug 31, 2019

This would allow the wiring up of v2 handlers and preventing wiring of removed on V2 API.

Wanted to get feedback if this approach is suitable before I continue with adding tests to cover the validation aspect.

If you like the approach this is taking do you have any guidelines around testing against multiple versions of monolog? e.g. something like this https://github.com/g1a/composer-test-scenarios

Release notes: https://github.com/Seldaek/monolog/releases/tag/2.0.0

Copy link

@gmponos gmponos left a comment

Choose a reason for hiding this comment

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

Maybe a deprecation could also be raised in cases where someone is still using v1 and a handler that is removed on v2

WDYT?

@Seldaek Seldaek requested a review from lyrixx September 2, 2019 10:22
@Seldaek Seldaek added this to the 3.5 milestone Sep 2, 2019
@inverse
Copy link
Contributor Author

inverse commented Sep 2, 2019

Thanks @gmponos - added that message. What do you think?

any experience with multiple version testing? Is that covered by the definitions within travis?

@gmponos
Copy link

gmponos commented Sep 3, 2019

Hello @inverse

any experience with multiple version testing? Is that covered by the definitions within travis?

To be honest no. I don't have that much.

WDYT?

What do you think?

Thank you for taking into account my feedback.. I truly wanted also the your opinion (and from the maintainers.. ) if it makes sense to have a deprecation on the configuration level.. It might be overhead since a deprecation will be triggered when the handlers will be initialized:

https://github.com/Seldaek/monolog/blob/1.x/src/Monolog/Handler/HipChatHandler.php#L100

I think that symfony configuration has a special option that you can set a config option as deprecated.. not sure if raises also a deprecation notice and if it would make sense to have it here as well..

Last sorry if I caused any trouble...

@inverse
Copy link
Contributor Author

inverse commented Sep 3, 2019

@gmponos Ahh you're right - it probably makes sense to allow the package not the bundle to flag such deprecation.

As for the Symfony one - looks like that's more for configuration keys which in this case is a string type which I don't think makes sense.

See: https://symfony.com/blog/new-in-symfony-3-4-deprecate-configuration-options

@inverse
Copy link
Contributor Author

inverse commented Sep 24, 2019

@lyrixx any input on this direction?

@fabpot fabpot merged commit 89998b5 into symfony:master Nov 13, 2019
@fabpot
Copy link
Member

fabpot commented Nov 13, 2019

Thank you.

@inverse inverse deleted the v2-wireup branch November 13, 2019 19:43
@jderusse jderusse mentioned this pull request Feb 15, 2021
jderusse added a commit that referenced this pull request Mar 21, 2021
This PR was merged into the 3.x-dev branch.

Discussion
----------

Remove invalid handlers

This PR removes the invalid handlers added in #321: Adding an handler is not only about adding the mapping `shortName` <=> `class`: The constructor's arguments have to be defined in the `buildHandler` function. Otherwise, we get the following exception:

```
InvalidArgumentException: Invalid handler type "logmatic" given for handler "logmatic"

/data/oss/monolog-bundle/DependencyInjection/MonologExtension.php:911
```

This PR adds support for handler "noop".

Re-adding supports for the other (more complex) handlers could be done in dedicated PR.

Commits
-------

44799b3 Remove invalid handlers
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.

None yet

5 participants