search

collective / collective.z3cform.datagridfield

Datagrid Field for z3c.forms
https://pypi.org/project/collective.z3cform.datagridfield/
8 stars 28 forks source link

Fix implementation for sub form adapter #116

Closed thomasmassmann closed 2 years ago

thomasmassmann commented 2 years ago

The sub form adapter needs a different signature than originally provided.

With those changes it is possible to have an AutoExtensibleForm as sub form in a data grid field.

This fixes #114.

There is another PR for this: #115. But I don’t think we should remove it.

@jensens, what’s your opinion on this?

jensens commented 2 years ago

My first thought is: Attention we use an outdated z3c.form in Plone and the API you use just does not exist in the latest major release https://github.com/zopefoundation/z3c.form/blob/master/src/z3c/form/object.py and https://github.com/zopefoundation/z3c.form/blob/master/CHANGES.rst#330-2016-03-09

And the reason we use an outdated z3c.form is exactly this removal.

thomasmassmann commented 2 years ago

Attention we use an outdated z3c.form in Plone and the API you use just does not exist in the latest major release

Then we should remove it and merge #115 and close this one.

And the reason we use an outdated z3c.form is exactly this removal.

So you are saying we need to keep using a version from 5 years ago because of the removed ObjectSubForm and SubFormAdapter? If it is required in Plone, why not add it to plone.z3cform instead and use the latest stable release of z3c.form?

jensens commented 2 years ago

So you are saying we need to keep using a version from 5 years ago because of the removed ObjectSubForm and SubFormAdapter? If it is required in Plone, why not add it to plone.z3cform instead and use the latest stable release of z3c.form?

Yes, its that worse. Nobody ever looked into it. It is probably time. But my personal z3c.form Zen-level was never enough for this. Putting it in plone.z3cform is a good idea. Would you give it a try?

thomasmassmann commented 2 years ago

I can start a PR. Ticket is created: https://github.com/plone/plone.z3cform/issues/19

thomasmassmann commented 2 years ago

But how do we proceed with this PR and #114?

Do we remove the custom adapter here? Do we actually need it? I tried without and it still works as expected.

jensens commented 2 years ago

But how do we proceed with this PR and #114?

Do we remove the custom adapter here? Do we actually need it? I tried without and it still works as expected.

I am not deep enough into this, may @thet has an opinion on this? Or we make this more visible at community.plone.org?

thet commented 2 years ago

I'm not sure why this this AutoExtensibleSubformAdapter is needed at all. If the plone.autoform forms (basically all automatically schema-generated forms in Plone) still do work with datagridfield, then the adapter seems superfluous. Especially as it looks like it's adding generic subform support to plone.autoforms where we already should have that in core.

Sometimes it helps to look at the history.

The adapter was added here, in 2012: https://github.com/collective/collective.z3cform.datagridfield/commit/419892501d56ba559f2357cf9997d32082fdcb65

Although that doesn't tell me much either.

However, using an outdated z3c.form API is not a problem in my opinion. We ARE using the outdated z3c.form API. I do like the idea of adding subform support to plone.z3cform and be able to upgrade z3c.form. And once we have it we can also update the imports in this package.

I think @thomasmassmann's fix is totally valid, if we need it at all.

thet commented 2 years ago

Asked for help on https://community.plone.org/t/datagridfield-and-subforms-tricky-z3c-form-question/14965

petschki commented 2 years ago

I'd recommend removing the ZCML registration with #115 and get rid of ObjectSubForm and SubformAdapter completely in a second step ... it's never used in Plone 6 anymore ...

petschki commented 2 years ago

so in the meantime coredev build is green with the latest z3c.form https://github.com/plone/buildout.coredev/pull/782 and might get merged soon for Plone 6 ... but I think the whole subform implementation has to be rewritten here. Maybe the docs have some pointers for that: https://z3cform.readthedocs.io/en/latest/mustread/subform.html

mauritsvanrees commented 2 years ago

so in the meantime coredev build is green with the latest z3c.form plone/buildout.coredev#782 and might get merged soon for Plone 6 ... but I think the whole subform implementation has to be rewritten here. Maybe the docs have some pointers for that: https://z3cform.readthedocs.io/en/latest/mustread/subform.html

I thought the idea was that you (or someone else) would copy the subform implementation from z3c.form 3.x to plone.z3cform. If that is not the case, then I wonder what the value is of switching to latest z3c.form. collective.z3c.datagridfield would be broken. Or am I misinterpreting things?

petschki commented 2 years ago

That was my first intention when investigating z3c.form update in coredev. But this SubFormAdapter implementation has no effect here at all so I think it would be better to keep the code here up to date to the latest z3c.form.object api.

petschki commented 2 years ago

For the sake of completeness: due to the fact, that plone.autoform needs those removed adapters in order to make form schema hints working on zope.schema.Object subforms I've backported it to plone.z3cform and re-enabled an updated version of the subform.txt doctest to demonstrate the new imports here https://github.com/plone/plone.autoform/pull/41

I'll change this create a new PR to use the backported implementation from plone.z3cform with a fallback to old z3c.form so this should be a backwards compatible change here. (breaking change in Plone 6)

petschki commented 2 years ago

this got fixed in #120