danepowell commented 2 years ago

Q	A
Branch	master for features and deprecations
Bug fix?	yes/no
New feature?	yes/no
BC breaks?	no
Deprecations?	no
Documented?	yes
Fixed tickets	#957

https://github.com/phpro/grumphp/pull/956 introduced PHP 8 compatibility. However, it also removed PHP 7.4 compatibility for no reason that I can see. Since PHP 7.4 and 8 are both still officially maintained, this package should support both. It's important that they are supported simultaneously in a single release so that downstream projects with fixed dependencies can run on both PHP 7.4 and 8.

New Task Checklist:

[ ] Are the dependencies added to the composer.json suggestions?
[ ] Is the doc/tasks.md file updated?
[ ] Are the task parameters documented?
[ ] Is the task registered in the tasks.yml file?
[ ] Does the task contains phpunit tests?
[ ] Is the configuration having logical allowed types?
[ ] Does the task run in the correct context?
[ ] Is the run() method readable?
[ ] Is the run() method using the configuration correctly?
[ ] Are all CI services returning green?

danepowell commented 2 years ago

The test failures must not be related to this PR, since they are failing even for PHP 8.1 and I didn't touch any functional code.

veewee commented 2 years ago

Looks like a newer dependency is breaking the tests. Not sure which one ATM.

However the once on php74 fail consistently over both lower and high dependencies. Looks like we are using >php74 syntax. Not sure if it's worth removing that syntax again

yguedidi commented 2 years ago

here is the new syntax, the mixed type hint: https://github.com/phpro/grumphp/blob/master/src/Task/TwigCs.php#L60

danepowell commented 2 years ago

I just moved the type hint to a docblock, which keeps all of the same functionality in a backwards-compatible way.

veewee commented 2 years ago

Alright, looks good! Thanks. Gonna keep it open until the latest deps issue gets fixed Probably related to new phpspec / prophecy versions that got released past week, which was still open for full php 81 support

veewee commented 2 years ago

@danepowell Cant figure out why the functional tests fail on windows PHP74 and if it is actually an issue or rather a failure on appveyor.

Maybe you have more luck with it?

danepowell commented 2 years ago

That's a mystery. I suspect it's a race condition of some kind in the Windows tests, can you "jiggle the handle" (restart tests a few times) and see if it changes?

The log up until the failures is normal, it even shows "GrumPHP is sniffing your code!", so the hooks should be present.

danepowell commented 2 years ago

I'm going to try to spin up a Windows VM locally to reproduce this. @veewee is there a way to get verbose output from tests, i.e. see what all of the fixture creation commands are doing? That would speed up debugging.

danepowell commented 2 years ago

Nailed it. @veewee this is good to go.

$windowsIsInsane

After spending hours debugging this, I couldn't agree more 😄

veewee commented 2 years ago

Glad to share my pain :) You can't imagine how much time I spent fixing windows madness in this repo ... 😅

Will look at the details of this PR later

veewee commented 2 years ago

Thanks for your work on this! Will keep on supporting 7.4 until EOL.

danepowell commented 2 years ago

Wonderful, thanks!

WCLLTD commented 1 year ago

hey! It looks like this issue may have cropped up again?

Across my Magento projects still using 7.4, I can't seem to install (tried on 5/6 different projects and same result each time) - it works fine on my 8.1 projects.

phpversion

PHP version 7.4.33 is unsupported

Thank you for keeping the code quality better. We appreciate your patience and efforts. Happy Coding!!

sam@WCL-L-011:~/sites/project-repo-m2(feature/grumphp-install-stag-2)$ cat grumphp.yml | tail -2 phpversion: project: '7.4'

Is this going to be stemming from the fact I'm using Ubuntu on a WSL install? Or was the Windows error in context of running in Windows itself? (which I'm not doing, so not sure if I've got an entirely different issue?)

for further context, I'm using the phpro/grumphp-shim composer package, for the dependancies.

Sorry for digging up old issue!

veewee commented 1 year ago

Hello @WCLLTD ,

This package now has a minimum of PHP 8.0. We follow the supported PHP versions and won't go any further than that. https://www.php.net/supported-versions.php

Nevertheless: GrumPHP has been stable for years now. So you can still use an older version of GrumPHP that match your project's needs.

WCLLTD commented 1 year ago

hey @veewee

thanks for the quick reply! Sorry I did just find a reply confirming this before, sorry for wasting your time here.. https://github.com/phpro/grumphp/pull/1053#issuecomment-1327649497

I will look at reviewing this further - will likely go back to an older release for the remaining 7.4 projects (which makes my want to move off 7.4 even more!!)

thank you!

phpro / grumphp

Restore PHP 7.4 compatibility #994

New Task Checklist: