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

Restore PHP 7.4 compatibility #994

Merged
merged 5 commits into from
Apr 8, 2022
Merged

Restore PHP 7.4 compatibility #994

merged 5 commits into from
Apr 8, 2022

Conversation

danepowell
Copy link
Contributor

@danepowell danepowell commented Mar 21, 2022

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

#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
Copy link
Contributor Author

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.

@danepowell danepowell mentioned this pull request Mar 21, 2022
9 tasks
@veewee
Copy link
Contributor

veewee commented Mar 21, 2022

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
Copy link
Contributor

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

@danepowell
Copy link
Contributor Author

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

@veewee
Copy link
Contributor

veewee commented Mar 24, 2022

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
Copy link
Contributor

veewee commented Mar 25, 2022

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Nailed it. @veewee this is good to go.

$windowsIsInsane

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

@veewee
Copy link
Contributor

veewee commented Mar 31, 2022

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 veewee added this to the 1.11.0 milestone Apr 8, 2022
@veewee veewee merged commit b86398c into phpro:master Apr 8, 2022
@veewee
Copy link
Contributor

veewee commented Apr 8, 2022

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

@danepowell
Copy link
Contributor Author

Wonderful, thanks!

@WCLLTD
Copy link

WCLLTD commented Jun 1, 2023

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
Copy link
Contributor

veewee commented Jun 1, 2023

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
Copy link

WCLLTD commented Jun 1, 2023

hey @veewee

thanks for the quick reply! Sorry I did just find a reply confirming this before, sorry for wasting your time here..
#1053 (comment)

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!

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

Successfully merging this pull request may close these issues.

4 participants