bug with using the form

[dbu]

we started to use this in a sonata admin form. but we get an error from the form layer:

Found the public method "removeExtraProperty()", but did not find a public "addExtraProperty()" on class PHPCRProxiesCG\Symfony\Cmf\Bundle\SeoBundle\Doctrine\Phpcr\SeoMetadata

the form type is at https://github.com/symfony-cmf/SeoBundle/blob/master/Form/Type/SeoMetadataType.php#L42 and the method at https://github.com/symfony-cmf/SeoBundle/blob/master/Model/SeoMetadata.php#L211

i found that PropertyAccessor::402 is looking for $addMethodFound = $this->isAccessible($reflClass, $addMethod, 1);, that is a method with only one parameter. if i change the signature of our addExtraProperty it indeed is found, but i only get the key and not the value.

if you want to see all in action, you could set up the cmf sandbox branch https://github.com/symfony-cmf/cmf-sandbox/tree/prepare-rc and go to http://cmf.lo/app_dev.php/de/admin/sandbox/main/demoseocontent/cms/content/home/edit and there on the SeoConfiguration tab and play with the collections.

do we just have the wrong signature on the method? or what should we do to make things work?

[Burgov]

Hi dbu,

It took me a while to pinpoint the exact cause of the problem and for now I don't see a clean solution.

Take a look at this 2 year old Symfony issue: https://github.com/symfony/symfony/issues/5013. The second point is the one we're running into right now.

Using adders and removers is only possible for simple arrays, not for what we are trying to achieve here. I doubt this will change in the near future, so I tried to find another solution.

Unfortunately I haven't been able to find a solution that would only require code changes in this bundle, but looking at the PropertyAccessor code I did find a way to trick it into using the setter anyway, by creating a new object implementing ArrayAccess but not Traversable (the latter of which would also trigger calling adders and removers), and unwrapping that right within the setter methods.

You can try out https://github.com/Burgov/cmf-sandbox/tree/prepare-rc to see it working. This is de FormBundle change: https://github.com/Burgov/KeyValueFormBundle/commit/6048a4d0598d34c039bd6e21310f48900371b7e8 And this the SEO bundle change: https://github.com/Burgov/SeoBundle/commit/52fd3ad82ebb9936211420ccc843c8b060da9e63

The only other options I see right now would be to either remove the adders and removers from the SeoMetadata class, or fix https://github.com/symfony/symfony/issues/5013

Maybe someone else does see a better fix though. Maybe @webmozart?

[dbu]

thanks a lot for digging through this! changing the form layer would be a long term thing - we want the sandbox to work with symfony 2.3 so in the short time its not helping.

so with this change, the form layer never calls add or remove, but always overwrites the whole field with a new object. the very unfortunate bit about this is that it requires the model class to know about burgov form bundle. but i don't see any better solution (and if i am not severly mistaken, instanceof just says false if the class does not exist, so its not introducing a hard dependency at least).

unless @webmozart comes up with a much better idea, i would say lets go with this.

[Burgov]

@dbu I tried to see if setting a custom data mapper or property accessor to the builder would help, but ran into the problem that a datamapper and it's property accessor apply to all the values of the children of a form, not the form itself. That would mean that the mapper had to be set to the SeoMetadataType (making a really hard dependency to this bundle), and that the property accessor would then apply to all of it's field, rather than just the array fields.

The only solution I could see that would only require changes in this bundle is to have 'mapped' set to false by default, and do the property setting by hand in the POST_SUBMIT, but I think the risk of the bugs is not worth it.

And your assumption is indeed correct, false is returned. For now, I think this would be the best solution.

I'll make the use of this helper class an option for the form type and submit it to master.

[ElectricMaxxx]

@Burgov i tried your changes (https://github.com/symfony-cmf/SeoBundle/pull/157) in our sandbox, and it's working fine. I kept our adders as we use them, but they weren't called by the form anymore. I just need them in our ExtraExtractor.

[Burgov]

@ElectricMaxxx that's good to hear.

For anyone interested in taking a stab at this issue, I've created a branch with a test to isolate the issue: https://github.com/Burgov/KeyValueFormBundle/tree/reproduce-issue-2

[dbu]

thanks a lot!