Add support for Monolog 2.x

[jeromegamez]

Hey everyone 👋

This is a follow up to #2293 and aims to add support for both monolog/monolog 1.x and 2.x.

Monolog 2.x was released on August 30th, 2019 and requires PHP ^7.2 whereas google/cloud keeps supporting PHP 5.6.

The specific issue is that Monolog 2.x adds method return types which are not supported by PHP 5.x. On the other hand, leaving the return type declarations out causes PHP 7.x to throw errors.

AppEngineFlexHandler and AppEngineFlexFormatter remain the implementation for Monolog 1.x.

AppEngineFlexV2Handler and AppEngineFlexV2Formatter are the new implementations for Monolog 2.x.

The new classes are the same as the old ones with the only difference being added return type declarations.

Notes

• I thought about extracting the common code instead of duplicating it, but I didn't want to go overboard in the very first iteration of the PR, I first wanted to check if you would consider merging the change at all :)
• The Monolog\Logger::API constant has been added in Monolog 1.6, which requires the version bump for the 1.x constraint. Monolog 1.6 has been released in 2013, so I don't think this should be a real-world issue compared to the previous ~1 constraint.
• The only place where I could find the handlers referenced is on https://cloud.google.com/php/getting-started/logging-application-events?hl=de (switching to English via the language switcher resulted in a 404 for me).

[jdpedrie]

Perhaps a factory class would be useful as well? LoggerFactory::getFormatter() and LoggerFactory::getHandler(), both detect which version of monolog is available and construct an implementation?

[jeromegamez]

Perhaps a factory class would be useful as well? LoggerFactory::getFormatter() and LoggerFactory::getHandler(), both detect which version of monolog is available and construct an implementation?

I thought about this as well, similar to the HttpHandlerFactory in google/auth, right?

[codecov]

Codecov Report

Merging #2302 into master will decrease coverage by 0.11%.
The diff coverage is 89.74%.

@@             Coverage Diff              @@
##             master    #2302      +/-   ##
============================================
- Coverage      92.6%   92.48%   -0.12%     
- Complexity     4467     4477      +10     
============================================
  Files           307      311       +4     
  Lines         13323    13352      +29     
============================================
+ Hits          12338    12349      +11     
- Misses          985     1003      +18

Impacted Files	Coverage Δ	Complexity Δ
Core/src/Logger/AppEngineFlexHandler.php	0% <0%> (-83.34%)	3 <0> (ø)
Core/src/Logger/AppEngineFlexFormatter.php	0% <0%> (-77.78%)	2 <0> (-1)
Core/src/Logger/AppEngineFlexFormatterV2.php	100% <100%> (ø)	2 <2> (?)
Core/src/Logger/AppEngineFlexHandlerV2.php	100% <100%> (ø)	3 <3> (?)
Core/src/Logger/FormatterTrait.php	100% <100%> (ø)	2 <2> (?)
Core/src/Logger/AppEngineFlexHandlerFactory.php	71.42% <71.42%> (ø)	4 <4> (?)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee34ce2...37de613. Read the comment docs.

[jdpedrie]

@jeromegamez yep!

[jeromegamez]

Just added an additional commit with the factory \o/ - I had to fix the lower version to ^1.1 because Monolog 1.0.* didn't have an autoloader 😅

[jdpedrie]

Thanks again @jeromegamez, appreciate the quick changes! A couple more things I see, and I'm also wondering whether the name format is ambiguous. AppEngineFlexV2Handler seems to me that it could imply it's talking about App Engine Flex V2. Perhaps AppEngineFlexHandlerV2 would be better? What do you think?

[jeromegamez]

Thank you @jdpedrie, I appreciate the fast responses! I made the changes and hope this looks better now. Once you are happy with the changes, I would like to squash the changes into a single commit, if you agree.

[jeromegamez]

I'm looking into the build failures right now, it seems I missed something in the other test suites.

[jdpedrie]

@jeromegamez it's not your fault, it's an issue with the parser we use to extract and test code snippets. Since it scans the entire project, it fails when it runs into the scalar type hints and return types (in PHP 5) and when those declarations are missing and the interface isn't properly fulfilled (in PHP 7.2+).

Modify Google\Cloud\Core\Testing\Snippet\Coverage\Coverage lines 30-33 to the following:

    private static $snippetExcludeList = [
        '/\\\GcTestListener/',
        '/\\\Google\\\Cloud\\\Core\\\Logger/',
        '/\\\Google\\\Cloud\\\Core\\\PhpArray/',
    ];

[jeromegamez]

Whew, I'm relieved 😌! I couldn't get it to work and was already running out of possible workarounds ^^ Thanks!

[jeromegamez]

Uh, I just realized that I totally misread your Factory proposal... shall I rewrite the factory again?

[jdpedrie]

No, I like how you did it. I don't believe there is generally a need to directly instantiate a formatter, so the single method for the handler is sufficient.

As to the latest build failures, it's the same issue as the previous one, but related to the doc generator. I'll need to investigate workarounds a bit further.

[jeromegamez]

@jdpedrie I added something to the doc generator that hopefully can solve the problem - at least executing dev/google-cloud doc -c cloud-core didn't throw a parse error anymore, and the JSON files are generated... as always: on my machine™ 😅 and I haven't had a deeper look into the files to check if they make sense - but they are JSON files \o/

[jeromegamez]

I had to update the PR again because I noticed that the class-to-file map in the CodeParser was incorrect - the V2 classes pointed to the V1 files 😓

[dwsupplee]

@jeromegamez

You, my friend, are awesome.

This is very well thought out and implemented, I can't thank you enough for taking the time to contribute 👏.

[jeromegamez]

Thank you @dwsupplee, this makes me happy 🥰, and thank you @jdpedrie for the fruitful interaction, this was a lot of fun!