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.

Sign up for GitHub

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

Jump to bottom

feat: Add support for Monolog 3.x #5334

Merged
merged 10 commits into from
Dec 14, 2022
Merged

feat: Add support for Monolog 3.x #5334

merged 10 commits into from
Dec 14, 2022

Conversation

jeromegamez
Copy link
Contributor

@jeromegamez jeromegamez commented Jun 10, 2022 •
edited
Loading

Hey everyone 👋,

this is a follow-up to #2302 and aims to add support for all currently available monolog/monolog versions.

  • AppEngineFlexHandlerV3 and AppEngineFlexFormatterV3 are the new implementations for Monolog 3.x.
  • The formatPayload method in the FormatterTrait should support arrays and the new LogRecord class

Prerequisites for this PR to be mergeable

Things to look into

  • The ReflectionHandlerV5 class is a 90% copy of the ReflectionHandlerV4 - perhaps this can be deduplicated

Notes

  • There are deprecation warnings that I was not yet able to tackle yet:
    • opis/closure

      Deprecated: Opis\Closure\SerializableClosure implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary) in /Users/jg/Code/opensource/google-cloud-php/vendor/opis/closure/src/SerializableClosure.php on line 18

:octocat:

@jeromegamez jeromegamez requested review from a team as code owners June 10, 2022 14:36
@jeromegamez
Copy link
Contributor Author

jeromegamez commented Jun 10, 2022 •
edited
Loading

I'll ignore the conventionalcommits.org tests for now, but it's good to see that the "Generate Documentation" job fails, this shows that https://github.com/googleapis/google-cloud-php/blob/main/Core/src/Testing/Reflection/ReflectionHandlerFactory.php needs updating to support phpdocumentor/reflection ^5

I caved so that there's one less failing job 🤷😅

@jeromegamez jeromegamez mentioned this pull request Jun 10, 2022
Merged
@jeromegamez
Copy link
Contributor Author

The reflection handlers should probably be refactored to avoid much-duplicated code, I will get to that once I'm back at my desk in a few hours

@jeromegamez jeromegamez force-pushed the monolog-3 branch 2 times, most recently from 4678789 to c15a529 Compare June 10, 2022 20:38
@jeromegamez jeromegamez changed the title Add support for Monolog 3.x feat: Add support for Monolog 3.x Jun 10, 2022
@jeromegamez
Copy link
Contributor Author

Alright, this is a rabbit hole - when I fix one problem, another one pops up, that's why I'm gradually working my way through the classes and force pushing all the time.

The good news is that we'll probably have PHP 8.1 compatibility when we're through (if that's even possible)

@jeromegamez jeromegamez mentioned this pull request Jun 12, 2022
Merged
@jeromegamez jeromegamez requested review from a team as code owners June 12, 2022 22:04
@jeromegamez jeromegamez force-pushed the monolog-3 branch 5 times, most recently from 3111268 to 04033a9 Compare June 12, 2022 23:33
@jeromegamez jeromegamez requested a review from a team as a code owner June 12, 2022 23:33
@jeromegamez
Copy link
Contributor Author

jeromegamez commented Jun 12, 2022 •
edited
Loading

Update: Sorry for all the force pushes - I needed to make a bunch of preparations in order to be able to run the tests without throwing errors with PHP 8.1 before adding Monolog 3 to the mix.

There's currently a blocker with a ReadOnly class because it collides with the reserved readonly in PHP 8.1.

This has already been addressed and a fix is on its way with protocolbuffers/protobuf#10041

Until then I think this PR is on hold, but I'm watching the protobuf repo and will continue as soon as it's through... perhaps google/gax und phpdocumentor/reflection will have released new versions as well by then 🤞

@jeromegamez jeromegamez marked this pull request as draft June 12, 2022 23:52
bshaffer
bshaffer requested changes Jun 13, 2022
View reviewed changes
.github/workflows/release.yml Outdated Show resolved Hide resolved
@lcobucci lcobucci mentioned this pull request Jun 21, 2022
Merged
@jeromegamez jeromegamez force-pushed the monolog-3 branch 6 times, most recently from 52288ec to 5c79326 Compare July 28, 2022 21:48
@bshaffer bshaffer mentioned this pull request Nov 28, 2022
Closed
1 task
@jeromegamez
Copy link
Contributor Author

@bshaffer I've seen that you merged main into this branch after #5442 but I rebased anyway to reduce the number of commits - just in case you didn't want to squash-merge later.

@jeromegamez jeromegamez requested a review from bshaffer December 8, 2022 00:24
@bshaffer bshaffer mentioned this pull request Dec 13, 2022
Closed
@bshaffer bshaffer requested a review from dwsupplee December 13, 2022 16:18
dwsupplee
dwsupplee suggested changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@dwsupplee dwsupplee left a comment

Choose a reason for hiding this comment

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

My comments are mostly housekeeping related, structurally this looks great! Thanks @jeromegamez! 👍

Core/src/Logger/AppEngineFlexFormatterV3.php Outdated Show resolved Hide resolved
Core/src/Logger/AppEngineFlexHandlerV3.php Outdated Show resolved Hide resolved
Core/src/Logger/AppEngineFlexHandlerV3.php Outdated Show resolved Hide resolved
Logging/src/LogMessageProcessor.php Outdated Show resolved Hide resolved
Logging/src/LogMessageProcessor/MonologMessageProcessor.php Outdated Show resolved Hide resolved
Logging/src/LogMessageProcessor/MonologV3MessageProcessor.php Show resolved Hide resolved
Logging/src/LogMessageProcessor/MonologV3MessageProcessor.php Outdated Show resolved Hide resolved
Logging/src/LogMessageProcessorFactory.php Outdated Show resolved Hide resolved
Logging/src/LogMessageProcessor.php Outdated Show resolved Hide resolved
Core/src/Exception/ServiceException.php Show resolved Hide resolved
@jeromegamez
fix: Allow ServiceException creation with null as the message
361f725 
Some tests explicitly set the message to `null` which breaks the
creation of parent exceptions with newer PHP versions.

Setting a default `$message = ''` does not prevent the problem, so
we declare it with `null` being allowed, and check and replace it in the
`ServiceException` constructor.

The change is made in the `ServiceException` instead of the tests
calling it incorrectly to reduce the scope of updated files in the
context of the PR that handles the problem.
@jeromegamez
feat: Enable the usage of "psr/log": "^2.0|^3.0"
bdd6336 
`phpdocumentor/reflection` started supporting `"psr/log": "^2.0|^3.0"`
with v5.3.

See phpDocumentor/Reflection#247
@jeromegamez
feat: Enable the installation of Monolog V3
2312f5d 
`psr/log` v1 included a TestLogger that was removed starting with v2.
`fig/log-test` support all current versions of `psr/log`.
@jeromegamez
feat: Introduce V3 versions of AppEngine Formatters and Handlers
5d52730
dwsupplee
dwsupplee suggested changes Dec 14, 2022
View reviewed changes
Logging/composer.json Show resolved Hide resolved
@dwsupplee dwsupplee merged commit 6ad6bc3 into googleapis:main Dec 14, 2022
@release-please release-please bot mentioned this pull request Dec 14, 2022
Merged
@bshaffer
Copy link
Contributor

We missed Debugger: #5711

@jeromegamez jeromegamez deleted the monolog-3 branch December 15, 2022 23:19
@bshaffer
Copy link
Contributor

We are going to wait until after the new year to release this, as our team is currently in a release freeze for the holidays!

@jeromegamez
Copy link
Contributor Author

Super glad to see it released now! I enjoyed the process! We'll meet again at latest when Monolog 4 is released 😄

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

@alexander-schranz alexander-schranz alexander-schranz left review comments

@jamiehollern jamiehollern jamiehollern left review comments

@lcobucci lcobucci lcobucci left review comments

@dwsupplee dwsupplee dwsupplee approved these changes

@bshaffer bshaffer bshaffer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jeromegamez @jamiehollern @bshaffer @lcobucci @alexander-schranz @dwsupplee