zikula / DynamicFormBundle

Symfony bundle with helpers for handling dynamic form fields.
MIT License
1 stars 0 forks source link

Symfony 6.4 Update issue #23

Open On5-Repos opened 6 months ago

On5-Repos commented 6 months ago

With symfony 6.4 the dynamic fields start to break because Doctrine Annotation is not supported anymore.

After update to 6.4, all forms break with error related to Response Entity.

Uncaught PHP Exception Twig\Error\RuntimeError: "An exception has been thrown during the rendering of a template ("Can't get a way to read the property "Question01" in class "App\Entity\FormResponse".")." at _form.html.twig line 201

If I downgrade to 6.3 then it starts to work. I suspect its related to Doctrine Annotations, as in 6.4 annotations are been completed deprecated without replacement.

Edit: Some more investigation show that TWIG is unable to query the Form --> Questions

craigh commented 6 months ago

Hi @On5-Repos

sure, annotations are deprecated in Symfony 6 in favor of annotations. A couple ideas you could try:

  1. You could enable annotations in framework.validation.enable_annotations config
  2. You could create your own versions of this library's entities with annotations instead.

I've not worked on this lib much because I didn't know many people are using it. I'd love to learn how you are using it.

On5-Repos commented 6 months ago

I like the idea of creating own entities that way future migration to 7 will be easy. used this to allow app users to create custom forms .. I was surprised how much they started to use dynamic forms. Initially, i thought it was not going to be used much... we can make it a more generic bundle for symfony and submit to the community to be approved for use. What are your thoughts?

On5-Repos commented 6 months ago

Option 2: created own entities but its still does not like it, what worked was returning TRUE for __asset magic method.

return isset($this->data[$name]); --> Throws exception that response Data Array does not the the KEY (question)

Remove __isset: If I remove it completely then it complains because of other magic methods are been called in Class.

return true: everything works as expected.

craigh commented 6 months ago

try changing to

    public function __isset(string $name): bool
    {
        return array_key_exists($name, $this->data);
    }
craigh commented 6 months ago

Also - (after just now looking at the actual code) the Entities in this bundle are not defined with annotation or attribute but rather by XML definition. So you if you having trouble with annotations, they are almost certainly a result of your own entity definitions within your code and not from this bundle.

On5-Repos commented 6 months ago

try changing to

    public function __isset(string $name): bool
    {
        return array_key_exists($name, $this->data);
    }

Yea I tried that as well .. as long as __isset returns false... it breaks .. and starts to complain that Key does not exist in data array.. Which holds answers

On5-Repos commented 6 months ago

Also - (after just now looking at the actual code) the entities in this bundle are not defined with annotation or attribute but rather by XML definition. So you if you are having trouble with annotations, they are almost certainly a result of your own entity definitions within your code and not from this bundle.

That's true ... my initial observation was incorrect it's not related to annotations at all. One thing during debug I notice is that field type is set to array in XML. Doctrine has made ARRAY type deprecated and recommends using JSON. I tested by updating xml field definitions to json. it works for all fields after doctrine migration, where i unserialize payload and then Json decoded .. save it back in the database. but for formOptions, it doesn't. As of now it holds Regex Entity in the serialised array for constraints but converts it into json that does come out well.. meaning REGEX ENTITY DEFINITIONS GOES MISSING What are your thoughts? I also try to create entities, but it complaints about duplicate definitions ..

On5-Repos commented 6 months ago

Hi @craigh

This fixed the problem, in Symfony 6.4 and I also tested it on 6.3. You can see below, if you are ok i can create a PR for this.

public function __isset(string $name): bool { if (isEmpty($this->data)) { return true; } return array_key_exists($name, $this->data); }

I have updated the unit tests as well, as follow

public function testMagicUnset(): void { $this->responseData->setData(['foo' => 1]); unset($this->responseData->foo); $this->assertFalse(array_key_exists('foo', $this->responseData->getData())); $this->assertNull($this->responseData->foo); // uses __get @phpstan-ignore-line }

Unit Test Results:

`PHPUnit 9.6.12 by Sebastian Bergmann and contributors.

Runtime: PHP 8.2.17 Configuration: /Users/aahmed/Documents/Repos/nParent/vendor/zikula/dynamic-form-bundle/phpunit.xml.dist

............................................................... 63 / 132 ( 47%) ............................................................... 126 / 132 ( 95%) ...... 132 / 132 (100%)

Time: 00:00.348, Memory: 36.00 MB

OK (132 tests, 476 assertions) `

craigh commented 6 months ago

this doesn't make any sense: if (isEmpty($this->data)) { return true; }

  1. there is no php function isEmpty()
  2. if data is empty, then the key you are looking for does not exist, so returning true makes no sense.
On5-Repos commented 6 months ago

@craigh correct, class DynamicFieldsType -- has inherit_data set to true, in this case response data array must have keys set. If i make inherit_data to false. it starts to work. hmm any suggestion ? what you think might be causing it..

Onside note, when I run unit test without on bundle as it is ":1) Zikula\Bundle\DynamicFormBundle\Tests\Form\Type\DynamicFieldsTypeTest::testSubmitValidData Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException: Can't get a way to read the property "name" in class "Zikula\Bundle\DynamicFormBundle\Entity\AbstractResponseData@anonymous"

its downloading symfony 6.4 components e.g (symfony/form (v6.4.6): Extracting archive)... which also breaks unit test.

Also I found this https://github.com/symfony/symfony/pull/40515

On5-Repos commented 6 months ago

@craigh [PropertyAccess] Fix checking for missing properties #54194

did you get anywhere with __isset method?

On5-Repos commented 6 months ago

@craigh I have updated __asset to check if key does not exist then set the key and it seems to work ok.

I will also update tests ...

public function __isset(string $name) { if (!array_key_exists($name, $this->data)) { $this->data[$name] = null; } return array_key_exists($name, $this->data); }