[3.4] Fix return types declarations #33332
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d198985 to
434cd93
Compare
ro0NL
reviewed
Aug 26, 2019
| @@ -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 | |||
There was a problem hiding this comment.
do we want this? At first sight i dont see any test relying on it...
There was a problem hiding this comment.
on branch 4.4 at least:
- Symfony\Component\BrowserKit\Tests\AbstractBrowserTest::testXmlHttpRequest
Failed asserting that '' is false.
There was a problem hiding this comment.
hm, i would patch 4.4 instead. It's a type violation IMHO.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
float to string to float isn't always accurate, we shouldn't take the risk
There was a problem hiding this comment.
fair enough.. maybe server params arent strings per se 😅
ro0NL
reviewed
Aug 26, 2019
ro0NL
reviewed
Aug 26, 2019
581d5f2 to
d8f8e04
Compare
nicolas-grekas
commented
Aug 26, 2019
| @@ -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); | |||
There was a problem hiding this comment.
self is not supported by old versions of phpunit's mock library.
|
Now green \o/ (failures unrelated) |
d8f8e04 to
2ceb453
Compare
ro0NL
approved these changes
Aug 26, 2019
|
Thank you @derrabus. |
nicolas-grekas
added a commit
that referenced
this pull request
Aug 26, 2019
…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
stof
reviewed
Aug 26, 2019
stof
reviewed
Aug 26, 2019
| @@ -43,7 +43,7 @@ class Crawler implements \Countable, \IteratorAggregate | |||
| private $document; | |||
|
|
|||
| /** | |||
| * @var \DOMElement[] | |||
| * @var \DOMNode[] | |||
There was a problem hiding this comment.
this change is wrong. We only ever create it with DOMElement in it, not arbitrary DOMNode
There was a problem hiding this comment.
public function addNode(\DOMNode $node) adds to this property.
There was a problem hiding this comment.
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');
// ...
}There was a problem hiding this comment.
See e.g.
The class has many similar checks.
63 tasks
nicolas-grekas
added a commit
that referenced
this pull request
Aug 26, 2019
…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
reviewed
Aug 27, 2019
| @@ -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; | |||
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
@nicolas-grekas I would be willing to contribute a fix. Should we just check for empty string instead of null?
There was a problem hiding this comment.
I think the fix should be ok this line: we should allow null to be returned.
There was a problem hiding this comment.
Okay, I'm on it, and will add a test for the expires header.
nicolas-grekas
added a commit
that referenced
this pull request
Aug 29, 2019
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 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.