Update minimum requirements and phpunit #357
Conversation
Ending PHP support has nothing to do with supported PHP versions by this project. This project still works with PHP 5.3 so the provided version is correct. |
|
Scrutinizer tests are failing because it tests things with PHP 7.0.x. Perhaps you should edit the Scrutinizer configuration file as part of this pull request. @paxter is right: we can still support down to PHP 5.3, although I'll admit it's probably not that useful at this point. Are you proposing this change because you're planning on doing things that depend on a newer version of PHPUnit? |
|
Tests fail with php 8, because phpunit is so old. So the intent of this PR
was to support the more recent versions of PHP (the library itself works
fine, but not the tests).
I'd like to see requiring PHP 7.4+, so we could use typehints in the
methods. That way, libraries that overwrite the methods (like
KnpMarkdownBundle) wouldn't show warnings for not including the typehints.
For example, function detab($text) {... could be function detab(string
$text): string {...
With php8.1 arriving later this month, and security support for 7.3 ending
in 29 days (active support for 7.3 ended almost a year ago), it doesn't
seem bad to make 7.4 a minimum requirement.
…On Fri, Nov 5, 2021 at 8:04 PM Michel Fortin ***@***.***> wrote:
Scrutinizer tests are failing
<https://scrutinizer-ci.com/g/michelf/php-markdown/inspections/3289af11-a6dd-4631-a6f5-5bd76636b2be>
because it tests things with PHP 7.0.x. Perhaps you should edit the
Scrutinizer configuration file as part of this pull request.
@paxter <https://github.com/paxter> is right: we can still support down
to PHP 5.3, although I'll admit it's probably not that useful at this
point. Are you proposing this change because you're planning on doing
things that depend on a newer version of PHPUnit?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#357 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEXIQKTQLQQSWJL7NPMO5LUKR5LVANCNFSM5HOJOUNQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Hmm. This might be a worthwhile reason to modernize a bit. Phpunit not running anymore on 8.0 could introduce risk on any future changes made. Personally I think it could also invigorate more involvement in the project. Remember that current versions on the library will continue to work with old PHP versions forever, just not future changes. It's just a matter of bumping the major composer version. |
|
Ok, let's do this. Type hints sound enticing, so 7.4. |
|
OK, I'll submit a PR in a bit.
…On Sat, Nov 6, 2021 at 10:45 AM Michel Fortin ***@***.***> wrote:
Ok, let's do this. Type hints sound enticing, so 7.4.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#357 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEXIQPDCTN2D6FMA74SKTDUKVES5ANCNFSM5HOJOUNQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
So I bumped this to 7.4 in my repo, and I can submit a PR, but I guess
before I do that let's brainstorm a bit about some other things we can do
besides something so trivial.
* Replace travis with github tasks
* This library has no dependencies, which is cool, but the Symfony String
component (https://symfony.com/doc/current/components/string.html) is worth
considering.
Thoughts?
…On Sat, Nov 6, 2021 at 11:22 AM Tac Tacelosky ***@***.***> wrote:
OK, I'll submit a PR in a bit.
On Sat, Nov 6, 2021 at 10:45 AM Michel Fortin ***@***.***>
wrote:
> Ok, let's do this. Type hints sound enticing, so 7.4.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#357 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAEXIQPDCTN2D6FMA74SKTDUKVES5ANCNFSM5HOJOUNQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
|
|
Instead of moving to github actions we could just migrate to travis-ci.com (instead of .org) which should be a no-code change, @michelf just has to log in to travis-ci.com and click a few buttons to activate it on this repo (it'll see the existing travis.yml and nothing else is needed). I guess the old This is just my personal opinion, but something doesn't sit right with doing everything under the GitHub (now Microsoft) umbrella... who knows how long it will be free for free/open source projects. |
|
@michaelbutler I'm not really following Travis-CI related news, but it looks like .org is more or less shutting down and .com is the replacement. And .com wants me to pick a plan and link a credit card, so I'm not sure I want to continue using Travis-CI. |
|
well if travis .com is asking for a credit card my recommendation is not to use it 😄 I was pretty sure it is free but if they require a credit card (maybe to thwart spam abuse) that's too bad. GitHub actions is probably the way to go then. |
PHP less than 7.2 is no longer supported (and 7.2 will be at EOL in a few weeks).
Bumping to PHP 7 also allows a more current phpunit.