Fine tune constructor types #32066

Tobion · 2019-06-16T23:19:37Z

Q	A
Branch?	4.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	symfony/symfony-docs#...

Fine tunes some constructor types that have been added in #24722

Form names as integer was only a workaround as forms names are used as array keys which get transformed to int. So it was added as a workaround in #6355 (comment)
With typehints added in #24722 those were mostly auto-cast anyway, e.g. in FormBuilder. There are only a few integer form names remaining documented, in the main entry points of the Form component (\Symfony\Component\Form\FormInterface::add). Internally it's always a string now. So I could remove some int docs which also fixes #30032 what @xabbuh tried to do.

Some of these changes we're just not done before because of broken tests. It's mainly a missing explicit mock for TranslationInterface::trans which returned null. If we had return types hints in interfaces, this wouldn't happen.

src/Symfony/Component/DomCrawler/AbstractUriElement.php

Tobion · 2019-06-16T23:23:43Z

src/Symfony/Component/DomCrawler/AbstractUriElement.php

@@ -70,7 +70,7 @@ public function getNode()
     */
    public function getMethod()
    {
-        return $this->method;
+        return $this->method ?? 'GET';


Must return string because of

symfony/src/Symfony/Component/BrowserKit/Client.php

Line 292 in 8496003

return $this->request($link->getMethod(), $link->getUri());

where request has string type which would otherwise fail. So lets default back to GET which is in the constructor.

Will it not be better to set the null replacement directly on the method assignation in the __construct ?
Line 46: $this->method = $method ? strtoupper($method) : 'GET';

Tobion · 2019-06-16T23:25:15Z

src/Symfony/Component/Form/Form.php

@@ -847,6 +847,8 @@ public function add($child, $type = null, array $options = [])
                throw new UnexpectedTypeException($child, 'string, integer or Symfony\Component\Form\FormInterface');
            }

+            $child = (string) $child;


The code forwarding this relies on the child name to be a string (instead of int)

Tobion · 2019-06-16T23:27:06Z

src/Symfony/Component/Form/FormConfigBuilder.php

     * @param string|null              $dataClass  The class of the form's data
     * @param EventDispatcherInterface $dispatcher The event dispatcher
     * @param array                    $options    The form options
     *
     * @throws InvalidArgumentException if the data class is not a valid class or if
     *                                  the name contains invalid characters
     */
-    public function __construct($name, ?string $dataClass, EventDispatcherInterface $dispatcher, array $options = [])
+    public function __construct(?string $name, ?string $dataClass, EventDispatcherInterface $dispatcher, array $options = [])


FormBuilder extends FormConfigBuilder. In FormBuilder the $name was already changed in #24722 so it must be here too.

Tobion · 2019-06-16T23:28:38Z

src/Symfony/Component/Form/FormConfigBuilder.php

     *
     * @throws UnexpectedTypeException  if the name is not a string or an integer
     * @throws InvalidArgumentException if the name contains invalid characters
+     *
+     * @internal since Symfony 4.4


This is only used in this single class. So wouldn't need to be public anymore. The whole method is not needed anyonce once we added real types in Form parameters in SF 5.

Tobion · 2019-06-16T23:44:24Z

src/Symfony/Component/HttpFoundation/RedirectResponse.php

@@ -32,7 +32,7 @@ class RedirectResponse extends Response
     *
     * @see http://tools.ietf.org/html/rfc2616#section-10.3
     */
-    public function __construct(?string $url, int $status = 302, array $headers = [])
+    public function __construct(string $url, int $status = 302, array $headers = [])


this was only nullable because of a unit test. but this is an error that is not meant to be caught. so it just throws a TypeError instead of \InvalidArgumentException

Tobion · 2019-06-16T23:46:15Z

src/Symfony/Component/Validator/ConstraintViolation.php

@@ -44,12 +44,12 @@ class ConstraintViolation implements ConstraintViolationInterface
     *                                         violation
     * @param int|null        $plural          The number for determining the plural
     *                                         form when translating the message
-     * @param mixed           $code            The error code of the violation


that must be an error. it cannot be mixed. \Symfony\Component\Validator\ConstraintViolationInterface::getCode says string|null. and calling code wouldn't be able to handle mixed.

There is a test that code can be an int. So I couldn't add the string type. But the test is not respecting the contract as getCode and setCode both say string. But that is a topic for another time.

I am going to prepare a PR to deprecate using anything else than a string.

Tobion · 2019-06-16T23:48:14Z

src/Symfony/Component/Form/Form.php

@@ -146,7 +146,7 @@ class Form implements \IteratorAggregate, FormInterface, ClearableErrorsInterfac
    private $lockSetData = false;

    /**
-     * @var string|int|null


this cannot be int anymore due to constructor types added in #24722

Tobion · 2019-06-17T01:18:30Z

src/Symfony/Component/Form/FormConfigBuilder.php

     */
-    public static function isValidName($name)
+    final public static function isValidName(?string $name): bool


there is no point in people overwriting the method as its called with self instead of late static binding.

javiereguiluz

Tobias, thanks a lot for the time you took to carefully review (and explain) all this.

fabpot · 2019-06-17T16:43:00Z

Thank you @Tobion.

@xabbuh

This PR was merged into the 4.4 branch. Discussion ---------- Fine tune constructor types | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | | License | MIT | Doc PR | symfony/symfony-docs#...  Fine tunes some constructor types that have been added in #24722 Form names as integer was only a workaround as forms names are used as array keys which get transformed to int. So it was added as a workaround in #6355 (comment) With typehints added in #24722 those were mostly auto-cast anyway, e.g. in FormBuilder. There are only a few integer form names remaining documented, in the main entry points of the Form component (`\Symfony\Component\Form\FormInterface::add`). Internally it's always a string now. So I could remove some int docs which also fixes #30032 what @xabbuh tried to do. Some of these changes we're just not done before because of broken tests. It's mainly a missing explicit mock for `TranslationInterface::trans` which returned null. If we had return types hints in interfaces, this wouldn't happen. Commits ------- 507794a Fine tune constructor types

nicolas-grekas · 2019-06-17T17:46:59Z

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

@@ -47,7 +47,7 @@ class ConstraintViolationBuilder implements ConstraintViolationBuilderInterface
    /**
     * @param TranslatorInterface $translator
     */
-    public function __construct(ConstraintViolationList $violations, Constraint $constraint, $message, array $parameters, $root, $propertyPath, $invalidValue, $translator, $translationDomain = null)
+    public function __construct(ConstraintViolationList $violations, Constraint $constraint, string $message, array $parameters, $root, string $propertyPath, $invalidValue, $translator, string $translationDomain = null)


this breaks the 3.4 CI, so let's allow null

see https://travis-ci.org/symfony/symfony/jobs/546867641

The job you linked succeded?

Yes, I relaunched it after #32074 and it passed.

nicolas-grekas · 2019-06-17T17:51:22Z

Given the broken tests on the CI, I'd be in favor of reverting this PR and adding a BC layer if we want to be strict on 5.0 instead.

nicolas-grekas · 2019-06-17T18:07:08Z

Ok, I'm going to submit a BC layer instead of reverting.

nicolas-grekas · 2019-06-17T18:40:03Z

See #32074

This PR was merged into the 4.4 branch. Discussion ---------- Add BC layer for updated constructor types | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Reverts some CS changes done in #32066 + replaces its BC breaks by a BC layer. Our CI is too good, it bites us hard when we break our own rules :) Commits ------- c34fcd9 Add BC layer for updated constructor types

…codes (xabbuh) This PR was merged into the 4.4 branch. Discussion ---------- [Validator] deprecate non-string constraint violation codes | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | symfony/symfony#32066 (comment) | License | MIT | Doc PR | Commits ------- e217729066 deprecate non-string constraint violation codes

…codes (xabbuh) This PR was merged into the 4.4 branch. Discussion ---------- [Validator] deprecate non-string constraint violation codes | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #32066 (comment) | License | MIT | Doc PR | Commits ------- e217729 deprecate non-string constraint violation codes

Tobion requested a review from xabbuh as a code owner June 16, 2019 23:19

carsonbot added the Status: Needs Review label Jun 16, 2019

Tobion commented Jun 16, 2019

View reviewed changes

src/Symfony/Component/DomCrawler/AbstractUriElement.php Show resolved Hide resolved

Tobion commented Jun 16, 2019

View reviewed changes

Tobion force-pushed the fine-tune-constructor-types branch 2 times, most recently from 785b9cf to 3baa43d Compare June 17, 2019 01:12

Tobion commented Jun 17, 2019

View reviewed changes

Tobion force-pushed the fine-tune-constructor-types branch from 3baa43d to 544bab4 Compare June 17, 2019 01:24

Fine tune constructor types

507794a

Tobion force-pushed the fine-tune-constructor-types branch from 544bab4 to 507794a Compare June 17, 2019 01:43

Tobion mentioned this pull request Jun 17, 2019

[5.0] Add return types in final classes #31996

Merged

5 tasks

javiereguiluz approved these changes Jun 17, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 17, 2019

nicolas-grekas added this to the next milestone Jun 17, 2019

nicolas-grekas approved these changes Jun 17, 2019

View reviewed changes

fabpot approved these changes Jun 17, 2019

View reviewed changes

fabpot merged commit 507794a into symfony:4.4 Jun 17, 2019

nicolas-grekas reviewed Jun 17, 2019

View reviewed changes

nicolas-grekas mentioned this pull request Jun 17, 2019

Add BC layer for updated constructor types #32074

Merged

Tobion deleted the fine-tune-constructor-types branch June 17, 2019 20:03

Tobion mentioned this pull request Jun 28, 2019

[Validator] pass error code as a string #32244

Merged

xabbuh mentioned this pull request Jun 28, 2019

[Validator] deprecate non-string constraint violation codes #32265

Merged

Tobion mentioned this pull request Jul 31, 2019

[Form] Deprecate passing ints #32821

Closed

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fine tune constructor types #32066

Fine tune constructor types #32066

Tobion commented Jun 16, 2019 •

edited

Loading

Tobion Jun 16, 2019

dFayet Jun 17, 2019 •

edited

Loading

Tobion Jun 16, 2019

Tobion Jun 16, 2019

Tobion Jun 16, 2019

Tobion Jun 16, 2019

Tobion Jun 16, 2019

Tobion Jun 17, 2019 •

edited

Loading

xabbuh Jun 28, 2019

xabbuh Jun 28, 2019

Tobion Jun 16, 2019

Tobion Jun 17, 2019

javiereguiluz left a comment

fabpot commented Jun 17, 2019

nicolas-grekas Jun 17, 2019

nicolas-grekas Jun 17, 2019

Tobion Jun 17, 2019

nicolas-grekas Jun 17, 2019

nicolas-grekas commented Jun 17, 2019 •

edited

Loading

nicolas-grekas commented Jun 17, 2019

nicolas-grekas commented Jun 17, 2019

Fine tune constructor types #32066

Fine tune constructor types #32066

Conversation

Tobion commented Jun 16, 2019 • edited Loading

Choose a reason for hiding this comment

dFayet Jun 17, 2019 • edited Loading

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

Choose a reason for hiding this comment

Tobion Jun 17, 2019 • edited Loading

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

javiereguiluz left a comment

Choose a reason for hiding this comment

fabpot commented Jun 17, 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

nicolas-grekas commented Jun 17, 2019 • edited Loading

nicolas-grekas commented Jun 17, 2019

nicolas-grekas commented Jun 17, 2019

Tobion commented Jun 16, 2019 •

edited

Loading

dFayet Jun 17, 2019 •

edited

Loading

Tobion Jun 17, 2019 •

edited

Loading

nicolas-grekas commented Jun 17, 2019 •

edited

Loading