Fine tune constructor types #32066
Fine tune constructor types #32066
Conversation
| public function getMethod() | ||
| { | ||
| return $this->method; | ||
| return $this->method ?? 'GET'; |
There was a problem hiding this comment.
Must return string because of
whererequest has string type which would otherwise fail. So lets default back to GET which is in the constructor.
There was a problem hiding this comment.
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';
| throw new UnexpectedTypeException($child, 'string, integer or Symfony\Component\Form\FormInterface'); | ||
| } | ||
|
|
||
| $child = (string) $child; |
There was a problem hiding this comment.
The code forwarding this relies on the child name to be a string (instead of int)
| * 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 = []) |
There was a problem hiding this comment.
FormBuilder extends FormConfigBuilder. In FormBuilder the $name was already changed in #24722 so it must be here too.
| * @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 |
There was a problem hiding this comment.
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.
| * @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 = []) |
There was a problem hiding this comment.
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
| * 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 |
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am going to prepare a PR to deprecate using anything else than a string.
| private $lockSetData = false; | ||
|
|
||
| /** | ||
| * @var string|int|null |
There was a problem hiding this comment.
this cannot be int anymore due to constructor types added in #24722
785b9cf to
3baa43d
Compare
| * @return bool Whether the name is valid | ||
| */ | ||
| public static function isValidName($name) | ||
| final public static function isValidName(?string $name): bool |
There was a problem hiding this comment.
there is no point in people overwriting the method as its called with self instead of late static binding.
3baa43d to
544bab4
Compare
544bab4 to
507794a
Compare
There was a problem hiding this comment.
Tobias, thanks a lot for the time you took to carefully review (and explain) all this.
|
Thank you @Tobion. |
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 <!-- 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 | | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> 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
| * @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) |
There was a problem hiding this comment.
this breaks the 3.4 CI, so let's allow null
There was a problem hiding this comment.
There was a problem hiding this comment.
The job you linked succeded?
|
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. |
|
Ok, I'm going to submit a BC layer instead of reverting. |
|
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
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::transwhich returned null. If we had return types hints in interfaces, this wouldn't happen.