update dependencies #19
Conversation
There was a problem hiding this comment.
Overall LGTM. Since we're refreshing the bundle, I would also add PHPStan and our code style (in separate PRs): https://github.com/facile-it/facile-coding-standard/
composer.json
Outdated
| "symfony/framework-bundle": "^2.7 || ^3.0 || ^4.0", | ||
| "symfony/dependency-injection": "^2.7 || ^3.0 || ^4.0", | ||
| "symfony/config": "^2.7 || ^3.0 || ^4.0", | ||
| "php": "^7.4 || ^8.1", |
There was a problem hiding this comment.
Why is PHP 8.0 dropped? I know it's EOL, but if we're still supporting 7.4, it doesn't make sense to drop only that one.
There was a problem hiding this comment.
I would see this with @ilario-pierbattista
There was a problem hiding this comment.
I agree with @Jean85, we should support all php versions >= 7.4
.github/workflows/main.yml
Outdated
| - name: Force Monolog | ||
| if: matrix.symfony == '^6.4' | ||
| run: composer require "monolog/monolog" --no-update |
There was a problem hiding this comment.
Why do we need this for 6.4 only?
There was a problem hiding this comment.
It's weird, but it fails otherwise. Look at the Actions, the last failing one. It doesn't find a monolog class, and I don't understand why. If you have a better solution, let me know.
There was a problem hiding this comment.
Due to the force push, I cannot see it...
There was a problem hiding this comment.
Due to the force push, I cannot see it...
it's still visible here https://github.com/facile-it/crossbar-http-publisher-bundle/actions/runs/9495808250/job/26168834801
There was a problem hiding this comment.
It seems that it's trying to load the Symfony Monolog Bridge's DebugProcessor, which is activated here: https://github.com/symfony/symfony/blob/7b5377056d151024d994757aba12db556aa2ee28/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1180-L1188
As you can see, it can be shut off by disabling debug mode, which should also be the correct way to proceed in a test; try debug: false in the app that you're spinning up during the tests and that should be fixed. It's strange that it gets activated though, since I see a class_exists check too...
There was a problem hiding this comment.
That's ugly. Probably this is caused by the committed composer.lock. We should try to remove it in CI before other dependencies are installed.
Btw I think it's useless to even have a committed composer.lock in a library project that aims to provide such a wide compatibility. What about it?
There was a problem hiding this comment.
I thought the same at first, but then I realized that in the Actions we perform a composer update. So it's not a lock-related problem.
Anyway, I agree that keeping the lock doesn't make much sense.
There was a problem hiding this comment.
If you have a composer.lock, composer update can still have a different behavior; I agree with @ilario-pierbattista, we should aim to remove it.
| public function createPublisher( | ||
| string $protocol, | ||
| string $host, | ||
| int $port, | ||
| string $path, | ||
| ?string $key, | ||
| ?string $secret, | ||
| $hostname, | ||
| bool $ignoreSsl = false | ||
| ): Publisher { |
There was a problem hiding this comment.
This (and further edits) should normally be considered breaking changes. Are we considering to release this as a new major?
There was a problem hiding this comment.
This (and further edits) should normally be considered breaking changes. Are we considering to release this as a new major?
I proposed this as a BC. I kept the 7.4 compatibility only to allow applying this upgrade before upgrading our project to PHP 8.
There was a problem hiding this comment.
@Jean85 yes, I'd propose to release those BCs with a new major.
composer.json
Outdated
| "symfony/framework-bundle": "^2.7 || ^3.0 || ^4.0", | ||
| "symfony/dependency-injection": "^2.7 || ^3.0 || ^4.0", | ||
| "symfony/config": "^2.7 || ^3.0 || ^4.0", | ||
| "php": "^7.4 || ^8.1", |
There was a problem hiding this comment.
I agree with @Jean85, we should support all php versions >= 7.4
| public function createPublisher( | ||
| string $protocol, | ||
| string $host, | ||
| int $port, | ||
| string $path, | ||
| ?string $key, | ||
| ?string $secret, | ||
| $hostname, | ||
| bool $ignoreSsl = false | ||
| ): Publisher { |
There was a problem hiding this comment.
@Jean85 yes, I'd propose to release those BCs with a new major.
.github/workflows/main.yml
Outdated
| - name: Force Monolog | ||
| if: matrix.symfony == '^6.4' | ||
| run: composer require "monolog/monolog" --no-update |
There was a problem hiding this comment.
That's ugly. Probably this is caused by the committed composer.lock. We should try to remove it in CI before other dependencies are installed.
Btw I think it's useless to even have a committed composer.lock in a library project that aims to provide such a wide compatibility. What about it?
|
|
Updates I did today: re-add php 8.0 compatibility, remove lock file, change test kernel to non-debug, remove monolog test hack. |
Fix #18