search

erusev / parsedown-extra

Markdown Extra Extension for Parsedown
MIT License
822 stars 124 forks source link

Fixed PHP 8.4 deprecations #185

Closed davidbyoung closed 1 month ago

davidbyoung commented 2 months ago

Fixes #184

davidbyoung commented 2 months ago

@erusev Is this repo still being maintained?

erusev commented 2 months ago

Were you able to run the tests?

davidbyoung commented 2 months ago

I did run the unit tests on PHP 8.4RC1 on my local machine, and they all passed. It's a backward-compatible change and that just makes the nullable parameters explicitly nullable.

davidbyoung commented 2 months ago

btw, I noticed that the tests defined in "scripts" in composer.json are largely broken. For example, the path to CommonMarkTestStrict.php and CommonMarkTestWeak.php are incorrect (they should be prefixed with "vendor/erusev/parsedown/"), and even then many of the test scenarios are failing. This is not due to the changes in my PR, though - it looks like some general cleanup is needed in this repo to get it healthy again.

davidbyoung commented 2 months ago

I've pushed an update to fix some of those issues, but likely more are needed. For example, we should probably add PHP 8.4 to CI and allow it to fail (it will until things like Psalm and PHP-CS-Fixer natively support PHP 8.4 without having to use things like PHP_CS_FIXER_IGNORE_ENV=1 to work around their lack of support).

erusev commented 2 months ago

Do you know why we have 3 branches?

CleanShot 2024-09-29 at 17 39 17@2x

What's each branch for?

davidbyoung commented 2 months ago

I'm not sure. It looked like 2.0.x was more up to date than master (eg it had GitHub Actions instead of TravisCI, it separated code into the src directory), which is why I rebased off it for this pull request.

erusev commented 2 months ago

Hm, ok, but if we're making a release for these changes, shouldn't it be a 0.8.x release?

davidbyoung commented 2 months ago

What's v2 for, then? Its most recent release was a few years after 0.8.x.

erusev commented 2 months ago

It's probably a version of the extensions that was designed to work with Parsedown 2.0. It's something @aidantwoods was working on but it might be abandoned.

I'm also not sure why 0.8 is a separate branch from master.

There's a discussion about the same thing in Parsedown at https://github.com/erusev/parsedown/pull/881.

erusev commented 1 month ago

@aidantwoods can I merge 0.8 into master and delete it?

aidantwoods commented 1 month ago

It's been a while, but I think parsedown 1.8.x (and extra 0.8) had some risk of breaking extensions due to internal changes to escaping HTML. I think I put these out as prereleases because I wasn't sure what the BC promise should be for not breaking extensions (given they hook into a protected API surface rather than a public one).

erusev commented 1 month ago

Do you think it still makes sense to have that branch? The latest release, 0.8.1 is almost 5 years old now.

We just merged the 1.8 of Parsedown into main here: https://github.com/erusev/parsedown/pull/881

aidantwoods commented 1 month ago

If we're happy releasing 1.8 as stable I think there's no need to keep the separate branches around

erusev commented 1 month ago

@davidbyoung

Can you create a PR that fixes PHP 8.4 deprecations in 0.8.x branch and also adds a GitHub workflow for testing similar to the one in the 2.0.x branch? I can then merge 0.8.x into main.

You can also use the GitHub workflow at https://github.com/erusev/parsedown as reference.

davidbyoung commented 1 month ago

Closing in favor of #186. @erusev There was only a single deprecation I had to fix, plus I had to pull in some minor changes from master to get the tests to work. I don't have permission to actually try running the GitHub Actions CI, so you'll need to approve that workflow when you get a minute. Finally, I didn't re-add all the tools that were in v2 of Parsedown Extra (eg Psalm) as that felt out of scope for this particular fix.

erusev commented 4 weeks ago

Merged.

Should we also make a release? Let's say 0.8.2.

davidbyoung commented 4 weeks ago

Merged.

Should we also make a release? Let's say 0.8.2.

Yep, sounds good.

davidbyoung commented 2 weeks ago

@erusev Can you tag 0.8.2?

erusev commented 1 week ago

Tagging it as a patch (0.8.2) is probably not right because it now points to a newer version of Parsedown version, and it looks like this new version introduces some breaking changes.

We should probably resolve https://github.com/erusev/parsedown/pull/881 first, and then make a release here.

Does this make sense?