Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FormType mapping for array on TextType not longer working #29809

Closed
alexander-schranz opened this issue Jan 7, 2019 · 28 comments
Closed

FormType mapping for array on TextType not longer working #29809

alexander-schranz opened this issue Jan 7, 2019 · 28 comments

Comments

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Jan 7, 2019

Symfony version(s) affected: 4.2.2, 4.1.10, 3.4.21

Description

Since the last release the following error appears when submitting array data to the TextType:

[structure.stagedData] This value is not valid. ({"{{ value }}":"NULL"})

How to reproduce

$builder->add('structure', TextType::class, ['property_path' => 'structure.stagedData']);

Submit an array as data structure for the TextType.

e.g.:

[
    // ...
    'data' => [
        'some' => 'data',
        'more' => [
              'unstructured' => 'data',
        ],
    ],
]

This did work until the last release.

Possible Solution

Not sure is there another formType which can just be used for mapping the data instead of TextType?

Additional context

The following PR #29307 did add the breaking change.

@xabbuh
Copy link
Member

xabbuh commented Jan 7, 2019

This looks expected to me. What would expect instead?

@alexander-schranz
Copy link
Contributor Author

@xabbuh I know that its a miss usage of TextType but I can't find any alternative to submit unstructured set of data. I thought maybe following could do the job:

$builder->add('structure', FormType::class, ['property_path' => 'structure.stagedData']);

But then I got a extra_fields error. When adding allow_extra_fields the getData is empty array so this also doesn't work.

So what I'm basically searching for is for a UnstructuredType, ArrayType or DataType or something like that. I tried to build one myself but could not get it to work maybe you can give me any hint.

@alexander-schranz
Copy link
Contributor Author

@xabbuh if I do something like the following it works currently but I'm afraid that in a future symfony release this will also crash?

namespace Sulu\Bundle\ContentBundle\Form\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\OptionsResolver\OptionsResolver;

class UnstructuredType extends AbstractType
{
    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults(array(
            'compound' => false,
            'multiple' => true,
        ));
    }
}
$builder->add('structure', UnstructuredType::class, ['property_path' => 'structure.stagedData']);

@alexander-schranz
Copy link
Contributor Author

@xabbuh as the logic is now based on multiple option. The multiple option should also be available in the FormType this would avoid creating a UnstructuredType if you just want to map some data. /cc @nicolas-grekas

@YetiCGN
Copy link

YetiCGN commented Jan 9, 2019

@alexander-schranz I don't think that's such a great idea. A TextType with option multiple I would expect to accept an array of strings (["lorem", "dolor", "ipsum"]), but not any JSON object.

@xabbuh
Copy link
Member

xabbuh commented Jan 9, 2019

Yeah, I don't think that should be part of the core.

@bvisonl
Copy link

bvisonl commented Jan 10, 2019

I am trying to do something like:

$builder->add('status', TextType::class, array("invalid_message" => "Invalid Order Status.", "multiple" => true, "compound" => false));

But I am getting that multiple is not a valid option... any idea what could I be missing to be able to use this multiple option? I depended too much on being able to send an array and work on it in the data transformer and this breaks a lot of my code

Update
Doing what @alexander-schranz said worked like a charm, I am going to fall over to that workaround in the meantime

@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2019

let's close here as there is nothing to fix

@xabbuh xabbuh closed this as completed Jan 10, 2019
@YetiCGN
Copy link

YetiCGN commented Jan 10, 2019

Yeah, but it's a major nuisance. Again. It's the fourth minor version of Symfony 3.4 LTS that I have to add a conflict entry for, because suddenly you're being stricter than before on a point release. I know, people shouldn't have abused this in the first place, but it's starting to make me lose trust in Semantic Versioning and the updateability of Symfony.

@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2019

I am sorry for the extra work this costs you, but effectively every bugfix is a BC break as there could always be someone relying on the wrong behaviour.

@YetiCGN
Copy link

YetiCGN commented Jan 10, 2019

Okay, that's true. I didn't see it that way. Thank you!

@alexander-schranz
Copy link
Contributor Author

@xabbuh do you think it would be possible to add a unstructered type to the symfony core, it would make it for me more not feeling as a hack if there is such a type in symfony core which is tested.

@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2019

I'd rather not want to do this. IMO in most of the cases such a form type means that you do not know what data to expect making validation nearly impossible. From my experience that's not really a common use case.

@alexander-schranz
Copy link
Contributor Author

@xabbuh maybe just a TestUnstructuredType so its at least tested that it works with unstructured data?

@bitgandtter
Copy link
Contributor

Any near fix? Really affected here. Thanks

@fnagel
Copy link
Contributor

fnagel commented Jan 14, 2019

Same issue here. Using RestBundle like explained here: https://symfony.com/doc/master/bundles/FOSRestBundle/2-the-view-layer.html#data-transformation

So, afaics this change causes issue with the RestBundle.

Using the workaround as proposed by @alexander-schranz works for me.

@kadet1090
Copy link

Well, you should consider to "fix" that issue for LTS and add UnstructuredType in later releases - for example I was using null as a type for unstructured field and it worked, i didn't even notice that it used TextType by default - and now it doesn't work anymore.

@stof
Copy link
Member

stof commented Jan 15, 2019

@kadet1090 the stricter validation for arrays was here to solve cases where crafted requests (sending unexpected data in fields where the backend expected something else) could lead to bugs and even to a security issue (depending on what gets done with the data).
Using a TextType with a non-scalar data is not meant to work (and always broke when trying to render the form btw)

@bvisonl
Copy link

bvisonl commented Jan 15, 2019

I completely agree with the "why" it should not be a "TextType"... it is indeed the right mindset and the obvious thing for this type is to receive a scalar. What I guess some are arguing here (and I concur) is that an alternative should exist (in the core, not 'boilerplated').. to the case when you are actually expecting something like a plain array on top of which you will operate in an XYZ way.

If Symfony itself was thought with the idea of moving away from these types of scenarios then I guess it's something we are going to have to live without as it goes against its values.

My two cents.

@kadet1090
Copy link

kadet1090 commented Jan 15, 2019

@stof Well, I totally see the point and I'm not arguing that the bug was that it was possible not that it's not possible anymore.

But it's also a BC break and BC breaks in LTS versions are not a good thing. There is also the thing that it is not well documented how to pass unstructured data to form (which can be useful for exmaple when interacting with react or some really complex not really defined json structures) and for me passing null worked so I assumed that this was the correct way of doing this because null probably means "whatever, give me anything" when in fact it was working due to bugous behaviour of assumed TextType.

@bitgandtter
Copy link
Contributor

bitgandtter commented Jan 15, 2019

regarding on what @xabbuh mention above

I am sorry for the extra work this costs you, but effectively every bugfix is a BC break as there could always be someone relying on the wrong behaviour.

IMHO braking production applications it's even worse, and either or not it's the expected behaviour; it has been there for a long, long time; and what really brakes its the BC symfony promise.

@althaus
Copy link
Contributor

althaus commented Jan 16, 2019

I must jump on the BC break, too. The update broke a (some years ago not that well built) form in our app. What about reverting the patch and adding a deprecation instead?

@YetiCGN
Copy link

YetiCGN commented Jan 16, 2019

@althaus Well, you could fix your Symfony version to 3.4.20 and refactor that form in the meantime to be compatible with 3.4.21+.

@althaus
Copy link
Contributor

althaus commented Jan 16, 2019

@YetiCGN I've already implemented that multiple workaround as I stumbled by accident upon the broken form after the update before it was deployed to our customers. Wouldn't have been nice if that had been discovered by them. :/

@xabbuh
Copy link
Member

xabbuh commented Jan 16, 2019

I appreciate if you could provide some feedback on #29911.

@xabbuh
Copy link
Member

xabbuh commented Jan 21, 2019

I'd appreciate if everyone affected by the issue described here could give their feedback on #29911. :) Please note that we are not going to change the behaviour of the TextType nor any other non-compound type provided by the core, but the proposed fix could resolve issues you might have experienced in your custom types.

@napakorn-sricomnerd
Copy link

Not sure if it is too late for this but just would like to share my work around solution.
Basically, converts array to JSON string. When it is passed in to set function, convert it back to array.

Not super pretty, but it works for all cases. I don't do custom type as it will take tooooo much effort to create all data types and maintain them. Unless Symfony4 unlock the multiple option for us.

In Controller

        $submittedData = $request->request->all();
        $submittedData['structure'] = json_encode($submittedData['structure']);
        $form->submit($submittedData);

In Entity Class


    /**
     * @var array
     * @ORM\Column(name="structure", type="json", nullable=false)
     */
    private $structure;

    /**
     * @param  $structure
     */
    public function setStructure($structure): void
    {
        if (is_string($structure)) {
            $structure = json_decode($structure, true);
        }
        $this->structure = $structure;
    }

In your Form

->add('structure', TextType::class, ['constraints' => new NotBlank()])

@prathje
Copy link

prathje commented Jun 20, 2020

As I stumbled on the same error: Using a legacy TypeGuesser could be another way to support the old behavior.

The custom LegacyArrayFormType:

use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\OptionsResolver\OptionsResolver;

class LegacyArrayFormType extends TextType {
    public function configureOptions(OptionsResolver $resolver)
    {
        parent::configureOptions($resolver);
        $resolver->setDefault('multiple', true);
        $resolver->setDefault('compound', false);
    }
}

And the type guesser (do not forget to add as service if needed):

class LegacyArrayFormTypeGuesser implements FormTypeGuesserInterface {
    public function guessType($class, $property)
    {
        return new Guess\TypeGuess(LegacyArrayFormType::class, [], Guess\Guess::LOW_CONFIDENCE);
    }

 ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests