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 returntype for DateIntervall::createFromDateString #2038

Merged

Conversation

verfriemelt-dot-org
Copy link
Contributor

this should fix phpstan/phpstan#8442
seems to be a simple fix, not sure if i done everything correct.

Copy link
Contributor

@staabm staabm left a comment

Choose a reason for hiding this comment

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

the snippet from phpstan/phpstan#8442 should be added to this PR as a NodeScopeResolverTest including proper assertType calls

@verfriemelt-dot-org
Copy link
Contributor Author

i will look into this! 👍

@verfriemelt-dot-org verfriemelt-dot-org force-pushed the bugfix/8442-wrong-return-type branch 2 times, most recently from 7b4b767 to 2095c78 Compare December 12, 2022 09:36
@staabm
Copy link
Contributor

staabm commented Dec 14, 2022

Would be great if you could adjust other similar existing extensions which also miss the getConstantStrings() handling, after this one was merged

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Also - you have some random spaces in there that I don't like but CS isn't picking up them as errors. Please get rid of them.

Like: if ( count or $strings[0 ].


public function getTypeFromStaticMethodCall(MethodReflection $methodReflection, StaticCall $methodCall, Scope $scope): ?Type
{
$strings = $scope->getType($methodCall->getArgs()[0 ]->value)->getConstantStrings();
Copy link
Member

Choose a reason for hiding this comment

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

This code will crash if someone has DateInterval::createFromDateString() without an argument in their code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, fixed that. but it leaves me wondering, what to return when this case is found?

		$arguments = $methodCall->getArgs();

		if (!isset($arguments[0])) {
			return null;
		}

i opted for the default type but running code like this with warnings supressed does not execute at all. so what would be the correct thing to do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i opted for this:

	assertType('DateInterval|false',DateInterval::createFromDateString());
	assertType('DateInterval|false',DateInterval::createFromDateString(new stdClass()));

src/Type/Php/DateIntervalDynamicReturnTypeExtension.php Outdated Show resolved Hide resolved
@verfriemelt-dot-org
Copy link
Contributor Author

Also - you have some random spaces in there that I don't like but CS isn't picking up them as errors. Please get rid of them.

Like: if ( count or $strings[0 ].

intresting, make cs-fix will not clean this up 🤔 i fixed them by hand :)

@ondrejmirtes
Copy link
Member

Thank you!

@verfriemelt-dot-org verfriemelt-dot-org deleted the bugfix/8442-wrong-return-type branch December 30, 2022 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants