Skip to content

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Dec 2, 2024

Working on something in FunctionCallParametersCheck which is tested via CallToFunctionParametersRuleTest but realised that the PHP version was hard-coded to 8.0 which is blocking me there. Also: bigger range of tests over multiple PHP versions, yay :) in general there seem to be many rules where similar things could be done.

Unfortunately this causes a small conflict in 2.0.x apparently.

UPDATE: Markus made me aware of #3662 and I adapted the named arguments test to be skipped exactly the same way to avoid troubles there.

@herndlm herndlm force-pushed the dynamic-php-version-call-to-function-parameters-rule branch 4 times, most recently from 2d44ec4 to ed8b686 Compare December 2, 2024 14:17
@herndlm herndlm marked this pull request as ready for review December 2, 2024 14:23
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@herndlm
Copy link
Contributor Author

herndlm commented Dec 2, 2024

ah, I just saw that CallMethodsRuleTest uses the current PHP version dynamically, but for the named arguments test it forces it to 8.0. Which method are we preferring? I see it has a disallow-named-arguments.php test, maybe that's the way to go then. Or something like marking the test skipped if the PHP version is too low? 🤔 😅

for the bug fix / feature PR I'm preparing it doesn't matter. I just need to be able to have a dynamic PHP version in this test file.

@staabm
Copy link
Contributor

staabm commented Dec 2, 2024

Please note: we are working on the same files

#3662

@herndlm
Copy link
Contributor Author

herndlm commented Dec 2, 2024

Please note: we are working on the same files

#3662

Oh, not good.. The scope dependent php version functionality is 2.x only, right? I'm also OK with the skipping approach of course, the constructor is conflicting any way..

@herndlm herndlm force-pushed the dynamic-php-version-call-to-function-parameters-rule branch from ed8b686 to 2217bcb Compare December 2, 2024 19:47
@staabm
Copy link
Contributor

staabm commented Dec 2, 2024

The scope dependent php version functionality is 2.x only, right?

yes

I'm also OK with the skipping approach of course, the constructor is conflicting any way..

no its fine. lets see which one is merged earlier :)

@herndlm herndlm force-pushed the dynamic-php-version-call-to-function-parameters-rule branch from 2217bcb to a8146c8 Compare December 2, 2024 19:48
@staabm
Copy link
Contributor

staabm commented Dec 2, 2024

ahh this looks great now, thanks

@herndlm
Copy link
Contributor Author

herndlm commented Dec 2, 2024

ahh this looks great now, thanks

for a moment I thought I broke gitlab though with all my back and forth .. xD apparently it wasn't happy with me force-pushing back to an old state hmm. anyway, closer to yours now, yeah

Screenshot From 2024-12-02 20-53-31

@herndlm
Copy link
Contributor Author

herndlm commented Dec 5, 2024

btw I have a PR prepared which adapts a rule dealing with an high-upvoted bug. but it would need this small test change first. would be great if this can be merged :)

@herndlm
Copy link
Contributor Author

herndlm commented Dec 20, 2024

@ondrejmirtes after this I was planning to fix phpstan/phpstan#11418, or do you consider that to be a new feature, then I can just skip this and work on 2.x after #3662 was merged.

I don't want to pressure you, just trying to figure out what the best approach is to avoid conflicts/issues.

@ondrejmirtes
Copy link
Member

This #3662 basically fixes the same problem, so in order to avoid conflicts, I'm closing this. Thanks.

@herndlm herndlm deleted the dynamic-php-version-call-to-function-parameters-rule branch December 20, 2024 09:05
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