Add scalar typehints/return types #23262

chalasr · Jun 22, 2017

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

Koc · Jun 22, 2017

src/Symfony/Component/Dotenv/Dotenv.php

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


phpdoc maybe also requires update

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

imo it would be better to have iterable

If PHP7.1, then iterable 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 to count the content, you can't rely on Traversable. So it really depends on the situation

Taluu · Jun 22, 2017

src/Symfony/Component/Dotenv/Dotenv.php

@@ -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


why not string on the required argument ?

Taluu · Jun 22, 2017

src/Symfony/Component/Dotenv/Dotenv.php

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


imo it would be better to have iterable

jvasseur · Jun 23, 2017

src/Symfony/Component/Dotenv/Dotenv.php

@@ -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')


Can we add the return type here ?

fabpot · Jun 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.

stof · Jun 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 · Jun 23, 2017

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

xabbuh · Jun 26, 2017

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

fabpot · Jul 3, 2017

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

chalasr · Jul 3, 2017

Sure.
Status: needs work

xabbuh · Jul 3, 2017

private methods could be candidates too

Tobion · Aug 5, 2017

src/Symfony/Component/Yaml/Parser.php

@@ -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());


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

Tobion · Aug 5, 2017

👍 To do this.

mnapoli · Aug 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?

chalasr · Aug 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

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

The FirewallConfig 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).

mnapoli · Aug 6, 2017

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

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

-    /**
-     * Parses next selector or combined node.


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.

yes, applies to all methods

mnapoli · Aug 6, 2017

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

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


Should the @throws be kept?

mnapoli · Aug 6, 2017

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

-     *
-     * @return XPathExpr
-     *
-     * @throws ExpressionErrorException


mnapoli · Aug 6, 2017

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

-     *
-     * @return XPathExpr
-     *
-     * @throws ExpressionErrorException


@throws annotations re-added

mnapoli · Aug 6, 2017

src/Symfony/Component/Yaml/Inline.php

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

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


Is this new method intended?

The idea was to not have to accept null values in Inline::parse(). But I have moved it to its own PR: #24060

mnapoli · Aug 6, 2017

src/Symfony/Component/Yaml/Inline.php

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


Is this code change intended in this PR?

ping @xabbuh

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

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

chalasr · Aug 31, 2017

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

fabpot · Sep 3, 2017

Thank you @chalasr.

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/a Commits ------- 7b1715b [Yaml] use scalar type hints where possible 6ce70e4 Add scalar typehints/return types on final/internal/private code

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 look like a good idea. Additionally, this change allows to not accept null values in `Inline::parse()` in 3.4 anymore. Commits ------- 1be23c2 [Yaml] add inline parser context initializer

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

carsonbot added Status: Needs Review Feature labels Jun 22, 2017

chalasr mentioned this pull request Jun 22, 2017

[Dotenv] Make load() variadic #23242

Merged

Koc reviewed Jun 22, 2017

View reviewed changes

chalasr force-pushed the dotenv4-typehints branch from 54b8f7b to 779c38c Compare 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

chalasr force-pushed the dotenv4-typehints branch from 779c38c to 9cac16d Compare June 22, 2017 22:33

jvasseur reviewed Jun 23, 2017

View reviewed changes

chalasr force-pushed the dotenv4-typehints branch from 9cac16d to 115928f Compare June 23, 2017 12:29

chalasr force-pushed the dotenv4-typehints branch from 4fe13fd to b2c916f Compare June 23, 2017 16:29

chalasr changed the title ~~[Dotenv] Add scalar typehints/return types~~ Add scalar typehints/return types Jul 3, 2017

nicolas-grekas added this to the 4.0 milestone Jul 3, 2017

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 3, 2017

chalasr force-pushed the dotenv4-typehints branch 7 times, most recently from 5717ecb to d2b8d38 Compare July 8, 2017 14:44

ogizanagi approved these changes Aug 5, 2017

View reviewed changes

Tobion reviewed Aug 5, 2017

View reviewed changes

mnapoli reviewed Aug 6, 2017

View reviewed changes

chalasr force-pushed the dotenv4-typehints branch from 97c227c to 9e46910 Compare August 7, 2017 08:15

xabbuh force-pushed the dotenv4-typehints branch 3 times, most recently from fa7c35b to a7e1656 Compare August 29, 2017 20:48

xabbuh mentioned this pull request Aug 29, 2017

[Yaml] mark some classes as final #24028

Merged

xabbuh force-pushed the dotenv4-typehints branch from a7e1656 to a9401a1 Compare August 31, 2017 09:38

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

6ce70e4

chalasr force-pushed the dotenv4-typehints branch from a9401a1 to e02137b Compare August 31, 2017 20:46

Tobion approved these changes Aug 31, 2017

View reviewed changes

[Yaml] use scalar type hints where possible

7b1715b

xabbuh force-pushed the dotenv4-typehints branch from e02137b to 7b1715b Compare 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

fabpot merged commit 7b1715b into symfony:master Sep 3, 2017

chalasr deleted the dotenv4-typehints branch September 3, 2017 16:18

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

derrabus mentioned this pull request Jun 25, 2019

[5.0] Add parameter type-hints #32179

Closed

57 tasks

Search code, repositories, users, issues, pull requests...

Uh oh!

Add scalar typehints/return types #23262

Add scalar typehints/return types #23262

Uh oh!

Conversation

chalasr commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jun 23, 2017

Uh oh!

stof commented Jun 23, 2017

Uh oh!

chalasr commented Jun 23, 2017

Uh oh!

xabbuh commented Jun 26, 2017

Uh oh!

fabpot commented Jul 3, 2017

Uh oh!

chalasr commented Jul 3, 2017

Uh oh!

xabbuh commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobion commented Aug 5, 2017

Uh oh!

mnapoli commented Aug 5, 2017

Uh oh!

chalasr commented Aug 6, 2017

Uh oh!

mnapoli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Aug 31, 2017

Uh oh!

fabpot commented Sep 3, 2017

Uh oh!

chalasr Jun 22, 2017 •

edited

Loading

xabbuh commented Jul 3, 2017 •

edited

Loading