Upgrade bundled michelf/php-markdown in serendipity_event_markdown for PHP > 8.0 compatibility

[spackmat]

Hi,

I upgraded to S9Y 2.4-beta1 and switched to PHP 8.0 (and later to 8.1). That works fine for the core, but the serendipity_event_markdown plugin raised some 500 errors. I switched to the "lib" markdown version inside its config, but that also did not work, as the bundled version of michelf/php-markdown is outdated.

I did a quick test with the current version 1.9.1 of michelf/php-markdown that seems to work fine with PHP 8.0 and PHP 8.1. The changelog of the two newer versions 1.9.0 and 1.9.1 don't mention breaking changes, so I don't expect BC breaks.

So I suggest to update the bundled lib inside the plugin accordingly. I can send a PR, if that is welcome.

And also I suggest to ditch the not updated bundled classic version of markdown (and the whole config item) or at least make the "lib" version the default. Having some annoying discussions about keeping support of very EOL PHP versions in plugins in a bad old memory, I opt out of sending a pull request for that.

Greets
spackmat

[onli]

A PR would be very welcome.

The markdown plugin is maintained by @th-h, who is chronically short on time, so I assume you can go ahead if you want to provide maintenance work. If the classic version supports PHP 7 it should not be removed, but if even that is too new it should, and changing the default to a maintained alternative will definitely be fine regardless of bc-version support. Imho all, but I'd also merge accordingly ;)

Keep in mind that if an option is removed in a multi selection box old blogs will fall back to the new option under the index, it should still work in that scenario.

[th-h]

I did a quick test with the current version 1.9.1 of michelf/php-markdown that seems to work fine with PHP 8.0 and PHP 8.1. The changelog of the two newer versions 1.9.0 and 1.9.1 don't mention breaking changes, so I don't expect BC breaks.

So I suggest to update the bundled lib inside the plugin accordingly. I can send a PR, if that is welcome.

It should pose no problem to upgrade the bundled library, yes. A PR would be welcome.

And also I suggest to ditch the not updated bundled classic version of markdown (and the whole config item) or at least make the "lib" version the default.

Yes, we should remove the "classic" version (and drop the config option, and change the config to the library version for existing installations, if it had been selected beforehand). I'll look into that when I have time (not really soon, I'm afraid).

[spackmat]

Oh, I just saw that the 1.9 branch of the lib did not mention inside the changelog-section of its readme that the minimum PHP version was bumped to 7.4 and that several breaking code changes were made that really need at least PHP 7.4. So I hold back my PR to ask if that would still be okay?

[onli]

Since 7.3 is EOL for me that would be okay. @th-h, what do you think?

We could also raise the required s9y version of the plugin to 2.4-beta1.

[th-h]

Minimum PHP version for our current release is announced as 7.0, so we would have to rise the required version of the plugin.

[spackmat]

That problem should IMHO be discussed on the main project level. When I run a software, I expect that also the officially bundled plugins respect the system requirements of the current core release. So if, for whatever reason, I run the current 2.4-beta1 of s9y on PHP 7.0, because that is the officially minimum required version, I would not be so happy to get 500 errors, when I install a plugin from the provided repository and then it turns out that this plugin has higher requirements.. Also the other way around (on PHP >=8.0), as it is the current situation.

What I want to say: The s9y core system should use the ongoing release of v2.4 to raise the minimum PHP version to one that is not already EOL. So in fact, with the current pace of s9y-releases, also PHP 7.4 will be EOL until then. ;) So maybe a 2.4.0 should be released before November, if PHP 7 compatibility shall be kept. So then, plugins and bundled libs can all raise to PHP 7.4 if needed. (Or raise all directly to PHP 8 to enable possibilities for cool refactorings to current standards.)

And for the markdown plugin: For me it works well with michelf/php-markdown patched to version 1.9.1 on PHP 7.4, 8.0 and 8.1, where the current version does in fact not work at all on PHP 8.0 and 8.1. So if s9y 2.4 allows PHP 8, the plugin also should IMHO. If the s9y core stays at PHP >= 7.0, that could be solved by bundling two versions of the lib and use the config option to choose between the PHP >=7.0,<7.4 and the >=7.4 version. Not my favorite solution, but still better than bundling just one and not being compatible to the >=7.0 requirement of the s9y core.

Edit: The plugin can by itself decide, which version of the bundled lib is loaded, so bundling two versions would be not as bad as I thought fist.

[onli]

Regarding the core decision: IIRC, we agreed that future s9y versions should on paper only support officially supported PHP version. So if we released 2.4 today it would be 7.4 as the lowest version - though of course we will not break BC with old PHP versions if it can be avoided with reasonable effort, and we won't break compatibility just because (and not for things like refactoring).

Plugins in spartacus can have requirements on their own - it would be okay for the plugin to say it needs PHP 8 (we have a metadata check for that, it should not lead to a 500). Of course better if that can be avoided, so all all installations can install the update.

Is that the info you need for a proper solution? :)

[spackmat]

So then a new plugin version can bump PHP version to >=7.4 differing from the current s9y release. If that is okay, I'll prepare PR for that.

[spackmat]

OK, see PR s9y/additional_plugins#141

we have a metadata check for that, it should not lead to a 500

I got one, because the old version had PHP 5.3.0 as minimum requirement and PHP 8.0 matches that and so it tried to load the outdated lib and that threw some 500s. The built in check also checks for minimum PHP version requirements, not for maximum version requirements. So I think in that context raising the requirement for the new plugin version (I went to 1.31) was the best way to cope with the situation. Older PHP versions get the last version 1.30.1 that works fine for them. Let's hope for people running the plugin on PHP <=7.3 that no security patches are needed for the older branch.

[th-h]

The built in check also checks for minimum PHP version requirements, not for maximum version requirements. So I think in that context raising the requirement for the new plugin version (I went to 1.31) was the best way to cope with the situation. Older PHP versions get the last version 1.30.1 that works fine for them.

That did not work for me. s9y happily updated the plugin, but as I'm still running PHP 7.3.31, that didn't work out. Restored the old plugin from backup, works again. I think the PHP minimum version check does not work the way it should.

[spackmat]

@th-h that ist not good. Did you open an issue on that? Because a missing plug-in requirement check in the core would break a lot of installations.