Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 17, 2019
Merged

Fine tune constructor types #32066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Symfony/Component/Config/Definition/BaseNode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ abstract class BaseNode implements NodeInterface
*/
public function __construct(?string $name, NodeInterface $parent = null, string $pathSeparator = self::DEFAULT_PATH_SEPARATOR)
{
if (false !== strpos($name = (string) $name, $pathSeparator)) {
if (null === $name) {
$name = '';
}
if (false !== strpos($name, $pathSeparator)) {
throw new \InvalidArgumentException('The name must not contain "'.$pathSeparator.'".');
}

Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ abstract class NodeDefinition implements NodeParentInterface
public function __construct(?string $name, NodeParentInterface $parent = null)
{
$this->parent = $parent;
$this->name = $name;
$this->name = $name ?? '';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Config/Resource/GlobResource.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class GlobResource implements \IteratorAggregate, SelfCheckingResourceInterface
*
* @throws \InvalidArgumentException
*/
public function __construct(?string $prefix, string $pattern, bool $recursive, bool $forExclusion = false, array $excludedPrefixes = [])
public function __construct(string $prefix, string $pattern, bool $recursive, bool $forExclusion = false, array $excludedPrefixes = [])
{
$this->prefix = realpath($prefix) ?: (file_exists($prefix) ? $prefix : false);
$this->pattern = $pattern;
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Config/Tests/Loader/FileLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public function testImportWithFileLocatorDelegation()
public function testImportWithGlobLikeResource()
{
$locatorMock = $this->getMockBuilder('Symfony\Component\Config\FileLocatorInterface')->getMock();
$locatorMock->expects($this->once())->method('locate')->willReturn('');
$loader = new TestFileLoader($locatorMock);

$this->assertSame('[foo]', $loader->import('[foo]'));
Expand All @@ -79,6 +80,7 @@ public function testImportWithGlobLikeResource()
public function testImportWithNoGlobMatch()
{
$locatorMock = $this->getMockBuilder('Symfony\Component\Config\FileLocatorInterface')->getMock();
$locatorMock->expects($this->once())->method('locate')->willReturn('');
$loader = new TestFileLoader($locatorMock);

$this->assertNull($loader->import('./*.abc'));
Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/DomCrawler/AbstractUriElement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ abstract class AbstractUriElement
protected $node;

/**
* @var string The method to use for the element
* @var string|null The method to use for the element
*/
protected $method;

Expand All @@ -36,7 +36,7 @@ abstract class AbstractUriElement
/**
* @param \DOMElement $node A \DOMElement instance
* @param string $currentUri The URI of the page where the link is embedded (or the base href)
* @param string $method The method to use for the link (GET by default)
* @param string|null $method The method to use for the link (GET by default)
*
* @throws \InvalidArgumentException if the node is not a link
*/
Expand Down Expand Up @@ -70,7 +70,7 @@ public function getNode()
*/
public function getMethod()
{
return $this->method;
return $this->method ?? 'GET';
Copy link
Contributor Author

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

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.

Copy link

@dFayet dFayet Jun 17, 2019 •
edited
Loading

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 the null replacement directly on the method assignation in the __construct ?
Line 46: $this->method = $method ? strtoupper($method) : 'GET';

}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public function validate($form, Constraint $formConstraint)
// Mark the form with an error if it contains extra fields
if (!$config->getOption('allow_extra_fields') && \count($form->getExtraData()) > 0) {
$this->context->setConstraint($formConstraint);
$this->context->buildViolation($config->getOption('extra_fields_message'))
$this->context->buildViolation($config->getOption('extra_fields_message', ''))
->setParameter('{{ extra_fields }}', '"'.implode('", "', array_keys($form->getExtraData())).'"')
->setInvalidValue($form->getExtraData())
->setCode(Form::NO_SUCH_FIELD_ERROR)
Expand Down
4 changes: 3 additions & 1 deletion src/Symfony/Component/Form/Form.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ class Form implements \IteratorAggregate, FormInterface, ClearableErrorsInterfac
private $lockSetData = false;

/**
* @var string|int|null
Copy link
Contributor Author

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

* @var string|null
*/
private $name;

Expand Down Expand Up @@ -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;
Copy link
Contributor Author

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)


if (null !== $type && !\is_string($type) && !$type instanceof FormTypeInterface) {
throw new UnexpectedTypeException($type, 'string or Symfony\Component\Form\FormTypeInterface');
}
Expand Down
20 changes: 9 additions & 11 deletions src/Symfony/Component/Form/FormConfigBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,23 +107,23 @@ class FormConfigBuilder implements FormConfigBuilderInterface
/**
* Creates an empty form configuration.
*
* @param string|int $name The form name
* @param string|null $name The form name
* @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 = [])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
self::validateName($name);

if (null !== $dataClass && !class_exists($dataClass) && !interface_exists($dataClass, false)) {
throw new InvalidArgumentException(sprintf('Class "%s" not found. Is the "data_class" form option set correctly?', $dataClass));
}

$this->name = (string) $name;
$this->name = $name ?? '';
$this->dataClass = $dataClass;
$this->dispatcher = $dispatcher;
$this->options = $options;
Expand Down Expand Up @@ -767,15 +767,17 @@ public function getFormConfig()
/**
* Validates whether the given variable is a valid form name.
*
* @param string|int|null $name The tested form name
* @param string|null $name The tested form name
*
* @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
Copy link
Contributor Author

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.

*/
public static function validateName($name)
{
if (null !== $name && !\is_string($name) && !\is_int($name)) {
throw new UnexpectedTypeException($name, 'string, integer or null');
if (null !== $name && !\is_string($name)) {
throw new UnexpectedTypeException($name, 'string or null');
}

if (!self::isValidName($name)) {
Expand All @@ -792,12 +794,8 @@ public static function validateName($name)
* * starts with a letter, digit or underscore
* * contains only letters, digits, numbers, underscores ("_"),
* hyphens ("-") and colons (":")
*
* @param string|null $name The tested form name
*
* @return bool Whether the name is valid
*/
public static function isValidName($name)
final public static function isValidName(?string $name): bool
Copy link
Contributor Author

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 with self instead of late static binding.

{
return '' === $name || null === $name || preg_match('/^[a-zA-Z0-9_][a-zA-Z0-9_\-:]*$/D', $name);
}
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Form/FormError.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ class FormError
*
* @see \Symfony\Component\Translation\Translator
*/
public function __construct(?string $message, string $messageTemplate = null, array $messageParameters = [], int $messagePluralization = null, $cause = null)
public function __construct(string $message, string $messageTemplate = null, array $messageParameters = [], int $messagePluralization = null, $cause = null)
{
$this->message = (string) $message;
$this->message = $message;
$this->messageTemplate = $messageTemplate ?: $message;
$this->messageParameters = $messageParameters;
$this->messagePluralization = $messagePluralization;
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Form/FormFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public function createNamedBuilder($name, $type = 'Symfony\Component\Form\Extens

$type = $this->registry->getType($type);

$builder = $type->createBuilder($this, $name, $options);
$builder = $type->createBuilder($this, (string) $name, $options);

// Explicitly call buildForm() in order to be able to override either
// createBuilder() or buildForm() in the resolved form type
Expand Down
8 changes: 2 additions & 6 deletions src/Symfony/Component/Form/Tests/Extension/Csrf/Type/FormTypeCsrfExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ protected function setUp()
{
$this->tokenManager = $this->getMockBuilder(CsrfTokenManagerInterface::class)->getMock();
$this->translator = $this->getMockBuilder(TranslatorInterface::class)->getMock();
$this->translator->expects($this->any())->method('trans')->willReturnArgument(0);

parent::setUp();
}
Expand Down Expand Up @@ -371,16 +372,11 @@ public function testsTranslateCustomErrorMessage()
->with($csrfToken)
->willReturn(false);

$this->translator->expects($this->once())
->method('trans')
->with('Foobar')
->willReturn('[trans]Foobar[/trans]');

$form = $this->factory
->createBuilder('Symfony\Component\Form\Extension\Core\Type\FormType', null, [
'csrf_field_name' => 'csrf',
'csrf_token_manager' => $this->tokenManager,
'csrf_message' => 'Foobar',
'csrf_message' => '[trans]Foobar[/trans]',
'csrf_token_id' => 'TOKEN_ID',
'compound' => true,
])
Expand Down
5 changes: 0 additions & 5 deletions src/Symfony/Component/Form/Tests/FormConfigTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ public function getHtml4Ids()
[123],
// NULL is allowed
[null],
// Other types are not
[1.23, 'Symfony\Component\Form\Exception\UnexpectedTypeException'],
[5., 'Symfony\Component\Form\Exception\UnexpectedTypeException'],
[true, 'Symfony\Component\Form\Exception\UnexpectedTypeException'],
[new \stdClass(), 'Symfony\Component\Form\Exception\UnexpectedTypeException'],
];
}

Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/HttpFoundation/RedirectResponse.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [])
Copy link
Contributor Author

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

{
parent::__construct('', $status, $headers);

Expand Down Expand Up @@ -82,7 +82,7 @@ public function getTargetUrl()
*/
public function setTargetUrl($url)
{
if (empty($url)) {
if ('' == $url) {
throw new \InvalidArgumentException('Cannot redirect to an empty URL.');
}

Expand Down
5 changes: 3 additions & 2 deletions src/Symfony/Component/HttpFoundation/Tests/RedirectResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ public function testGenerateMetaRedirect()

/**
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage Cannot redirect to an empty URL.
*/
public function testRedirectResponseConstructorNullUrl()
public function testRedirectResponseConstructorEmptyUrl()
{
$response = new RedirectResponse(null);
$response = new RedirectResponse('');
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Validator/ConstraintViolation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

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.

Copy link
Contributor Author

@Tobion Tobion Jun 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

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 a string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #32265

* @param string|null $code The error code of the violation
* @param Constraint|null $constraint The constraint whose validation
* caused the violation
* @param mixed $cause The cause of the violation
*/
public function __construct(?string $message, ?string $messageTemplate, array $parameters, $root, ?string $propertyPath, $invalidValue, int $plural = null, $code = null, Constraint $constraint = null, $cause = null)
public function __construct(string $message, ?string $messageTemplate, array $parameters, $root, ?string $propertyPath, $invalidValue, int $plural = null, $code = null, Constraint $constraint = null, $cause = null)
{
$this->message = $message;
$this->messageTemplate = $messageTemplate;
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/Validator/Test/ConstraintValidatorTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ protected function restoreDefaultTimezone()
protected function createContext()
{
$translator = $this->getMockBuilder(TranslatorInterface::class)->getMock();
$translator->expects($this->any())->method('trans')->willReturnArgument(0);
$validator = $this->getMockBuilder('Symfony\Component\Validator\Validator\ValidatorInterface')->getMock();
$contextualValidator = $this->getMockBuilder('Symfony\Component\Validator\Validator\ContextualValidatorInterface')->getMock();

Expand Down Expand Up @@ -330,7 +331,7 @@ public function assertRaised()
private function getViolation()
{
return new ConstraintViolation(
null,
$this->message,
$this->message,
$this->parameters,
$this->context->getRoot(),
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

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?

Copy link
Member

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.

{
if (!$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
throw new \TypeError(sprintf('Argument 8 passed to %s() must be an instance of %s, %s given.', __METHOD__, TranslatorInterface::class, \is_object($translator) ? \get_class($translator) : \gettype($translator)));
Expand Down