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

Fix CS #506

Closed
wants to merge 1 commit into from
Closed

Fix CS #506

wants to merge 1 commit into from

Conversation

ondrejmirtes
Copy link
Contributor

Hi, I'm in the process of creating my own PHP 7.1-compatible fork (hope you're fine with that! It will land in PHPStan pretty soon I expect). I stumbled upon this build issue (it fails here on master), so I'm sending a fix :)

Thank you!

@Ocramius
Copy link
Member

Do you really need PHP 7.1 support, considering that it's EOL'd and out of security support? On this end, we'll likely keep compat only with the current minor, since PHP is an elusive target.

@@ -205,7 +205,7 @@ private function assertMemoization(
Reflector $reflector,
Identifier $identifier
) use (
& $fetchedSymbolsCount
&$fetchedSymbolsCount
Copy link
Member

Choose a reason for hiding this comment

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

This is potentially caused by doctrine/coding-standard:6.0.0 depending on PSR-12 rules that were changed in upstream phpcs. Upgrading to doctrine/coding-standard:7.0.2 should fix these, in theory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kukulich told me that he'll be updating BetterRefleciton to doctrine/coding-standard:7.x so I'll leave it up to him :)

@ondrejmirtes
Copy link
Contributor Author

Do you really need PHP 7.1 support, considering that it's EOL'd and out of security support?

I'm thinking pragmatically. There's nothing in PHP 7.2 that I'd take advantage of. It also breaks my heart when someone composer requires PHPStan and it installs PHPStan 0.9.2 (ancient version by now) because it's the last one that supports PHP 7.0. The user does not even know they're not on the latest version.

When it's cheap for me to support PHP 7.1, there isn't a reason not to. Funny thing - BetterReflection would still fully support PHP 7.1 if it weren't for the constraint in composer.json. The only thing I had to do to make the build pass was to downgrade PHPUnit to 7.5.18 :)

Also, thanks to PHAR-only distribution, I now have the advantage of a build step. I could theoretically develop PHPStan for PHP 7.4 only, and transform the sources to also support PHP 7.1 every time before publishing.

@Ocramius
Copy link
Member

When it's cheap for me to support PHP 7.1, there isn't a reason not to.

We dropped previous versions because it was not cheap for us to maintain them 🤷‍♀️

@ondrejmirtes
Copy link
Contributor Author

Yeah, but this does not apply now because of small jumps in 7.2 and 7.3. I also wanted void and nullable typehints so I dropped 7.0.

@Ocramius Ocramius added this to the 4.0.0 milestone Jan 10, 2020
@Ocramius Ocramius self-assigned this Jan 10, 2020
@Ocramius Ocramius added the bug label Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants