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

Full Compatibility with Monolog 3.6 #17

Closed
wants to merge 9 commits into from

Conversation

hirasso
Copy link

@hirasso hirasso commented Apr 17, 2024

Warning: This PR is huge.

I'm opening it here to let you have a look if you are interested. If it's just too many changes for your taste, I'll continue to maintain my fork for my use.

Questions:

  • There where a lot of weird comments that I couldn't make sense of, for example `//. Removed all of them from the code as they seemed IDE-specific to me
  • If you decide to continue maintaining the package, we should think about a github action to run the tests in PRs. I could help out there as I'm a maintainer of another rather large open source repo.

@markheydon markheydon self-assigned this Apr 18, 2024
@markheydon
Copy link
Member

Thanks for this @hirasso, very true I've struggled to get to updating it to latest versions of stuff so your efforts are appreciated!

Not sure what you mean on the 'weird comments', I couldn't see anything in the changes that take out anything that looked odd. That said, I did at some point use IDEs other than VS Code, in particular I remember one of them (PHP Storm maybe?) that used to do something odd with comments.

You mention Actions too, there's some already, aren't they doing what you suggest? Or did I miss something?

@markheydon markheydon added the code improvement Improvement to the code label Apr 18, 2024
@hirasso
Copy link
Author

hirasso commented Apr 18, 2024

Hey @markheydon! Great to know you're open to my contribution! I saw the action notification about the failing tests. Could we update the PHP version in the action to 8.2? Since Monolog ^3 requires at least PHP 8.1, I figure most users of this package likely keep their PHP updated.

Regarding the code comments I deleted, I also believe they're related to PHP Storm. One example:

//<editor-fold desc="Magic Methods">

Also, it's possible the code coverage dropped below 100% in my PR since I'm not familiar with coverage tools. However, I prioritize writing tests for critical components, so I don't think it's a big issue. I prefer keeping my repos free of coverage report batches.

@hirasso
Copy link
Author

hirasso commented May 8, 2024

I've ditched the idea of using WP_CLI together with Monolog logs, so I'll close this PR. Turns out there is a much simpler solution with simply using php://output. That also makes it pretty straightforward to dump the context of a LogRecord using Symphony's VarDumper package. I've posted my new solution over at StackOverflow: https://stackoverflow.com/a/78447765/586823

@hirasso hirasso closed this May 8, 2024
@markheydon
Copy link
Member

Thanks for the update @hirasso, was good to have some interest in the project while it lasted though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code improvement Improvement to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$logger->notice() should be resolved to WP_CLI::log instead of WP_CLI::warning
2 participants