Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge 1.8.x-beta into master #881

Merged
merged 13 commits into from
Oct 12, 2024
Merged

Merge 1.8.x-beta into master #881

merged 13 commits into from
Oct 12, 2024

Conversation

xPaw
Copy link
Contributor

@xPaw xPaw commented Aug 28, 2024

The changes are minimal, master already had all the code changes from 1.8 branch, so this simply adds github actions and bumps up the php version.

@erusev mind taking a look so that there's less confusion about master and the 1.8 branch?

@erusev
Copy link
Owner

erusev commented Aug 28, 2024

@xabbuh do you have any thoughts on this?

@BenjaminHoegh
Copy link

I think it would be a good idea as 1.8 seems stable as improve common mark support a lot. It also fixes a lot of issues with GFM. And for extensions there is little to no work for adding support for the changes

@xabbuh
Copy link
Contributor

xabbuh commented Sep 2, 2024

@erusev sounds reasonable to me

@erusev
Copy link
Owner

erusev commented Sep 29, 2024

If we merge 1.8.x-beta into master, can we then remove the 1.8.x-beta branch? I find the branch situation a bit confusing.

@xPaw
Copy link
Contributor Author

xPaw commented Sep 29, 2024

I believe it can be removed, yes.

@erusev
Copy link
Owner

erusev commented Sep 29, 2024

If we merge 1.8.x-beta into master, can we then remove the 1.8.x-beta branch? I find the branch situation a bit confusing.

@xabbuh do you have any thoughts on this?

@aragon999
Copy link

One thing, which is changed by this merge in addition to the changes which are in the 1.8.x branch, is that the tests are not compatible with php 7.1 (phpunit version 7.5).

But in my opinion this is fine, as this version is already eol for some time now (December 2019): https://www.php.net/eol.php

Maybe this php version should be dropped as well for the 1.8.0 release.

@xPaw
Copy link
Contributor Author

xPaw commented Sep 30, 2024

PHPUnit 7 specifies PHP 7.1 - PHP 7.3 support, is that not the case?

The github action does have 7.1 in the test matrix, but it didn't run on this PR. Does seem to work here though: https://github.com/xPaw/parsedown/actions/runs/10592900295/job/29353218280

@aragon999
Copy link

@xPaw ah yes, sorry for the confusion. I was thinking about it the other way around, i.e. see the following commment:
https://github.com/sebastianbergmann/phpunit/blob/7.5.20/src/Framework/TestCase.php#L407

But if you add the void declaration in the child class, it is fine: https://3v4l.org/tNYS3#v7.1.33

@erusev erusev merged commit 582f9f9 into erusev:master Oct 12, 2024
@xabbuh
Copy link
Contributor

xabbuh commented Oct 12, 2024

This looks reasonable to me. 👍

@xPaw xPaw deleted the merge-1.8.x branch October 12, 2024 08:45
@erusev
Copy link
Owner

erusev commented Nov 3, 2024

Should we also release 1.8.0 and make it latest? @aidantwoods @xabbuh @xPaw

@xPaw
Copy link
Contributor Author

xPaw commented Nov 3, 2024

I've been running that code in production for a while.

@erusev
Copy link
Owner

erusev commented Nov 9, 2024

What should the release notes for 1.8.0 say?

@BenjaminHoegh
Copy link

BenjaminHoegh commented Nov 10, 2024

What should the release notes for 1.8.0 say?

Can't you use the one from the draft #601 ?

you could easily make it something like this from that list:

New Features

Bug Fixes

This release addresses multiple issues, improving parsing accuracy and compliance with CommonMark:

@dregad
Copy link

dregad commented Nov 13, 2024

It's great to see Parsedown getting some attention again, thanks guys.

I'm not sure if you are aware, but a few years ago @aidantwoods posted this: #714 (comment)

I ended up deciding not to release 1.8.x as a minor upgrade due to quite a few internal changes that might break extensions (context: #601 (comment)). If you're not using any extensions then I'd just go ahead and use 1.8.x for now (it contains quite a few bug fixes over 1.7.x).

EDIT: details of the actual breakage are in #601 (comment), not in the post originally referenced in the above quote.

I personally don't use extensions so I'm fine with the potential BC break, but you might want to at least mention that in the 1.8 release notes. Maybe @aidantwoods will care to comment.

@BenjaminHoegh
Copy link

It's great to see Parsedown getting some attention again, thanks guys.

I'm not sure if you are aware, but a few years ago @aidantwoods posted this: #714 (comment)

I ended up deciding not to release 1.8.x as a minor upgrade due to quite a few internal changes that might break extensions (context: #601 (comment)). If you're not using any extensions then I'd just go ahead and use 1.8.x for now (it contains quite a few bug fixes over 1.7.x).

EDIT: details of the actual breakage are in #601 (comment), not in the post originally referenced in the above quote.

I personally don't use extensions so I'm fine with the potential BC break, but you might want to at least mention that in the 1.8 release notes. Maybe @aidantwoods will care to comment.

Valid point, maybe we should consider changing the version number to 2.0.0 as this would follow semantic version. Then the current branch for 2.0 would have to change to 3.0

@erusev
Copy link
Owner

erusev commented Nov 16, 2024

@aidantwoods Would love to get your thoughts on this.

@erusev
Copy link
Owner

erusev commented Nov 23, 2024

Looks like initially @aidantwoods wanted to release this as 1.8:

On making this a minor release, I think it is certainly "fair game" due to not breaking the public API. -- #601 (comment)

But then he decided against it:

I ended up deciding not to release 1.8.x as a minor upgrade due to quite a few internal changes that might break extensions. -- #714 (comment)

I'm open to releasing it as 2.0, but the fact that @aidantwoods never released it as a stable release makes me wonder if it's good enough.

erusev referenced this pull request Dec 9, 2024
@joeworkman
Copy link

If you want to play it safe, release it as 2.0...

@Evgeny1973
Copy link

PHP 8.4 says:

Deprecated: Parsedown::blockSetextHeader(): Implicitly marking parameter $Block as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/vendor/erusev/parsedown/Parsedown.php on line 715

Deprecated: Parsedown::blockTable(): Implicitly marking parameter $Block as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/vendor/erusev/parsedown/Parsedown.php on line 853

Can you update, pls?

@xabbuh
Copy link
Contributor

xabbuh commented Jan 14, 2025

@Evgeny1973 with which version of this package do you get it?

@Evgeny1973
Copy link

@xabbuh Parsedown 1.7.4 and Symfony 7.1.

@Evgeny1973
Copy link

Evgeny1973 commented Jan 14, 2025

Sorry, as I see fixes of these deprecations to the master branch have already been made. Looking forward to the release!

@dregad
Copy link

dregad commented Jan 15, 2025

@xabbuh would it be possible to tag 1.7.5 with this fix ? see #889

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants