Support PHP 8.1 and Symfony 6.1 (limiting BC to PHP 7.2) #281
Conversation
This fixes a deprecation on Symfony 6.1: The "Symfony\Component\Console\Command\Command::$defaultName" property is considered final. You should not override it in "Ekino\NewRelicBundle\Command\NotifyDeploymentCommand".
This reverts commit 9291a12.
|
Hi, I've just had the same problem... It seems that this PR was opened 5 months ago, and approved almost 4 months ago. Are there any news on when it will be merged, or if it won't? |
.github/workflows/ci.yml
Outdated
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| php: ['7.1', '7.2', '7.3', '7.4', '8.0'] | ||
| php: ['7.2', '7.3', '7.4', '8.0', '8.1'] |
There was a problem hiding this comment.
I've put the PHP 7.1 option back in place, so now this only adds 8.1 and switches the latest to it.
If you prefer, I already had the fix for PHP 7.1 composer support (requires updating the shivammathur/setup-php action to at least 2.18.1 and adding a new property to the matrix, just for PHP 7.1) but it will end up failing with a fatal error anyway...
Logging/AdaptiveHandler.php
Outdated
| @@ -28,7 +29,10 @@ public function __construct( | |||
| parent::__construct($level, $bubble, $appName, $explodeArrays, $transactionName); | |||
| } | |||
|
|
|||
| protected function write(array $record): void | |||
| /** | |||
| * @inheritDoc | |||
There was a problem hiding this comment.
please remove, this brings no value
Tests/AppKernel.php
Outdated
| @@ -14,6 +14,7 @@ | |||
| namespace Ekino\NewRelicBundle\Tests; | |||
|
|
|||
| use Ekino\NewRelicBundle\EkinoNewRelicBundle; | |||
| use Exception; | |||
| /** | ||
| * @throws Exception | ||
| */ | ||
| public function __serialize(): array |
There was a problem hiding this comment.
Not entirely sure right now (bear in mind that I did this more than 3 months ago), but AFAICR, it had something to do with something failing under Symfony 6.1
composer.json
Outdated
| @@ -13,15 +13,15 @@ | |||
| } | |||
| ], | |||
| "require": { | |||
| "php": "^7.1 | ^8.0", | |||
| "php": "^7.2 | ^8.0", | |||
There was a problem hiding this comment.
Bumping to 7.2 was done to be able to remove parameter type declaration from write method. Check PR description for more info :)
There was a problem hiding this comment.
I get that removing the support of 7.1 ease the PR (even if there are ways to workarround).
Still, this is not related to this PR.
There was a problem hiding this comment.
I think it was done to make CI green. Would you prefer to remove it and let CI red for this PR?
There was a problem hiding this comment.
Well, if you guys find a way to make it run under PHP 7.1 🤷♂️ I've tried everything I had in mind, but I couldn't find a way without locking maximum Monolog version... Keep in mind that releasing this as-is with PHP 7.1 support WILL break projects under 7.1 that depend on the bundle and don't have a locked version for it.
|
Any update regarding this merge request? |
This fixes deprecations and compilation failures with Symfony 6.1 (thanks to @ruudk for their fork) and PHP 8.1, mainly revolving around Monolog's
AbstractProcessingHandler::writechanged signature.It's worthy of note that:
AdaptiveHandlerwork for both previous and current versions as is in PHP 7.1, while this PR's proposed solution passes all tests in PHP >= 7.2.That being said, this is basically the easy (and AFAIK the only sane) way out - dropping support for PHP 7.1. If that is not satisfying, I will try to find a way to support both PHP 7.1 and 8.1 without limiting monolog to
<3.0later this week, but I can already tell it's not going to be a quick (nor pretty) job...If PHP 7.1 support is really a must, I would suggest keeping a separate branch with monolog's version locked and split this PR into a new major version.
The CI pass evidence (apart from the latter changes to
ci.ymlrearranging the matrix and reverting the branch trigger) is already available here.