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

Supports PHPUnit 11 #109

Merged
merged 7 commits into from
May 6, 2024
Merged

Conversation

zonuexe
Copy link
Contributor

@zonuexe zonuexe commented May 2, 2024

First of all, thank you for providing this package!
This was very helpful for us migrating from PHPUnit 9 to 10.

And we are trying to update to PHPUnit 11 so we will submit a PR.

composer.json Show resolved Hide resolved
Copy link
Owner

@michaelpetri michaelpetri left a comment

Choose a reason for hiding this comment

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

Nice additions, thank you 💪 🤗

@michaelpetri
Copy link
Owner

Seems like there are some issues with the lock file, as soon the ci is green, i can merge and release it.

@zonuexe
Copy link
Contributor Author

zonuexe commented May 2, 2024

@michaelpetri Thank you for suggestion. I fixed and squashed it.

@michaelpetri
Copy link
Owner

I think we only need to drop support for php 8.1 and everything will be green 🤞

@zonuexe
Copy link
Contributor Author

zonuexe commented May 3, 2024

Dropping 8.1 makes sense since it is still possible to install past versions of phpunit-consecutive-arguments, but it is somewhat unfriendly for users.

Here we can work around the laminas-continuous-integration-action constraint by removing composer.lock before unit testing in 8.1.

I tested it here and it seems to work: zonuexe#1

@zonuexe zonuexe requested a review from michaelpetri May 3, 2024 07:24
@michaelpetri
Copy link
Owner

I think this doesn't make that much sense since laminas action checks against lowest, locked and highest versions by design? I really don't see any problem to drop 8.1, it's totally okay to only support only the latest two major versions. Also keep in mind, this lib has not that much users and the current 0.2 version is only used by php 8.2 users. So let's just drop it before introducing some kind of CI hacks here! :)

https://packagist.org/packages/michaelpetri/phpunit-consecutive-arguments/php-stats

@zonuexe
Copy link
Contributor Author

zonuexe commented May 3, 2024

@michaelpetri You're right. I pushed the fix.

psalm.xml Show resolved Hide resolved
@zonuexe
Copy link
Contributor Author

zonuexe commented May 3, 2024

@michaelpetri
This failure seems to be caused by problems with laminas-continuous-integration-action and psalm. I'm trying to submit issues and PRs to each project, and it looks like phpunit-consecutive-arguments can be merged if Psalm passes locally.

refs laminas/laminas-ci-matrix-action#301

@michaelpetri
Copy link
Owner

I think we can just exclude the local xsd from our config. I'm currently on a train with a weak to zero Internet connection, will check later or tomorrow.

@zonuexe
Copy link
Contributor Author

zonuexe commented May 3, 2024

Thank you for your work.

This is because the schema path is embedded in laminas-continuous-integration-action, so from psalm.xml of this project xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" does not solve this problem.

@zonuexe
Copy link
Contributor Author

zonuexe commented May 5, 2024

A temporary workaround was provided by laminas-ci-matrix-action 1.26.0.
Please see laminas/laminas-ci-matrix-action#301 (comment).

@michaelpetri
Copy link
Owner

I'm not a big fan of this workaround, but let's not block this any longer so you can keep working on your project :D

I'm very sorry I didn't reply over the weekend, my phone decided to kill itself and I was busy with personal things. Thanks for all of your effort 💪 🤗

@michaelpetri michaelpetri merged commit 79e733a into michaelpetri:main May 6, 2024
9 checks passed
@zonuexe zonuexe deleted the supports-phpunit-11 branch May 6, 2024 09:31
@zonuexe
Copy link
Contributor Author

zonuexe commented May 6, 2024

Thank you very much for your cooperation!

I'm not a big fan of this workaround

me too. These should be improved by fixes from upstream projects!

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.

None yet

2 participants