[3.4] Fix return types declarations #33332

nicolas-grekas · 2019-08-25T19:54:50Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

This is the subset of #33236 that applies to branch 3.4.
This PR ensures return types are correctly declared, and adjust some implementations that return incorrect types.

ro0NL · 2019-08-26T06:44:43Z

src/Symfony/Component/BrowserKit/Client.php

@@ -141,9 +141,9 @@ public function setServerParameter($key, $value)
     * Gets single server parameter for specified key.
     *
     * @param string $key     A key of the parameter to get
-     * @param string $default A default value when key is undefined
+     * @param mixed  $default A default value when key is undefined


do we want this? At first sight i dont see any test relying on it...

on branch 4.4 at least:

Symfony\Component\BrowserKit\Tests\AbstractBrowserTest::testXmlHttpRequest
Failed asserting that '' is false.

hm, i would patch 4.4 instead. It's a type violation IMHO.

or a BC break :)
REQUEST_TIME_FLOAT isn't a string either so that forcing a string here wouldn't be good (for the return type I mean)

float values can be represented as string :) for now i'd rely on the fact we dont use strict types: https://3v4l.org/3lAC8

getting server parameters as strings seems really valueable IMHO.

float to string to float isn't always accurate, we shouldn't take the risk

fair enough.. maybe server params arent strings per se 😅

src/Symfony/Component/Config/Definition/ArrayNode.php

src/Symfony/Component/Form/Tests/ResolvedFormTypeTest.php

nicolas-grekas · 2019-08-26T07:18:12Z

src/Symfony/Component/Form/FormInterface.php

@@ -31,7 +31,7 @@ interface FormInterface extends \ArrayAccess, \Traversable, \Countable
     * @throws Exception\LogicException            when trying to set a parent for a form with
     *                                             an empty name
     */
-    public function setParent(self $parent = null);
+    public function setParent(FormInterface $parent = null);


self is not supported by old versions of phpunit's mock library.

nicolas-grekas · 2019-08-26T07:23:06Z

Now green \o/ (failures unrelated)

nicolas-grekas · 2019-08-26T08:20:33Z

Thank you @derrabus.

…rabus) This PR was merged into the 3.4 branch. Discussion ---------- [3.4] Fix return types declarations | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is the subset of #33236 that applies to branch 3.4. This PR ensures return types are correctly declared, and adjust some implementations that return incorrect types. Commits ------- 2ceb453 [SecurityBundle] fix return type declarations c1d7a88 [BrowserKit] fix return type declarations 07405e2 [PropertyInfo] fix return type declarations 5f3b4b6 [Bridge/Doctrine] fix return type declarations 8706f18 [Form] fix return type declarations a32a713 [Console] fix return type declarations 523e9b9 [Intl] fix return type declarations 73f504c [Templating] fix return type declarations 2b8ef1d [DomCrawler] fix return type declarations 2ea98bb [Validator] fix return type declarations 6af0c80 [Process] fix return type declarations 28646c7 [Workflow] fix return type declarations 5f9aaa7 [Cache] fix return type declarations 5072cfc [Serializer] fix return type declarations 70feaa4 [Translation] fix return type declarations ca1fad4 [DI] fix return type declarations 9c63be4 [Config] fix return type declarations 05fe553 [HttpKernel] Fix return type declarations e0d79f7 [Security] Fix return type declarations c1b7118 [Routing] Fix return type declarations ef5ead0 [HttpFoundation] fix return type declarations

src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php

stof · 2019-08-26T10:02:04Z

src/Symfony/Component/DomCrawler/Crawler.php

@@ -43,7 +43,7 @@ class Crawler implements \Countable, \IteratorAggregate
    private $document;

    /**
-     * @var \DOMElement[]
+     * @var \DOMNode[]


this change is wrong. We only ever create it with DOMElement in it, not arbitrary DOMNode

public function addNode(\DOMNode $node) adds to this property.

And is it a BC break ?
Because the following change break our test suite:

/** - * @return \ArrayIterator|\DOMElement[] + * @return \ArrayIterator|\DOMNode[] */ public function getIterator() {

------ --------------------------------------------------------------- Line XXXXXX/Report/ReportBuilder.php ------ --------------------------------------------------------------- 71 Call to an undefined method DOMNode::getAttribute(). 78 Parameter #1 $node of class Symfony\Component\DomCrawler\Link constructor expects DOMElement, DOMNode given. ------ ---------------------------------------------------------------

Here is my code:

foreach ($crawler->filter('a') as $node) { $href = $node->getAttribute('href'); $link = new Link($node, $crawler->getBaseHref(), 'GET'); // ... }

See e.g.

symfony/src/Symfony/Component/DomCrawler/Crawler.php

Line 765 in 593ec61

if (!$node instanceof \DOMElement) {

The class has many similar checks.

…CI (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- Add return-types with help from DebugClassLoader in the CI | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #33228 | License | MIT | Doc PR | - I've spent a great deal of time on this PR, experimenting with adding return types to the codebase. TL;DR: my conclusion is that we cannot make it for 5.0. There are two reasons for this: 1. The burden this will put on the community is immense, especially when considering that third party libs must also be updated for any apps to work at all on a return-typed 5.0. Symfony must add them last, not first. 2. We need return type covariance, yet this won't be available before PHP 7.4, while 5.0 supports 7.2. What's attached? - ~a draft patching logic in `DebugClassLoader` to add return-type where it discovers this should be done~ - return types added automatically thanks to #33283 - ~manual fixes for situations not handled (yet, if possible at all) by that logic in `DebugClassLoader`~ #33332 What's achieved? Tests are green \o/ At this stage, I think we have to acknowledge we won't add return-types in 5.0 but prepare a serious plan to add them in 6.0. This plan could be: - [x] make DebugClassLoader able to automate adding return types. - [x] in 4.4: add all possible return types that don't break BC, e.g. in `Tests` and in generated code - [x] spot and fix places where annotations aren't accurate, add more annotations where possible. - [x] ensure `DebugClassLoader` triggers the best possible deprecations that encourage ppl to add return-types in their libs/apps. This means we could decide to disable the current ones (see #33235) and to re-enable them in 5.1. This will also give us the time to fine-tune the tooling (item 1. on this list) Ideally, we could reach a point where we could test branch 4.4 *with* return-types: we'd use the tooling to add them automatically in the CI job, then we'd run tests and they should be green. Let's do this? Help Wanted, here is how: *With PHP 7.4*, run `php .github/patch-types.php`. This will add return types everywhere possible. Then run tests, e.g. `./phpunit src/Symfony/Component/HttpFoundation --exclude-group legacy,issue-32995` Here are the components that fail with return types added, please help me check them all with a PR on [my fork](https://github.com/nicolas-grekas/symfony/tree/eh-return-types): - [x] src/Symfony/Bridge/Doctrine - [x] src/Symfony/Bridge/Monolog - [x] src/Symfony/Bridge/PhpUnit - [x] src/Symfony/Bridge/ProxyManager - [x] src/Symfony/Bridge/Twig - [x] src/Symfony/Bundle/DebugBundle - [x] src/Symfony/Bundle/FrameworkBundle - [x] src/Symfony/Bundle/SecurityBundle - [x] src/Symfony/Bundle/TwigBundle - [x] src/Symfony/Bundle/WebProfilerBundle - [x] src/Symfony/Bundle/WebServerBundle - [x] src/Symfony/Component/Asset - [x] src/Symfony/Component/BrowserKit - [x] src/Symfony/Component/Cache - [x] nicolas-grekas#28 src/Symfony/Component/Config - [x] src/Symfony/Component/Console - [x] src/Symfony/Component/CssSelector - [x] src/Symfony/Component/Debug - [x] nicolas-grekas#28 src/Symfony/Component/DependencyInjection - [x] src/Symfony/Component/DomCrawler - [x] src/Symfony/Component/Dotenv - [x] src/Symfony/Component/ErrorHandler - [x] src/Symfony/Component/ErrorRenderer - [x] nicolas-grekas#24 src/Symfony/Component/EventDispatcher - [x] src/Symfony/Component/ExpressionLanguage - [x] src/Symfony/Component/Filesystem - [x] src/Symfony/Component/Finder - [x] src/Symfony/Component/Form - [x] src/Symfony/Component/HttpClient - [x] src/Symfony/Component/HttpFoundation - [x] src/Symfony/Component/HttpKernel - [x] src/Symfony/Component/Inflector - [x] src/Symfony/Component/Intl - [x] src/Symfony/Component/Ldap - [x] src/Symfony/Component/Lock - [x] src/Symfony/Component/Mailer - [x] src/Symfony/Component/Messenger - [x] src/Symfony/Component/Mime - [x] src/Symfony/Component/OptionsResolver - [x] src/Symfony/Component/Process - [x] src/Symfony/Component/PropertyAccess - [x] src/Symfony/Component/PropertyInfo - [x] nicolas-grekas#25 src/Symfony/Component/Routing - [x] nicolas-grekas#26 src/Symfony/Component/Security - [x] src/Symfony/Component/Security/Core - [x] src/Symfony/Component/Security/Guard - [x] src/Symfony/Component/Security/Http - [x] nicolas-grekas#29 src/Symfony/Component/Serializer - [x] src/Symfony/Component/Security/Csrf - [x] src/Symfony/Component/Stopwatch - [x] src/Symfony/Component/Templating - [x] nicolas-grekas#27 src/Symfony/Component/Translation - [x] src/Symfony/Component/Validator - [x] src/Symfony/Component/VarDumper - [x] src/Symfony/Component/VarExporter - [x] src/Symfony/Component/WebLink - [x] src/Symfony/Component/Workflow - [x] src/Symfony/Component/Yaml - [x] src/Symfony/Contracts Commits ------- 11149a1 Add return-types with help from DebugClassLoader in the CI

danrot · 2019-08-27T07:39:07Z

src/Symfony/Component/HttpFoundation/HeaderBag.php

@@ -121,7 +121,7 @@ public function get($key, $default = null, $first = true)
        }

        if ($first) {
-            return \count($headers[$key]) ? $headers[$key][0] : $default;
+            return \count($headers[$key]) ? (string) $headers[$key][0] : $default;


I think this introduces a regression for the Expires header. It seems to be that this causes the Expires header to be written too often, because the check in the following line will now always be true, because $value will now be an empty string if it is not set:

symfony/src/Symfony/Component/HttpFoundation/HeaderBag.php

Line 208 in 0890970

if (null === $value = $this->get($key)) {

@nicolas-grekas I would be willing to contribute a fix. Should we just check for empty string instead of null?

I think the fix should be ok this line: we should allow null to be returned.

Okay, I'm on it, and will add a test for the expires header.

Created the PR: #33353

This PR was squashed before being merged into the 3.4 branch (closes #33353). Discussion ---------- Return null as Expire header if it was set to null | Q | A | ------------- | --- | Branch? | 3.4  | Bug fix? | yes | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets |  | License | MIT | Doc PR | symfony/symfony-docs#...  This PR fixes a regression introduces in #33332. If you set the `Expires` header to null when creating a `Response`, the `getExpires` function returned a date instead of null. ```php $response = new Response(null, 200, ['Expires' => null]); $response->getExpires(); // Returns a date currently, but should return null ``` See also [the comment](#33332 (comment)) in the PR introducing this regression. Commits ------- 5e3c7ea Return null as Expire header if it was set to null

nicolas-grekas added this to the 3.4 milestone Aug 25, 2019

nicolas-grekas requested review from dunglas and xabbuh as code owners August 25, 2019 19:54

carsonbot added Status: Needs Review Bug labels Aug 25, 2019

nicolas-grekas force-pushed the fix-ret-types34 branch 2 times, most recently from d198985 to 434cd93 Compare August 25, 2019 20:08

ro0NL reviewed Aug 26, 2019

View reviewed changes

src/Symfony/Component/Config/Definition/ArrayNode.php Outdated Show resolved Hide resolved

ro0NL reviewed Aug 26, 2019

View reviewed changes

src/Symfony/Component/Form/Tests/ResolvedFormTypeTest.php Outdated Show resolved Hide resolved

nicolas-grekas force-pushed the fix-ret-types34 branch 2 times, most recently from 581d5f2 to d8f8e04 Compare August 26, 2019 07:15

nicolas-grekas commented Aug 26, 2019

View reviewed changes

nicolas-grekas and others added 16 commits August 26, 2019 09:50

[HttpFoundation] fix return type declarations

ef5ead0

[Routing] Fix return type declarations

c1b7118

[Security] Fix return type declarations

e0d79f7

[HttpKernel] Fix return type declarations

05fe553

[Config] fix return type declarations

9c63be4

[DI] fix return type declarations

ca1fad4

[Translation] fix return type declarations

70feaa4

[Serializer] fix return type declarations

5072cfc

[Cache] fix return type declarations

5f9aaa7

[Workflow] fix return type declarations

28646c7

[Process] fix return type declarations

6af0c80

[Validator] fix return type declarations

2ea98bb

[DomCrawler] fix return type declarations

2b8ef1d

[Templating] fix return type declarations

73f504c

[Intl] fix return type declarations

523e9b9

[Console] fix return type declarations

a32a713

nicolas-grekas added 5 commits August 26, 2019 09:52

[Form] fix return type declarations

8706f18

[Bridge/Doctrine] fix return type declarations

5f3b4b6

[PropertyInfo] fix return type declarations

07405e2

[BrowserKit] fix return type declarations

c1d7a88

[SecurityBundle] fix return type declarations

2ceb453

nicolas-grekas force-pushed the fix-ret-types34 branch from d8f8e04 to 2ceb453 Compare August 26, 2019 07:54

ro0NL approved these changes Aug 26, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 26, 2019

nicolas-grekas merged commit 2ceb453 into symfony:3.4 Aug 26, 2019

nicolas-grekas deleted the fix-ret-types34 branch August 26, 2019 09:12

stof reviewed Aug 26, 2019

View reviewed changes

src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php Show resolved Hide resolved

stof reviewed Aug 26, 2019

View reviewed changes

nicolas-grekas mentioned this pull request Aug 26, 2019

Add return-types with help from DebugClassLoader in the CI #33236

Merged

63 tasks

danrot reviewed Aug 27, 2019

View reviewed changes

danrot mentioned this pull request Aug 27, 2019

Return null as Expire header if it was set to null #33353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.4] Fix return types declarations #33332

[3.4] Fix return types declarations #33332

nicolas-grekas commented Aug 25, 2019

ro0NL Aug 26, 2019

nicolas-grekas Aug 26, 2019

ro0NL Aug 26, 2019

nicolas-grekas Aug 26, 2019 •

edited

ro0NL Aug 26, 2019

nicolas-grekas Aug 26, 2019

ro0NL Aug 26, 2019

nicolas-grekas Aug 26, 2019

nicolas-grekas commented Aug 26, 2019

nicolas-grekas commented Aug 26, 2019

stof Aug 26, 2019

nicolas-grekas Aug 26, 2019

lyrixx Aug 29, 2019

nicolas-grekas Aug 29, 2019

danrot Aug 27, 2019 •

edited

danrot Aug 27, 2019

nicolas-grekas Aug 27, 2019

danrot Aug 27, 2019

danrot Aug 27, 2019

[3.4] Fix return types declarations #33332

[3.4] Fix return types declarations #33332

Conversation

nicolas-grekas commented Aug 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 26, 2019

nicolas-grekas commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrot Aug 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 26, 2019 •

edited

danrot Aug 27, 2019 •

edited