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

[2.0] Drop support for PHP 7.2 #625

Closed
colinodell opened this issue Jun 3, 2021 · 6 comments · Fixed by #627
Closed

[2.0] Drop support for PHP 7.2 #625

colinodell opened this issue Jun 3, 2021 · 6 comments · Fixed by #627
Assignees
Labels
implemented Change has been implemented
Milestone

Comments

@colinodell
Copy link
Member

PHP 7.2 is end-of-life and only 5% of our users currently use that version. PHP 7.3 also has a few features we might be able to leverage, like case-folding improvements. Let's therefore drop support for 7.2 in the next major release.

Anyone stuck on 7.1 or 7.2 may continue using 1.x until they are able to upgrade.

@colinodell colinodell added the do not close Issue which won't close due to inactivity label Jun 3, 2021
@colinodell colinodell added this to the v2.0 milestone Jun 3, 2021
@colinodell colinodell self-assigned this Jun 3, 2021
@markhalliwell
Copy link
Contributor

This would definitely be helpful for #546

@colinodell colinodell added implemented Change has been implemented and removed do not close Issue which won't close due to inactivity labels Jun 5, 2021
@GrahamCampbell
Copy link
Member

I don't think it would be unreasonable to make version 2.0 PHP 8 only. Version 1.0 is rock solid and people who need to stay behind on PHP 7 for a bit can keep using that. Related: Symfony 6 will be PHP 8 only.

@colinodell
Copy link
Member Author

You're not wrong, but this library is used by many Drupal sites and Drupal will support PHP 7.3 until they release Drupal 10 next summer.

@markhalliwell I'll defer to you on this - do you feel comfortable with us going to PHP 7.4 or 8.0 for the 2.0 release? Or would you prefer that we stick with 7.3+ for now?

(I do suspect I'll be releasing version 3.0 in a year or so, and that will definitely be PHP 8 only)

@colinodell
Copy link
Member Author

Also, I plan to tag 2.0.0beta1 once I get feedback on the PHP version question 😉

@markhalliwell
Copy link
Contributor

It should be fine to require PHP 8 for 2.0.

The Drupal Markdown module is already setup to handle specific library requirements.

The only requirement it uses for libraries like CommonMark is which major version it currently supports; specific PHP requirements are indirectly indicated by the requested version of the library's composer.json:

https://git.drupalcode.org/project/markdown/-/blob/8.x-2.x/src/Plugin/Markdown/CommonMark/CommonMark.php#L37-39

 *         @InstallableRequirement(
 *           constraints = {"Version" = ">=0.4.0 <=1.0.0 || ^1.0 || ^2.0"}
 *         ),

We even do this with the bundled extensions. Take the Mention extension for example, while we support it, it isn't actually useable unless the installed CommonMark version is 1.5 or higher:

https://git.drupalcode.org/project/markdown/-/blob/8.x-2.x/src/Plugin/Markdown/CommonMark/Extension/MentionExtension.php#L27-31

 *          @InstallableRequirement(
 *             id = "parser:commonmark",
 *             callback = "::getVersion",
 *             constraints = {"Version" = "^1.5 || ^2.0"},
 *          ),

Like @GrahamCampbell said, if people have lower PHP requirements then they can use the older versions until they want/can upgrade. This is how sites that are still using PHP 5 are doing it, they just haven't been able to upgrade to the latest CommonMark versions with all the new fancy features 😄

IMHO, that is more than fair and honestly what Semver was designed to do from the get-go.

@colinodell
Copy link
Member Author

Oh, that's awesome! Thanks for that feedback.

#671 :)

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

Successfully merging a pull request may close this issue.

3 participants