google-cloud-php icon indicating copy to clipboard operation
google-cloud-php copied to clipboard

feat: Add support for Monolog 3.x

Open jeromegamez opened this issue 2 years ago β€’ 6 comments

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

  • https://github.com/phpDocumentor/Reflection/pull/247 is merged but not released (this PR uses ^5.x-dev for now)
  • ~https://github.com/googleapis/gax-php/pull/389 is not reviewed/merged/released (this PR uses my PR branch from over there for now)~
  • https://github.com/GoogleCloudPlatform/grpc-gcp-php/pull/45 is not (yet) merged - if it's just because of the CLA, I can create a new PR there

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 avatar Jun 10 '22 14:06 jeromegamez

~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 avatar Jun 10 '22 14:06 jeromegamez

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 avatar Jun 10 '22 15:06 jeromegamez

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 avatar Jun 10 '22 21:06 jeromegamez

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 https://github.com/protocolbuffers/protobuf/pull/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 avatar Jun 12 '22 23:06 jeromegamez

Whew! Alright, 20 action runs later, I think I'm getting somewhere - the test suites finally don't exit with errors anymore πŸ₯³

Due to the two PRs not being merged/released yet, this one is not mergeable as well, but if someone wants to try this PR in their projects, that would be the best test πŸ˜…. And of course, nobody has reviewed the changes yet 😬.

(The Docs Generator Job is still failing, and I can't figure out why. If you have an idea, please let me know, otherwise I'll give it a closer look tomorrow with a rested brain πŸ’ͺ)

jeromegamez avatar Jul 28 '22 22:07 jeromegamez

I had to add the dev branch of the reflection library because its dependency of psr/log 1 prevented Monolog from installing, since it needs version 3. I hoped my PR over there would have been released, but removing that part of the doc generation altogether sounds even better!

I will rebase this PR with main later (in a few hours) to see if everything still works 🀞🏻.

Do you see a chance to merge and release https://github.com/GoogleCloudPlatform/grpc-gcp-php/pull/45 in the near future? This is another blocker πŸ˜…

jeromegamez avatar Sep 14 '22 18:09 jeromegamez

Alright! I managed to green up all checks πŸ₯³

Concerning the jump to a new major version: phpdocumentor/reflection is exclusively used as a dev-dependency in the following places (I think you mentioned that already, @bshaffer):

  • Core (only tests)
  • docs generation

Although it's unfortunate to have to rely on a not-yet-released dev dependency, we already hat one dev dependency before (which is now removed):

CleanShot 2022-10-11 at 00 39 38@2x

One out, one in? πŸ˜…

#5325 could still be applied afterward, and I have a notification on the next phpdocumentor/reflection stable 5.x release.

To re-iterate the COMPOSER_TOKEN: ${{ secrets.GITHUB_TOKEN }} topic: this doesn't expose a (new) secret. The token is available on every workflow run, and its only purpose is to avoid running into rate limits when fetching composer dependencies.

jeromegamez avatar Oct 10 '22 22:10 jeromegamez

I also want to add that many changes seemingly are not related to Monolog, but without them, the tests failed on my machine with PHP 8.1.

jeromegamez avatar Oct 10 '22 22:10 jeromegamez

Great work @jeromegamez. I'm relying on this PR to make one of the APIs work with Symfony 6, so many thanks for your efforts. Is there anything you would like support with?

jamiehollern avatar Oct 11 '22 10:10 jamiehollern

Good news! phpdocumentor/reflection 5.3 was just released, and I have reverted the ${GITHUB_TOKEN} changes from the workflows.

jeromegamez avatar Oct 14 '22 14:10 jeromegamez

The package test is failing in ErrorReporting with the following:

PHP Fatal error:  Declaration of Google\Cloud\Logging\PsrLogger::emergency($message, array $context = []) must be compatible with Psr\Log\LoggerInterface::emergency(Stringable|string $message, array $context = []): void in /home/runner/work/google-cloud-php/google-cloud-php/Logging/src/PsrLogger.php on line 172

Edit: After some investigation, the newer psr/log: 3.0 is being pulled in, which our Google\Cloud\Logging\PsrLogger is not compatible with. So we'll need to add a dependency on "psr/log": "^1.0||^2.0" in Logging/composer.json or wait until we drop support for PHP 7.0 and add the void return types to this logger.

bshaffer avatar Oct 19 '22 18:10 bshaffer

I have a solution for making it compatible with all three psr/log versions 90% working locally because I had a feeling people would demand it. I will propose this in a separate PR based on the changes in this one 🀞. I hope the latest push will finally do the trick ☺️

I have another question - at the moment, run-package-tests.sh doesn't output the installed dependencies so that we won't see which Monolog version was actually installed (same with Guzzle). What would you think about adding composer show to the output or removing the -q parameter on installation? Or would that blow the output up too much?

jeromegamez avatar Oct 19 '22 20:10 jeromegamez

It went through! 🀯 πŸ₯³

jeromegamez avatar Oct 19 '22 21:10 jeromegamez

@bshaffer With the changed testing process, would you like me to try reverting the changes related to components other than Core and Logging? That would lead to new PRs though, I don't know how exhausted you guys are of me already 😬.

jeromegamez avatar Oct 19 '22 21:10 jeromegamez

I removed almost all commits that are not strictly related to the feature and renamed/reordered the others to read better and to have a fix/chore/feat order. πŸ’ͺ

This PR has had quite a journey! ☺️

jeromegamez avatar Oct 19 '22 22:10 jeromegamez

@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 avatar Nov 29 '22 08:11 jeromegamez

We missed Debugger: https://github.com/googleapis/google-cloud-php/pull/5711

bshaffer avatar Dec 14 '22 21:12 bshaffer

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!

bshaffer avatar Dec 19 '22 17:12 bshaffer

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

jeromegamez avatar Jan 27 '23 19:01 jeromegamez