[PHP 8.4] Fixes for implicit nullability deprecation

[Ayesh]

Fixes all issues that emit deprecation notices on PHP 8.4 for implicit nullable parameter type declarations.

See:

• RFC
• PHP 8.4: Implicitly nullable parameter declarations deprecated

[xabbuh]

@erusev Could you please take a look at this? This would help us make the CI for Twig green (see https://github.com/twigphp/Twig/actions/runs/9804849131/job/27073403194#step:10:25).

[erusev]

Looks like in order to make a new, 1.7.5 release, we should also fix this in the 1.7.x branch.

Can someone help w/ that?

[xabbuh]

@erusev see #871 (for 1.7) and #872 (for 1.8)

[erusev]

Thanks! Were you able to run the tests? They don't seem to run on my machine — looks like it's bc the phpunit version is old and doesn't work with php8, and I tried downgrading to an earlier php versions but couldn't do it.

[xabbuh]

I opened a PR to use GitHub actions. While doing so I changed the constraint for PHPUnit to allow compatible versions for all PHP versions supported by this package. I also noticed that we need to revert this PR for 1.7.x as PHP 5.3 does not allow the explicit nullable argument syntax (this requires PHP 7.1+).

[erusev]

Nice!

I also noticed that we need to revert this PR for 1.7.x as PHP 5.3 does not allow the explicit nullable argument syntax (this requires PHP 7.1+).

What would be the right way to release the nullable argument fix? A new minor release?

[Ayesh]

I think a minor release would be fine, because it's merely a bug fix + requirement bump, but no fundamental API changes.
Those who are running on PHP 5.3 will not be able update (Compose refuses to update), but those who running PHP 7.1+ can upgrade and have no BC impact.

[xabbuh]

IMO the 1.7.x branch does not need to be updated. If I did not miss anything, 1.8.0 is not released yet. Thus I suggest to drop support for PHP < 7.1 in it and apply the fix only there. People using legacy PHP versions can use 1.7 then, while anyone wanting to use PHP 8.4 can use 1.8.

[erusev]

The problem is, I'm not very familiar with 1.8.0 — @aidantwoods used to work on it, and since it's labeled as beta, i don't know if it's stable enough.

What's the problem w/ releasing it as a patch in 1.7?

[xabbuh]

First, dropping support for a PHP version does not really feel right to me. It’s not a BC break as Composer will prevent people from updating that are using legacy PHP versions. However, you will never be able to publish any future bugfix containing support for legacy PHP versions.

[erusev]

How about 1.9 then?

[xabbuh]

That won’t help much as according to semver 1.9 must contain all the features that are part of 1.8.

From the Symfony/Twig point of view merging the changes into 1.8 without doing any release is totally fine. And since PHP 8.4 hasn’t been released yet drafting a 1.8 release isn’t that urgent IMO.

[Ayesh]

We could rename the 1.8 branch to 1.9, and release the 1.7+fixes+PHP bump as 1.8.

However, it looks like there are non-zero installations reported on Composer for Parsedown 1.8 series.

[erusev]

From the Symfony/Twig point of view merging the changes into 1.8 without doing any release is totally fine. And since PHP 8.4 hasn’t been released yet drafting a 1.8 release isn’t that urgent IMO.

Ok, let's do that.

Do you mind creating a PR?

[xabbuh]

You mean #872?

[erusev]

You mean #872?

Right, yes, sorry.

#873 is for the 1.7 branch, right? Is this on purpose? I imagine it would also be helpful in the 1.8 branch.

And what's the role of branch main?

[xabbuh]

My reasoning was that the 1.7.x can regularly be merged into the 1.8.x-beta branch. But I can also submit a separate PR for 1.8 if that works better for you.

[erusev]

But I can also submit a separate PR for 1.8 if that works better for you.

Yes pls, would be nice to be able to run the tests on #872.

[xabbuh]

Is your plan to keep support for legacy PHP versions in 1.8?

[erusev]

The explicit nullable argument syntax requires PHP 7.1+, right?

If we want it in 1.8, looks like we should drop support for earlier versions.

[xabbuh]

see #874

[erusev]

Merged, thanks!

p.s. Do you know why run fails:
https://github.com/erusev/parsedown/actions/runs/9937863179

[Ayesh]

Re: CI failures, I think it's because the matrix values are expected to have a key:value structure and not a list.

So for example, instead of:

matrix:
                include:
                    - '7.1'
                    - '7.2'
                    - '7.3'
                    - '7.4'
                    - '8.0'
                    - '8.1'
                    - '8.2'
                    - '8.3'
                    - '8.4'

we need something like:

matrix:
        php: [ '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3', '8.4' ]

I see that @xabbuh made those changes not too long ago. I think he will follow-up with improvements to there.

[xabbuh]

see #875

[erusev]

Thanks, also merged #872