[Zope-CMF] Re: Add forms and menus

[yuppie]

Hi Martin!


Martin Aspeli wrote:
>> Martin Aspeli wrote:
>>> It's also worth noting that z3c.form (via plone.z3cform, which should 
>>> be plain CMF compatible, though you may want a different default 
>>> template) has support for such views in quite a generic way. The 
>>> "CMF" version of that looks like this:
>>>
>>> http://dev.plone.org/plone/browser/plone.z3cform/trunk/plone/z3cform/add.py 
>>>
>>>
>>> z3c.form is generally nicer to work with than formlib.
>>
>> Maybe we should discuss this in a different thread. Here a short 
>> reply: My code for the AddForm would look quite different, especially 
>> for CMF trunk, 
> 
> I'm curious how it would look different. Now is the time to get the 
> correct base class behavior in plone.z3cform, before we release a new 
> version of it.

Ok. I added some comments to the 'add' method of plone.z3cform:

>     def add(self, object):
>         
>         container = aq_inner(self.context)
>         content = object
>         
>         name = self.contentName
>         chooser = INameChooser(container)
> 
>         # Ensure that construction is allowed
> 
>         portal_types = getToolByName(container, 'portal_types')
>         fti = portal_types.getTypeInfo(content)
> 
>         if fti is not None:
>             # Check add permission
>             if not fti.isConstructionAllowed(container):
>                 raise Unauthorized(u"You are not allowed to create a %d here" % fti.getId())

Inside an add form this should always be true. If it isn't true, we'll 
get an error anyway. So this check is redundant and can be removed.

>             # Check allowable content types
>             if  getattr(aq_base(container), 'allowedContentTypes', None) is not None and \
>                     not fti.getId() in container.allowedContentTypes():
>                 raise Unauthorized(u"You are not allowed to create a %d here" % fti.getId())

allowedContentTypes is quite expensive. I use this code instead:

   #check container constraints
   container_ti = portal_types.getTypeInfo(container)
   portal_type = content.getPortalTypeName()
   if container_ti is not None and \
           not container_ti.allowType(portal_type):
       raise ValueError('Disallowed subobject type: %s' % portal_type)

>         # check preconditions
>         checkObject(container, name, content)

I doubt constraints based on __setitem__ and __parent__ work in Zope 2.

>         if IContainerNamesContainer.providedBy(container):
>             # The container picks it's own names.
>             # We need to ask it to pick one.
>             name = chooser.chooseName(self.contentName or '', content)
>         else:
>             request = self.request
>             name = request.get('add_input_name', name)
> 
>             if name is None:
>                 name = chooser.chooseName(self.contentName or '', content)
>             elif name == '':
>                 name = chooser.chooseName('', content)
>             else:
>                 # Invoke the name chooser even when we have a
>                 # name. It'll do useful things with it like converting
>                 # the incoming unicode to an ASCII string.
>                 name = chooser.chooseName(name, container)
>         
>         if not name:
>             raise ValueError("Cannot add content: name chooser did not provide a name")

All that name handling is copied from an old version of Five's 
BasicAdding, which in turn is copied from old Zope 3 code. I think we 
should use our own code here to make sure we understand the code and it 
reflects our policy.

Creating the content I set a provisional id. In the 'add' method I just 
use this line:

   name = chooser.chooseName(content.getId(), content)

>         content.id = name
>         container._setObject(name, content)
>         content = container._getOb(name)
>         
>         if fti is not None:
>             fti._finishConstruction(content)

CMF trunk uses events instead of _finishConstruction.

>         self.contentName = name

>> but in general that's the way to go. Since z3c.form became the 
>> standard in the Zope 3 world I'd like to see Zope 2 and CMF moving in 
>> the same direction. Unfortunately using plone.z3cform is no option for 
>> CMF because it has a different license and repository. *If* Plone 
>> wants to donate that code to the Zope Foundation or someone writes 
>> something similar (maybe five.z3cform), I'd be happy to help with CMF 
>> integration.
> 
> Bah, I hate these discussions. I'm sure Daniel Nouri would be happy to 
> relicense. Re-invention for the sake of a license is just too dumb.
> 
> I'd prefer to keep the name to avoid breaking existing packages, though.
> 
> Another option is to factor out a few things to a five.z3cform and have 
> plone.z3cform import from it as appropriate.

+1

plone.z3cform seems to contain Plone specific stuff. Factoring out the 
generic stuff to a five.z3cform package sounds good to me.

But I don't know if everybody agrees that CMF 2.2 should depend on z3c.form.

> How about we use a naming convention that's linked to the factory that's 
> persisted in the FTI, i.e. we look for a view called 
> "add-<factory_name>" and link to that if it's available.
> 
> The idea is that the factory name is unique and specific to the content 
> type.

I sometimes use different factories for one content type, but a 1:1 
relationship doesn't seem to be necessary for your proposal.

> Different portal types that use the same factory would almost by 
> necessity have the same add view.

This is the required 1:1 relationship. But if different portal types use 
the same add view, we have to pass the portal type name to the add view. 
(addFile currently accepts portal_type as argument to override the 
default portal type if necessary.)

> We could make this overrideable as well, with another FTI property.

I guess I'd rather have a flexible explicit URL than an implicit URL 
ruled by convention.


Cheers,

	Yuppie