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

Adding monolog as the default logger to OXID #601

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@flow-control
Copy link
Contributor

flow-control commented Sep 28, 2017

There are really awesome libraries for application logging in PHP, and as OXID does not use one of them, i thought it's about time. I stopped reworking everything logging at the OxidEsales\EshopCommunity\Core\Exception Class, as i saw the deprecated notices everywhere. If you like, i can rewrite that part as well and remove the deprecated methods. I think with the upcoming release of 6.0, it is a good point in time to remove the methods marked for deprecation in OXID version 5.3. But this could also be done in another pull request

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Sep 28, 2017

CLA assistant check
All committers have signed the CLA.

@keywan-ghadami-oxid

This comment has been minimized.

Copy link
Contributor

keywan-ghadami-oxid commented Sep 29, 2017

@flow-control I also love to see built a built in logger support. Thank you for your pull requestest. One idea to improve your code: the core Logger class is not needed, it is substituted with monolog anyway, and would need a loggerimpl. to be functional.

@flow-control

This comment has been minimized.

Copy link
Contributor

flow-control commented Sep 29, 2017

I feel the same way about that core logger class, but it's done in a similar way as the database layer. Someone told me that this is done in case you ever ditch doctrine. If we agree on the psr3 interface, than of course this argument back than would be invalid. So you are right, I will remove it. What's your opinion on the exception classes, should I refactor those to use the new logger, or should that be kept open for another PR?

@keywan-ghadami-oxid

This comment has been minimized.

Copy link
Contributor

keywan-ghadami-oxid commented Oct 1, 2017

@flow-control that is very difficult question for me, I did similar pull request in the past with monolog configuration file and configurable exception handler, dependency injection and unittests and it grown to big for me to maintain it over a long a time while core team was refactoring for version 6.
#437 and another back in 2015 (that was also conflicting with namespace impl.)
I still would like to have all the features, but that would also increase the effort and maybe discussion and so the duration until the logger finds it's way to the core.
So my suggestion is for you and the the core team to try a very basic(mvp) in the first step.
But feel free to decide on your self, you can also take code from my pull request into yours (no need to mention my name by doing so).

@flow-control flow-control force-pushed the flow-control:add_monolog branch 2 times, most recently from 99b7b5c to e01f304 Dec 6, 2017

@flow-control

This comment has been minimized.

Copy link
Contributor

flow-control commented Dec 6, 2017

rebased on current master

@proudcommerce

This comment has been minimized.

Copy link

proudcommerce commented Dec 6, 2017

+1

2 similar comments
@mattvb91

This comment has been minimized.

Copy link
Contributor

mattvb91 commented Jan 26, 2018

+1

@tmloberon

This comment has been minimized.

Copy link
Contributor

tmloberon commented Feb 5, 2018

+1

@flow-control

This comment has been minimized.

Copy link
Contributor

flow-control commented Feb 8, 2018

I did a rebase on the current master, any news on this?

@flow-control flow-control force-pushed the flow-control:add_monolog branch 2 times, most recently from 8ef9f9f to e600977 Feb 8, 2018

@flow-control flow-control force-pushed the flow-control:add_monolog branch from e600977 to 2ea5886 Feb 27, 2018

@flow-control

This comment has been minimized.

Copy link
Contributor

flow-control commented Feb 27, 2018

rebase on master, any news on this @Sieg

@SchramlTim

This comment has been minimized.

Copy link

SchramlTim commented Mar 1, 2018

+1

@Sieg

This comment has been minimized.

Copy link
Contributor

Sieg commented Mar 5, 2018

Hi @flow-control, the discussion about your pull request is in progress. What you could do for now - please check the failing test.

@flow-control flow-control force-pushed the flow-control:add_monolog branch 3 times, most recently from 5a2e2e6 to eb89f15 Mar 5, 2018

@flow-control

This comment has been minimized.

Copy link
Contributor

flow-control commented Mar 6, 2018

Hey @Sieg, i fixed the unit test

@mattvb91

This comment has been minimized.

Copy link
Contributor

mattvb91 commented Apr 5, 2018

@Sieg any update on this?

@flow-control flow-control force-pushed the flow-control:add_monolog branch from eb89f15 to 32247bb Apr 6, 2018

@flow-control

This comment has been minimized.

Copy link
Contributor

flow-control commented Apr 6, 2018

did a rebase on current master

@Sieg

This comment has been minimized.

Copy link
Contributor

Sieg commented Apr 11, 2018

Hello guys, thanks for the updates. Core team has a story for integrating the PSR logger, and the pull request is linked there.

iegupov added a commit that referenced this pull request Apr 17, 2018

@gregorhyneck

This comment has been minimized.

Copy link
Contributor

gregorhyneck commented Apr 18, 2018

As already mentioned by @Sieg, the core team will merge your pull request soon. Thank you for that! We decided to have some things on top (everyting fine with your pull request):

  • log level configurable
  • wrapper for monolog in order to use only the methods defined in Psr\Log\LoggerInterface

The core team will implement those changes soon.

gregorhyneck added a commit that referenced this pull request Apr 25, 2018

robertblank added a commit that referenced this pull request May 7, 2018

robertblank added a commit that referenced this pull request May 7, 2018

iegupov added a commit that referenced this pull request May 11, 2018

robertblank added a commit that referenced this pull request May 15, 2018

robertblank added a commit that referenced this pull request May 17, 2018

iegupov added a commit that referenced this pull request May 17, 2018

iegupov added a commit that referenced this pull request May 17, 2018

iegupov added a commit that referenced this pull request May 17, 2018

iegupov added a commit that referenced this pull request May 18, 2018

@Sieg

This comment has been minimized.

Copy link
Contributor

Sieg commented May 31, 2018

Hello guys! Thanks for the contribution. As you have probably already noticed, the core team have implemented some more changes on the top of this pull request, and it is merged to b-6.x and master recently!

@Sieg Sieg closed this May 31, 2018

@Sieg Sieg self-assigned this May 31, 2018

@flow-control flow-control deleted the flow-control:add_monolog branch Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment