8000 [Form] backport type fixes by nicolas-grekas · Pull Request #42013 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] backport type fixes #42013

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
Jul 17, 2021
Merged

[Form] backport type fixes #42013

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.
Loadi 8000 ng
Diff view
Diff view
24 changes: 9 additions & 15 deletions src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,8 @@ public function getDecoratedFactory()
/**
* {@inheritdoc}
*
* @param callable|Cache\ChoiceValue|null $value The callable or static option for
* generating the choice values
* @param callable|Cache\ChoiceFilter|null $filter The callable or static option for
* filtering the choices
* @param mixed $value
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this is really correct as we are forwarding the input and there is no guarantee that the decorated factory handles every input type as well.

Copy link
Member Author
@nicolas-grekas nicolas-grekas Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

A decorator cannot provide this guarantee, but it cannot restrict the accepted types either.
The decorated will throw as expected if an incompatible type is provided.
That's the correct behavior IMHO.

* @param mixed $filter
*/
public function createListFromChoices(iterable $choices, $value = null/*, $filter = null*/)
{
Expand Down Expand Up @@ -127,12 +125,8 @@ public function createListFromChoices(iterable $choices, $value = null/*, $filte
/**
* {@inheritdoc}
*
* @param ChoiceLoaderInterface|Cache\ChoiceLoader $loader The loader or static loader to load
* the choices lazily
* @param callable|Cache\ChoiceValue|null $value The callable or static option for
* generating the choice values
* @param callable|Cache\ChoiceFilter|null $filter The callable or static option for
* filtering the choices
* @param mixed $value
* @param mixed $filter
*/
public function createListFromLoader(ChoiceLoaderInterface $loader, $value = null/*, $filter = null*/)
{
Expand Down Expand Up @@ -174,11 +168,11 @@ public function createListFromLoader(ChoiceLoaderInterface $loader, $value = nul
/**
* {@inheritdoc}
*
* @param array|callable|Cache\PreferredChoice|null $preferredChoices The preferred choices
* @param callable|false|Cache\ChoiceLabel|null $label The option or static option generating the choice labels
* @param callable|Cache\ChoiceFieldName|null $index The option or static option generating the view indices
* @param callable|Cache\GroupBy|null $groupBy The option or static option generating the group names
* @param array|callable|Cache\ChoiceAttr|null $attr The option or static option generating the HTML attributes
* @param mixed $preferredChoices
* @param mixed $label
* @param mixed $index
* @param mixed $groupBy
* @param mixed $attr
Copy link
Contributor

Choose a reason for hiding this comment

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

In https://github.com/symfony/symfony/pull/41998/files#diff-cb1213e76aa5d215066d613f70f20e58f403200646f57a1d82b18983d86d9c3eR171-R178 you added more concrete types. Sure this should be mixed?

Copy link
Member Author
@nicolas-grekas nicolas-grekas Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In that commit, I widened the accepted types to make tests pass, without thinking more about it.

Now, using my brain a bit more, I realized that we had to add string because that's what PropertyAccessDecorator accepts. But since when is CachingFactoryDecorator coupled in anyway with PropertyAccessDecorator???

A decorator cannot know/restrict the types it accepts here, because it doesn't know what it decorates exactly.

So yes, I'm sure :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense

*/
public function createView(ChoiceListInterface $list, $preferredChoices = null, $label = null, $index = null, $groupBy = null, $attr = null)
{
Expand Down
3 changes: 2 additions & 1 deletion src/Symfony/Component/Form/Extension/Core/DataAccessor/PropertyPathAccessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Symfony\Component\PropertyAccess\Exception\UninitializedPropertyException;
use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\PropertyAccess\PropertyPathInterface;

/**
* Writes and reads values to/from an object or array using property path.
Expand Down Expand Up @@ -84,7 +85,7 @@ public function isWritable($data, FormInterface $form): bool
return null !== $form->getPropertyPath();
}

private function getPropertyValue($data, $propertyPath)
private function getPropertyValue($data, PropertyPathInterface $propertyPath)
{
try {
return $this->propertyAccessor->getValue($data, $propertyPath);
Expand Down
5 changes: 1 addition & 4 deletions src/Symfony/Component/Form/Tests/VersionAwareTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@ trait VersionAwareTest
{
protected static $supportedFeatureSetVersion = 404;

/**
* @param int $requiredFeatureSetVersion
*/
protected function requiresFeatureSet($requiredFeatureSetVersion)
protected function requiresFeatureSet(int $requiredFeatureSetVersion)
{
if ($requiredFeatureSetVersion > static::$supportedFeatureSetVersion) {
$this->markTestSkipped(sprintf('Test requires features from symfony/form %.2f but only version %.2f is supported.', $requiredFeatureSetVersion / 100, static::$supportedFeatureSetVersion / 100));
Expand Down
0