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

Add PHP 8.4 support in a PHP >= 5.1 compatible way #900

Open
wants to merge 3 commits into
base: 1.7.x
Choose a base branch
from

Conversation

dregad
Copy link

@dregad dregad commented Jan 26, 2025

I believe mine is the better approach for the 1.7.x branch (which still officially supports PHP 5.3 and later) as it maintains backwards-compatibility for anyone still using PHP < 7.1.

I hope this can be merged quickly, and a new release tagged. Thanks in advance, and let me know if there is anything else I can do to speed that up.

This re-does @xabbuh's commit 7b307a9,
in a PHP >= 5.1 backwards-compatible way.
Changing the $Block parameter's default value from null to array()
caused the PHPUnit tests to fail with "Argument erusev#2 ($Block) must be of
type array, null given".

Initializing $CurrentBlock variable in lines() method to array() instead
of null, and change subsequent tests from isset() to !empty().

Also change blockCode() method signature, which was missed in previous
commit.
@dregad
Copy link
Author

dregad commented Jan 27, 2025

🟢 Build is passing for all PHP versions now

I'm not sure why the workflow is running on my fork and not here though, is that normal ?

@@ -712,9 +712,9 @@ protected function blockRule($Line)
#
# Setext

protected function blockSetextHeader($Line, array $Block = null)
protected function blockSetextHeader($Line, array $Block = array())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a BC break as the method can now longer be called with null as the second argument anymore.

@@ -850,9 +850,9 @@ protected function blockReference($Line)
#
# Table

protected function blockTable($Line, array $Block = null)
protected function blockTable($Line, array $Block = array())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@dregad
Copy link
Author

dregad commented Jan 27, 2025

@xabbuh you're absolutely right, I am well aware of that. BUT:

  1. it's only changing the signature of a protected method, not a public interface, so impact to the outside world is limited to extension classes that actually overload one or more of these 3 methods - blockCode(), blockSetextHeader() or blockTable() - so most likely a very small population
  2. the fix is very simple: just adapt the signature of overloaded function to match, and make sure you send the parent an empty array instead of null. So in my opinion the impact and risk are quite low, and with a clear message in the release notes, acceptable.

As an added side benefit, we have stronger typing (array instead of array|null).

For SemVer compliance, considering that a protected function is technically not a public API:

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced to the public API.

I believe this could be released as 1.8.0, with a clear message in the release notes. I am willing to draft it if you want.

In any case, there is absolutely no way to fix 8.4 compatibility without some minor BC break, so if you disagree with my proposal, at least take your pick from one of the alternatives:

Status quo is not an option.

@MyIgel
Copy link

MyIgel commented Jan 27, 2025

Regarding the 2.0.0 release: Is there anything that needs help / a plan going forward so that this could be an option too?

@dregad
Copy link
Author

dregad commented Feb 10, 2025

@erusev could this please be merged, and a new version released ? Let me know if I can help.

@Evgeny1973
Copy link

What are we waiting for?

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.

4 participants