NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Add types to public and protected properties#51069

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:7.0fromnicolas-grekas:typed-props

Jul 28, 2023

Merged

Add types to public and protected properties#51069

nicolas-grekas merged 1 commit intosymfony:7.0fromnicolas-grekas:typed-props

Jul 28, 2023

Conversation

Copy link

Member

nicolas-grekas commentedJul 21, 2023•
edited
Loading

Q	A
Branch?	7.0
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

🥵

Allowed by#45360

Follows#51068 and#51067

nicolas-grekas requested review fromOskarStark,chalasr,xabbuh andyceruto ascode owners

July 21, 2023 17:18

carsonbot added Status: Needs Review Bug labels

Jul 21, 2023

carsonbot added this to the7.0 milestone

Jul 21, 2023

nicolas-grekas added Feature and removed Bug labels

Jul 21, 2023

94noni reviewed

Jul 21, 2023

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Translation/Translator.phpShow resolvedHide resolved

nicolas-grekas force-pushed thetyped-props branch from77d0ede to803a018Compare

July 23, 2023 09:48

nicolas-grekas requested review fromdunglas,lyrixx andwouterj ascode owners

July 23, 2023 09:48

derrabus requested changes

Jul 23, 2023

View reviewed changes

Copy link

Member

derrabus left a comment

Choose a reason for hiding this comment

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

Reviewing this PR is exhausting. I've reviewed up to the HttpFoundation component and will take a break now. 😓

src/Symfony/Component/Config/Definition/BaseNode.php OutdatedShow resolvedHide resolved

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.phpShow resolvedHide resolved

src/Symfony/Bundle/FrameworkBundle/Test/KernelTestCase.php OutdatedShow resolvedHide resolved

src/Symfony/Component/BrowserKit/Cookie.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Cache/Adapter/PdoAdapter.php OutdatedShow resolvedHide resolved

src/Symfony/Component/HttpFoundation/StreamedResponse.phpShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Jul 23, 2023

nicolas-grekas force-pushed thetyped-props branch 13 times, most recently from55de9b0 tobd629d9Compare

July 26, 2023 15:22

nicolas-grekas force-pushed thetyped-props branch frombd629d9 to65437fdCompare

July 26, 2023 15:35

nicolas-grekas mentioned this pull request

Jul 26, 2023

More short closures + isset instead of null checks + etc.#51121

Merged

Copy link

MemberAuthor

nicolas-grekas commentedJul 26, 2023

PR is ready for review. Needs#51121 to be 🍏

derrabus reviewed

Jul 26, 2023

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Routing/Router.phpShow resolvedHide resolved

src/Symfony/Component/Config/Definition/Builder/NodeBuilder.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php

		protectedmixed$trueEquivalent =true;
		protectedmixed$falseEquivalent =false;
		protectedstring$pathSeparator = BaseNode::DEFAULT_PATH_SEPARATOR;
		protectedNodeParentInterface\|NodeInterface\|null$parent;

Copy link

Member

Choose a reason for hiding this comment

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

The return type ofend() is wider. My gut feeling is that they should match.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

All those classes in the return type ofend() implementNodeParentInterface so we could reduce this to only it. I suppose the list is here to hint the IDE for autocompletion. Maybe we should keep onlyNodeParentInterface as native type and use@return for the rest?

The other strange thing is about the need to addNodeInterface to make this code work:

symfony/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php

Lines 352 to 361 in1935af6

	$node =newArrayNode($this->name,$this->parent,$this->pathSeparator);

	$this->validateConcreteNode($node);

	$node->setAddIfNotSet($this->addDefaults);

	foreach ($this->childrenas$child) {
	$child->parent =$node;
	$node->addChild($child->getNode());
	}

end() can't return anArrayNode because of its return type. Either the return type is wrong, or what?
Up to investigate and fix this on a lower branch?

Copy link

Member

derrabusJul 27, 2023

Choose a reason for hiding this comment

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

All those classes in the return type ofend() implementNodeParentInterface so we could reduce this to only it. I suppose the list is here to hint the IDE for autocompletion. Maybe we should keep onlyNodeParentInterface as native type and use@return for the rest?

Would work for me to unblock the PR.

Up to investigate and fix this on a lower branch?

Maybe, but not right away. 🙈

Copy link

MemberAuthor

nicolas-grekasJul 27, 2023

Choose a reason for hiding this comment

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

use@return for the rest

PR updated

Copy link

Member

stofJun 4, 2025

Choose a reason for hiding this comment

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

The return type was not wrong. ArrayNode is not a NodeDefinition.

nicolas-grekas added a commit that referenced this pull request

Jul 27, 2023

minor#51121More short closures + isset instead of null checks + etc…

1935af6

…. (nicolas-grekas)This PR was merged into the 6.4 branch.Discussion----------More short closures + isset instead of null checks + etc.| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -Some of the isset() checks are needed to make#51069 green on high-deps job.Commits-------3df6471 More short closures + isset instead of null checks + etc.

nicolas-grekas force-pushed thetyped-props branch 3 times, most recently frome3eabd6 to39b070cCompare

July 27, 2023 07:52

nicolas-grekas mentioned this pull request

Jul 27, 2023

Ensure all properties have a type#51129

Merged

nicolas-grekas added a commit that referenced this pull request

Jul 27, 2023

minor#51129Ensure all properties have a type (nicolas-grekas)

fa38296

This PR was merged into the 6.4 branch.Discussion----------Ensure all properties have a type| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -Before#51069Commits-------25a8615 Ensure all properties have a type