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

feat: adds conditional return types to some global helper functions #1260

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

canvural
Copy link
Collaborator

@canvural canvural commented Jun 2, 2022

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

This PR adds conditional return types to some global helper functions. Replaces some of the existing extensions we have.

Breaking changes
n/a

@canvural canvural merged commit 0517e9a into master Jun 2, 2022
@canvural canvural deleted the conditional-return-type-stubs branch June 2, 2022 17:16
@paras-malhotra
Copy link

@canvural, I believe with this PR, it is not possible to change stubs. So, if I have a Request.stub file in my project, it will still pick up the Request.stub file from Larastan.

Related: #1265

@szepeviktor
Copy link
Collaborator

Could you share your stub and the relevant config lines?

@paras-malhotra
Copy link

paras-malhotra commented Jun 11, 2022

Here is the stub: https://github.com/enlightn/enlightn/blob/1937be5e6bd3142dfd03de90927ddd571f16477c/stubs/Request.stub

And the config lines: https://github.com/enlightn/enlightn/blob/1937be5e6bd3142dfd03de90927ddd571f16477c/phpstan.neon#L19-L20

Note: To add Larastan 2.x support to Enlightn, we needed to specify nunomaduro/larastan": "^2.0 <= 2.1.6 in enlightn/enlightn#117 because it doesn't work with 2.1.7 and onwards

@szepeviktor
Copy link
Collaborator

szepeviktor commented Jun 11, 2022

Does it succeed if you overwrite vendor/nunomaduro/larastan/stubs/Http/Request.stub ?
phpstan clear-result-cache first!

@paras-malhotra
Copy link

paras-malhotra commented Jun 11, 2022

@szepeviktor, yes it does. Overwriting the Larastan Request.stub file in the vendor/nunomaduro/larastan directory succeeds in the latest version of Larastan

@szepeviktor
Copy link
Collaborator

szepeviktor commented Jun 11, 2022

I suggest you to create a 1-class repo with 2 simple stubs and open an issue in @phpstan.

@paras-malhotra
Copy link

Sure, thanks. I opened phpstan/phpstan#7461

@szepeviktor
Copy link
Collaborator

szepeviktor commented Jun 11, 2022

I opened phpstan/phpstan#7461

Before running PHPStan: cp -f Your.stub Larastan.stub :(

@paras-malhotra
Copy link

Can we not go back to the stubFiles configuration in Larastan rather than using a custom stubFilesExtension? It will fix this change in behaviour.

@szepeviktor
Copy link
Collaborator

@canvural can tell that.

@szepeviktor
Copy link
Collaborator

szepeviktor commented Jun 11, 2022

@paras-malhotra Have you tried pushing in your custom stubs file by stubFilesExtension?

@paras-malhotra
Copy link

@paras-malhotra Have you tried pushing in your custom stubs file by stubFilesExtension?

No, I haven't tried that yet. I am assuming you mean adding a stubFilesExtension service in Enlightn?

This code seems to just append the stub files from all extensions, so I'm not sure that would work.

@canvural
Copy link
Collaborator Author

Hi,

Multiple stub file for one class were never supported by PHPStan. So in previous versions your code was working "by accident"

Can we not go back to the stubFiles configuration in Larastan rather than using a custom stubFilesExtension?

Stubs in Larastan are growing. Using stubFiles we'd had to keep extension.neon in sync with the new stubs. I do not want to maintain that anymore. stubFilesExtension seemed like a good solution.

On the other hand maybe we can go back to it if this is implemented.

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

3 participants