NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

Add scalar typehints/return types#23262

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

fabpot merged 2 commits intosymfony:masterfromchalasr:dotenv4-typehints

Sep 3, 2017

Merged

Add scalar typehints/return types#23262

fabpot merged 2 commits intosymfony:masterfromchalasr:dotenv4-typehints

Sep 3, 2017

Conversation

Copy link

Member

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no (final, already breaks if doc not respected)
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23242 (comment)
License	MIT
Doc PR	n/a

Choose a reason for hiding this comment

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

phpdoc maybe also requires update

Copy link

MemberAuthor

chalasrJun 22, 2017•
edited
Loading

Choose a reason for hiding this comment

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

in fact the type should bearray, no need for supporting Traversable. fixed

Copy link

Contributor

Choose a reason for hiding this comment

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

imo it would be better to haveiterable

Copy link

Contributor

Choose a reason for hiding this comment

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

If PHP7.1, theniterable is a solution. But it depends, If you really want an array, it's not the same as requiring an iterable. For example, if you need tocount the content, you can't rely onTraversable. So it really depends on the situation

chalasr force-pushed thedotenv4-typehints branch from54b8f7b to779c38cCompare

June 22, 2017 10:47

stof approved these changes

Jun 22, 2017

View reviewed changes

ogizanagi approved these changes

Jun 22, 2017

View reviewed changes

Taluu reviewed

Jun 22, 2017

View reviewed changes

src/Symfony/Component/Dotenv/Dotenv.php Outdated

		@@ -45,7 +45,7 @@
		* @throws FormatException when a file has a syntax error
		* @throws PathException when a file does not exist or is not readable
		*/
		public function load($path, ...$paths)
		public function load($path,string...$paths): void

Copy link

Contributor

TaluuJun 22, 2017

Choose a reason for hiding this comment

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

why notstring on the required argument ?

Copy link

MemberAuthor

chalasrJun 22, 2017

Choose a reason for hiding this comment

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

fixed

src/Symfony/Component/Dotenv/Dotenv.php

		@@ -65,7 +65,7 @@ public function load($path, ...$paths)
		*
		* @param array $values An array of env variables

Copy link

Contributor

TaluuJun 22, 2017

Choose a reason for hiding this comment

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

imo it would be better to haveiterable

chalasr force-pushed thedotenv4-typehints branch from779c38c to9cac16dCompare

June 22, 2017 22:33

jvasseur reviewed

Jun 23, 2017

View reviewed changes

src/Symfony/Component/Dotenv/Dotenv.php Outdated

		@@ -88,7 +88,7 @@ public function populate($values)
		*
		* @throws FormatException when a file has a syntax error
		*/
		public function parse($data, $path = '.env')
		public function parse(string$data, string $path = '.env')

Copy link

Contributor

jvasseurJun 23, 2017

Choose a reason for hiding this comment

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

Can we add the return type here ?

Copy link

MemberAuthor

chalasrJun 23, 2017

Choose a reason for hiding this comment

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

done

chalasr force-pushed thedotenv4-typehints branch from9cac16d to115928fCompare

June 23, 2017 12:29

Copy link

Member

fabpot commentedJun 23, 2017

What about other similar cases in the code base. I don't see why we should make the change here and not for similar use cases.

Copy link

Member

stof commentedJun 23, 2017

@fabpot this component is easy to change, because everything is final.

For other components, we could start using them in constructors, but mostly nothing else (returns types could be added later, once we have PHP 7.2 as min requirement)

chalasr force-pushed thedotenv4-typehints branch from4fe13fd tob2c916fCompare

June 23, 2017 16:29

Copy link

MemberAuthor

chalasr commentedJun 23, 2017

@fabpot @stof I found a few cases where making the change is as easy (final, no contract, no parent). Seeb2c916f.

Copy link

Member

xabbuh commentedJun 26, 2017

Internal class and methods could be easy to update to (seehttps://github.com/symfony/symfony/compare/master...xabbuh:yaml-scalar-type-hints?expand=1 for how this could look like in the Yaml component).

Copy link

Member

fabpot commentedJul 3, 2017

I like@xabbuh changes.@chalasr Do you have time to make those changes across the board when possible without BC breaks.

chalasr changed the title~~[Dotenv] Add scalar typehints/return types~~Add scalar typehints/return types

Jul 3, 2017

nicolas-grekas added this to the4.0 milestone

Jul 3, 2017

Copy link

MemberAuthor

chalasr commentedJul 3, 2017

Sure.
Status: needs work

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

Jul 3, 2017

Copy link

Member

xabbuh commentedJul 3, 2017•
edited
Loading

private methods could be candidates too

chalasr force-pushed thedotenv4-typehints branch 7 times, most recently from5717ecb tod2b8d38Compare

July 8, 2017 14:44

ogizanagi approved these changes

Aug 5, 2017

View reviewed changes

Tobion reviewed

Aug 5, 2017

View reviewed changes

src/Symfony/Component/Yaml/Parser.php Outdated

		@@ -178,9 +178,8 @@ private function doParse($value, $flags)
		$context = 'mapping';

		// force correct settings
		Inline::parse(null,$flags, $this->refs);
		Inline::initialize($flags, $this->getRealCurrentLineNb());

Copy link

Contributor

TobionAug 5, 2017

Choose a reason for hiding this comment

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

getRealCurrentLineNb is also marked as internal. Why not add return type there?

Copy link

Contributor

Tobion commentedAug 5, 2017

👍 To do this.

Copy link

Contributor

mnapoli commentedAug 5, 2017

As it is now there are still a lot of useful comments (in the phpdoc) that are removed in this PR, it might be better to preserve all comments that bring additional information?

Copy link

MemberAuthor

chalasr commentedAug 6, 2017

@mnapoli I tried to remove comments only where naming+typehints are clear enough. For instance,FirewallConfig methods returning?string were documented as@return string|null The firewall's xxx if configured, null otherwise. The class name says that these values are parsed configuration, the return type says it can be null. Wouldn't the comment be redundant? I think so.

There are not that much removed comments actually, would you mind commenting on the ones that should be kept to you?

mnapoli reviewed

Aug 6, 2017

View reviewed changes

Copy link

Contributor

mnapoli left a comment

Choose a reason for hiding this comment

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

@chalasr OK I've reviewed the diff and "a lot" was probably exaggerated sorry ^^

TheFirewallConfig class was the main problem I had, but it seems it's obvious that everything is a service ID there so maybe all those docblocks are not so useful indeed. In the end when you ignore that class there are just a few spots that I've noted as questions (also the@throws that were removed might be worth preserving).

src/Symfony/Component/CssSelector/Parser/Parser.php

		@@ -128,16 +121,7 @@ private function parseSelectorList(TokenStream $stream)
		return $selectors;
		}

		/**
		* Parses next selector or combined node.

Copy link

Contributor

mnapoliAug 6, 2017

Choose a reason for hiding this comment

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

This comment might be useful? (I don't know the class so might be wrong)

The same applies to the other methods of that class, but if it's expected that all methods parse the "next […]" then feel free to ignore my comment.

Copy link

MemberAuthor

chalasrAug 7, 2017

Choose a reason for hiding this comment

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

yes, applies to all methods

src/Symfony/Component/CssSelector/XPath/Translator.php Outdated

		*
		* @return Extension\ExtensionInterface
		*
		* @throws ExpressionErrorException

Copy link

Contributor

mnapoliAug 6, 2017

Choose a reason for hiding this comment

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

Should the@throws be kept?

src/Symfony/Component/CssSelector/XPath/Translator.php Outdated

		*
		* @return XPathExpr
		*
		* @throws ExpressionErrorException

Copy link

Contributor

mnapoliAug 6, 2017

Choose a reason for hiding this comment

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

Same here?

src/Symfony/Component/CssSelector/XPath/Translator.php Outdated

		*
		* @return XPathExpr
		*
		* @throws ExpressionErrorException

Copy link

Contributor

mnapoliAug 6, 2017

Choose a reason for hiding this comment

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

Same here?

Copy link

MemberAuthor

chalasrAug 7, 2017

Choose a reason for hiding this comment

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

@throws annotations re-added

src/Symfony/Component/Yaml/Inline.php Outdated

		@@ -33,6 +33,18 @@ class Inline
		private static $objectForMap = false;
		private static $constantSupport = false;

		public static function initialize(int $flags, int $parsedLineNumber = null)

Copy link

Contributor

mnapoliAug 6, 2017

Choose a reason for hiding this comment

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

Is this new method intended?

Copy link

Member

xabbuhSep 1, 2017

Choose a reason for hiding this comment

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

The idea was to not have to acceptnull values inInline::parse(). But I have moved it to its own PR:#24060

src/Symfony/Component/Yaml/Inline.php

		$output[] = sprintf('%s: %s', self::dump($key, $flags), self::dump($val, $flags));
		}

		return sprintf('{ %s }', implode(', ', $output));

Copy link

Contributor

mnapoliAug 6, 2017

Choose a reason for hiding this comment

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

Is this code change intended in this PR?

Copy link

MemberAuthor

chalasrAug 7, 2017

Choose a reason for hiding this comment

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

ping@xabbuh

Copy link

Member

xabbuhSep 1, 2017

Choose a reason for hiding this comment

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

Yes, this part is handling the dumping of objects as YAML mappings and thus allows us to type hint the$value argument indumpArray() witharray.

chalasr force-pushed thedotenv4-typehints branch from97c227c to9e46910Compare

August 7, 2017 08:15

xabbuh force-pushed thedotenv4-typehints branch 3 times, most recently fromfa7c35b toa7e1656Compare

August 29, 2017 20:48

xabbuh mentioned this pull request

Aug 29, 2017

[Yaml] mark some classes as final#24028

Merged

xabbuh added a commit that referenced this pull request

Aug 30, 2017

feature#24028[Yaml] mark some classes as final (xabbuh)

2b4b224

This PR was merged into the 3.4 branch.Discussion----------[Yaml] mark some classes as final| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Making these classes final unlocks more scalar type hint possibilities on top of the changes proposed in#23262.Commits-------330c6bf [Yaml] mark some classes as final

xabbuh force-pushed thedotenv4-typehints branch froma7e1656 toa9401a1Compare

August 31, 2017 09:38

Add scalar typehints/return types on final/internal/private code

6ce70e4

chalasr force-pushed thedotenv4-typehints branch froma9401a1 toe02137bCompare

August 31, 2017 20:46

Copy link

MemberAuthor

chalasr commentedAug 31, 2017

Updated by@xabbuh next to#24028 to add some more typehints, his changes look good to me.
PR ready.

Tobion approved these changes

Aug 31, 2017

View reviewed changes

[Yaml] use scalar type hints where possible

7b1715b

xabbuh force-pushed thedotenv4-typehints branch frome02137b to7b1715bCompare

September 1, 2017 05:53

xabbuh mentioned this pull request

Sep 1, 2017

[Yaml] add inline parser context initializer#24060

Merged

ogizanagi approved these changes

Sep 1, 2017

View reviewed changes

Copy link

Member

fabpot commentedSep 3, 2017

Thank you@chalasr.

fabpot merged commit7b1715b intosymfony:master

Sep 3, 2017

fabpot added a commit that referenced this pull request

Sep 3, 2017

feature#23262Add scalar typehints/return types (chalasr, xabbuh)

4016b3c

This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#23242 (comment)| License       | MIT| Doc PR        | n/aCommits-------7b1715b [Yaml] use scalar type hints where possible6ce70e4 Add scalar typehints/return types on final/internal/private code

chalasr deleted the dotenv4-typehints branch

September 3, 2017 16:18

fabpot added a commit that referenced this pull request

Sep 6, 2017

minor#24060[Yaml] add inline parser context initializer (xabbuh)

1022bb4

This PR was merged into the 3.4 branch.Discussion----------[Yaml] add inline parser context initializer| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#23262 (comment)| License       | MIT| Doc PR        |Calling the parser to initialize some internal context does not looklike a good idea. Additionally, this change allows to not accept nullvalues in `Inline::parse()` in 3.4 anymore.Commits-------1be23c2 [Yaml] add inline parser context initializer

fabpot mentioned this pull request

Oct 19, 2017

Release v4.0.0-BETA1#24612

Merged

jakzal mentioned this pull request

Oct 25, 2017

[Intl] Allow passing null as a locale fallback#24694

Merged

fabpot added a commit that referenced this pull request

Oct 26, 2017

bug#24694[Intl] Allow passing null as a locale fallback (jakzal)

0686378

This PR was merged into the 4.0-dev branch.Discussion----------[Intl] Allow passing null as a locale fallback| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -[`Null` is passed in `update-data.php`](https://github.com/symfony/symfony/blob/e2b4a35a720b85173d15055d0554f86602d43593/src/Symfony/Component/Intl/Resources/bin/update-data.php#L209) to prevent falling back to English locale during icu data import. It's been always possible, but since it hasn't been documented in the docblock it was missed while merging#23262.Commits-------e2b4a35 [Intl] Allow passing null as a locale fallback