Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fine tune constructor types#32066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| publicfunctiongetMethod() | ||
| { | ||
| return$this->method; | ||
| return$this->method ??'GET'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Must return string because of
| return$this->request($link->getMethod(),$link->getUri()); |
request has string type which would otherwise fail. So lets default back toGET which is in the constructor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will it not be better to set thenull replacement directly on themethod assignation in the__construct ?
Line 46:$this->method = $method ? strtoupper($method) : 'GET';
| thrownewUnexpectedTypeException($child,'string, integer or Symfony\Component\Form\FormInterface'); | ||
| } | ||
| $child = (string)$child; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The code forwarding this relies on the child name to be a string (instead of int)
| * the name contains invalid characters | ||
| */ | ||
| publicfunction__construct($name, ?string$dataClass,EventDispatcherInterface$dispatcher,array$options = []) | ||
| publicfunction__construct(?string$name, ?string$dataClass,EventDispatcherInterface$dispatcher,array$options = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FormBuilder extendsFormConfigBuilder. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | ||
| */ | ||
| publicfunction__construct(?string$url,int$status =302,array$headers = []) | ||
| publicfunction__construct(string$url,int$status =302,array$headers = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | ||
| * @parammixed $code The error code of the violation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is a test thatcode can be an int. So I couldn't add the string type. But the test is not respecting the contract asgetCode andsetCode both say string. But that is a topic for another time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am going to prepare a PR to deprecate using anything else than astring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
see#32265
| private$lockSetData =false; | ||
| /** | ||
| * @var string|int|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this cannot be int anymore due to constructor types added in#24722
785b9cf to3baa43dCompare| * @return bool Whether the name is valid | ||
| */ | ||
| publicstaticfunctionisValidName($name) | ||
| finalpublicstaticfunctionisValidName(?string$name):bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there is no point in people overwriting the method as its called withself instead of late static binding.
3baa43d to544bab4Compare544bab4 to507794aCompare
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Tobias, thanks a lot for the time you took to carefully review (and explain) all this.
fabpot commentedJun 17, 2019
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 <!-- seehttps://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#24722Form 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 alsofixes#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 | ||
| */ | ||
| publicfunction__construct(ConstraintViolationList$violations,Constraint$constraint,$message,array$parameters,$root,$propertyPath,$invalidValue,$translator,$translationDomain =null) | ||
| publicfunction__construct(ConstraintViolationList$violations,Constraint$constraint,string$message,array$parameters,$root,string$propertyPath,$invalidValue,$translator,string$translationDomain =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this breaks the 3.4 CI, so let's allow null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The job you linked succeded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I relaunched it after#32074 and it passed.
nicolas-grekas commentedJun 17, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedJun 17, 2019
Ok, I'm going to submit a BC layer instead of reverting. |
nicolas-grekas commentedJun 17, 2019
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
Uh oh!
There was an error while loading.Please reload this page.
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 alsofixes#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.