Add scalar typehints/return types #23262

chalasr · 2017-06-22T09:31:29Z

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 · 2017-06-22T09:40:00Z

src/Symfony/Component/Dotenv/Dotenv.php

     *
     * Note that existing environment variables are never overridden.
     *
     * @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 · 2017-06-22T22:23:35Z

src/Symfony/Component/Dotenv/Dotenv.php

     * @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 · 2017-06-22T22:23:50Z

src/Symfony/Component/Dotenv/Dotenv.php

     *
     * Note that existing environment variables are never overridden.
     *
     * @param array $values An array of env variables


imo it would be better to have iterable

jvasseur · 2017-06-23T10:03:22Z

src/Symfony/Component/Dotenv/Dotenv.php

     * @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 · 2017-06-23T15:59:38Z

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 · 2017-06-23T16:01:58Z

@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 · 2017-06-23T16:32:42Z

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

xabbuh · 2017-06-26T08:13:12Z

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 · 2017-07-03T07:06:44Z

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

chalasr · 2017-07-03T09:45:19Z

Sure.
Status: needs work

xabbuh · 2017-07-03T09:51:31Z

private methods could be candidates too

Tobion · 2017-08-05T19:26:32Z

src/Symfony/Component/Yaml/Parser.php


                // 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 · 2017-08-05T19:34:58Z

👍 To do this.

mnapoli · 2017-08-05T19:50:00Z

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 · 2017-08-06T10:04:20Z

@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 · 2017-08-06T14:18:53Z

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

    }

-    /**
-     * 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 · 2017-08-06T14:20:20Z

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

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


Should the @throws be kept?

mnapoli · 2017-08-06T14:20:28Z

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

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


mnapoli · 2017-08-06T14:20:35Z

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

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


@throws annotations re-added

mnapoli · 2017-08-06T14:23:51Z

src/Symfony/Component/Yaml/Inline.php

    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 · 2017-08-06T14:24:23Z

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 · 2017-08-31T20:56:30Z

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

fabpot · 2017-09-03T16:17:15Z

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

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!

Reviewers

chalasr Jun 22, 2017 •

edited

Loading

xabbuh commented Jul 3, 2017 •

edited

Loading