Update to PHP 8.4 by settermjd · Pull Request #32 · mnapoli/FrontYAML

settermjd · 2025-04-26T12:17:43Z

I've been refactoring and modernising an older code base recently and hit a wall using PHP 8.4 because of the current implementation of FrontYaml. Given that I love using it and want to continue, I've created this PR to modernise it for PHP 8.4 (and to remove explicit support for older versions that are long since EOL).

Specifically, the PR:

Updates the required versions of PHP to be 8.2 - 8.4
Updates the third party packages (both dev and non-dev) to be the latest version that supports PHP 8.2
Completes adding type support throughout the library
Removes any deprecated or soon-to-be deprecated code
Updates the "ci" GitHub Workflow configuration to reflect the newly supported PHP versions, and to update outdated dependencies to the latest versions

This change updates the Composer configuration to only support PHP 8.2, 8.3, and 8.4, and then updates the relevant packages to the latest version that supports at least PHP 8.2 as well. Along with that, it updates PHPUnit to version 11 as it's the latest version that at least supports PHP 8.2.

This change removes usage of deprecated code in the codebase, either replacing it with the recommended replacement, or the standard replacement. So, MarkdownConverterInterface was replaced with the recommended replacement of ConverterInterface, and the applicable function call was replaced. Then, getMockForAbstractClass, which is deprecated without replacement in PHPUnit 12 was replaced with createMock.

This change makes explicit use of types throughout the codebase wherever they were missing. In addition, it uses union types for nulls, as that's required in PHP 8.4.

This change updates the GitHub Workflow to support only PHP 8.2 - 8.4.

On attempting to run the workflow, I encountered the following error: "Missing download info for actions/cache@v2". After some searching on the error, the recommended fix was to update from version 2 to 4. So, this change is being made to test the solution and, all being well, correct the issue.

mnapoli

Thanks a lot! I added a few questions inline

mnapoli · 2025-04-26T13:25:40Z

src/Bridge/CommonMark/CommonMarkParser.php

+    private ConverterInterface|null $parser;

-    public function __construct(MarkdownConverterInterface $commonMarkConverter = null)
+    public function __construct(ConverterInterface|null $commonMarkConverter = null)


Just to be sure: this isn't a BC break?

Hmm, good question. I'd not thought of that. Will check.

I'll admit that I cannot say with 100% certainty that it isn't. However, I believe it's not as, up until now, the parameter has been allowed to be null, marking it as a null type implicitly. I believe that this change just makes that state explicit.

No I mean the change from MarkdownConverterInterface to ConverterInterface

Sorry, I misunderstood. Thanks for clarifying. Will look into it today.

I hope that I've better understood you this time. ConverterInterface has been available since 2.2.0, back in early 2022. I suspect that it likely wouldn't be a BC break, but hope that I've assessed this correctly.

This is a BC break, we can't merge that as-is.

Here's an example:

// This is userland code $parser = new ...(); // class that implements MarkdownConverterInterface new Mni\FrontYAML\Bridge\CommonMark\CommonMarkParser($parser);

the code above works today. With your PR, it wouldn't work.

Thanks for discussing this further. I don't think I fully understood your point, originally.

@mnapoli, I reverted the BC break in 286d132.

src/Parser.php

settermjd · 2025-04-27T11:46:22Z

@mnapoli, thanks for taking the time to review my PR.

src/Bridge/CommonMark/CommonMarkParser.php

@mnapoli

As was pointed out by @mnapoli, https://github.com/mnapoli/FrontYAML/pull/32/files#r2061309338, yamlParser and markdownParser should not be allowed to be null. So, this change removes that, specific, part of cd4c2dd.

@mnapoli

This reverts the change to CommonMarkParser in 803e158 allowing $parser to be both null and an instance of ConverterInterface, as pointed out by @mnapoli in mnapoli#32 (comment).

settermjd · 2025-05-01T12:01:51Z

Hey @mnapoli, any further feedback?

settermjd · 2025-08-08T01:21:54Z

Ping @mnapoli.

mnapoli · 2025-08-08T10:56:16Z

Hey @settermjd

Catching up with this, is my comment above not relevant anymore?

This is a BC break, we can't merge that as-is.

@mnapoli

Thank you to @mnapoli for pointing out that the original change would have been a BC break.

This change continues adding type declarations, where applicable, both in source and test code, and makes a few minor style changes.

settermjd · 2025-10-10T11:54:37Z

@mnapoli thanks for catching my mistake, earlier. I've added a further commit, adding further type support. Otherwise, I've left the code as is. I'm up for creating a further PR adding in more QA tooling, and improving the code styling.

mnapoli

I'm not sure what happened there but there are so many unrelated changes to code formatting that we don't want. I started reviewing but it's beyond just 1 file, please don't reformat the project for unrelated code changes.

mnapoli · 2025-10-10T12:18:35Z

src/Bridge/CommonMark/CommonMarkParser.php

-<?php declare(strict_types=1);
+<?php
+
+declare(strict_types=1);


Please don't

mnapoli · 2025-10-10T12:18:44Z

src/Bridge/CommonMark/CommonMarkParser.php

 * Bridge to the League CommonMark parser
 */
-class CommonMarkParser implements MarkdownParser
+final class CommonMarkParser implements MarkdownParser


Same please revert

mnapoli · 2025-10-10T12:18:51Z

src/Bridge/CommonMark/CommonMarkParser.php

+    public function __construct(MarkdownConverterInterface|null $commonMarkConverter = null)
    {
-        $this->parser = $commonMarkConverter ?: new CommonMarkConverter;
+        $this->parser = $commonMarkConverter ?: new CommonMarkConverter();


Please revert

mnapoli · 2025-10-10T12:19:15Z

src/Bridge/CommonMark/CommonMarkParser.php

    public function parse(string $markdown): string
    {
-        return $this->parser->convertToHtml($markdown)->getContent();
+        return $this->parser?->convertToHtml($markdown)?->getContent() ?? '';


The parser will never be null

settermjd added 5 commits April 26, 2025 22:09

Add complete type support

cd4c2dd

This change makes explicit use of types throughout the codebase wherever they were missing. In addition, it uses union types for nulls, as that's required in PHP 8.4.

Update the GitHub Workflow

ec93b3c

This change updates the GitHub Workflow to support only PHP 8.2 - 8.4.

mnapoli reviewed Apr 26, 2025

View reviewed changes

mnapoli reviewed Apr 27, 2025

View reviewed changes

src/Bridge/CommonMark/CommonMarkParser.php Outdated Show resolved Hide resolved

settermjd added 2 commits April 28, 2025 18:16

Revert addition of union type in Parser

6317532

As was pointed out by @mnapoli, https://github.com/mnapoli/FrontYAML/pull/32/files#r2061309338, yamlParser and markdownParser should not be allowed to be null. So, this change removes that, specific, part of cd4c2dd.

Revert allowing CommonMarkParser::parser to be null

7f27a4c

This reverts the change to CommonMarkParser in 803e158 allowing $parser to be both null and an instance of ConverterInterface, as pointed out by @mnapoli in mnapoli#32 (comment).

settermjd requested a review from mnapoli April 30, 2025 12:37

settermjd added 2 commits October 10, 2025 21:43

Revert BC break in CommonMarkParser

286d132

Thank you to @mnapoli for pointing out that the original change would have been a BC break.

Continue adding type support

ea7f166

This change continues adding type declarations, where applicable, both in source and test code, and makes a few minor style changes.

mnapoli requested changes Oct 10, 2025

View reviewed changes

mnapoli mentioned this pull request Nov 30, 2025

fix: Remove deprecation for league/commonmark #35

Closed

Conversation

settermjd commented Apr 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnapoli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

settermjd Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

settermjd commented Apr 27, 2025

Uh oh!

Uh oh!

settermjd commented May 1, 2025

Uh oh!

settermjd commented Aug 8, 2025

Uh oh!

mnapoli commented Aug 8, 2025

Uh oh!

settermjd commented Oct 10, 2025

Uh oh!

mnapoli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

settermjd commented Apr 26, 2025 •

edited

Loading

settermjd Apr 28, 2025 •

edited

Loading