feat: upgrade monolog/monolog to v3 #332
Conversation
|
I guess the tests need an update because we now need 8.1. Should I change them? This probably has other implications I'm not aware of. |
|
I am afraid raising minimum PHP version at this time is not in line with for Graby primary consumers. Both Wallabag and selfoss still target PHP 7.4. And while I am open to potentially raising it in selfoss, I would still want to support PHP 8.0 at least until it no longer has upstream security support. |
|
Yeah that makes sense. Given that php 8.0 will be eol in November, should I pick this up again at that time? |
|
If you are willing to implement it, supporting Monolog I would not bet on dropping PHP 8.0 support in November. There are still distros where PHP 8.0 or even 7.4 will be supported for the next while. For example, Debian 11, which ships PHP 7.4, will continue to receive security updates for three more years. |
Done |
|
Now the tests fail with the same issue as in #333 |
| "ext-curl": "*", | ||
| "ext-tidy": "*", | ||
| "fossar/htmlawed": "^1.2.8", | ||
| "guzzlehttp/psr7": "^2.0", | ||
| "j0k3r/graby-site-config": "^1.0.147", | ||
| "j0k3r/httplug-ssrf-plugin": "^2.0", | ||
| "j0k3r/php-readability": "^1.2.9", | ||
| "monolog/monolog": "^1.18.0|^2.3", | ||
| "j0k3r/php-readability": "dev-master", |
There was a problem hiding this comment.
Is there any reason you are bumping readability?
There was a problem hiding this comment.
Because of psr/log, it's detailed in the PR description
There was a problem hiding this comment.
Because readability 1x only supports psr/log 1.
There was a problem hiding this comment.
Maybe you can put ^2.0 it'll be enough for the psr/log v3
flake.nix
Outdated
| pkgs.mkShell { | ||
| buildInputs = with pkgs; [ | ||
| (php82.withExtensions | ||
| ({ all, ... }: with all; [ |
There was a problem hiding this comment.
Not sure if @j0k3r wants to include Nix packaging. I could continue to maintain it for the time being as I use something like this locally.
It might be sufficient to use enabled attribute to load the default extensions:
https://github.com/fossar/selfoss/blob/b0eb43d80bedde5da3204b842511afe245d8c17c/flake.nix#L37-L40
And make the PHP version parametric:
There was a problem hiding this comment.
If you can maintain it, I'm ok.
I didn't what Flake was.
Maybe put a comment at the very top of flake.nix with a link to sth that explain what it does/is.
| @@ -15,15 +15,15 @@ | |||
| "minimum-stability": "dev", | |||
| "prefer-stable": true, | |||
| "require": { | |||
| "php": ">=7.4", | |||
| "php": ">=7.4|>=8.1", | |||
| * @param string $th Row header content | ||
| * @param string $td Row standard cell content | ||
| * @param bool $escapeTd false if td content must not be html escaped | ||
| */ | ||
| private function addRowWithLevel(int $level, string $th, string $td = ' ', bool $escapeTd = true): string | ||
| private function addRowWithLevel(Level $level, string $th, string $td = ' ', bool $escapeTd = true): string |
There was a problem hiding this comment.
This will not work with Monolog 2, will it?
There was a problem hiding this comment.
At least there is a specific build to run on lower deps, so it's checked https://github.com/j0k3r/graby/actions/runs/5375860418/jobs/9752256955?pr=332#step:4:115
There was a problem hiding this comment.
Hmm, I'm getting this error, I think I have monolog 2 installed.
Graby\Monolog\Formatter\GrabyFormatter::addRowWithLevel(): Argument #2 ($th) must be of type string, int gi
Resolves #331
This probably needs some more fixing. I've added the nix flake to be able to run composer et al, can remove it if you don't need it.
I had to upgrade
j0k3r/php-readabilitytodev-masterbecause monolog now requirespsr/login at least version 3 and the last release only supports v1. As such, this is probably not mergable right now because we now have a dependency onmasterfrom that package which will give users something different every time. If you release a new version of that package, I'll happily pin it to that release version.As per monolog's release docs, the minimum required php version is now 8.1 so I upgraded that as well. I think I migrated all uses of it but I'm not 100% sure. This probably needs more testing from someone who knows how that feature works.