[Zope-CMF] Re: Add forms and menus

[Martin Aspeli]

Hi Yuppie,

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

Thanks for that!

>>     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.

Cool.

>>             # 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)

Nice!

>>         # check preconditions
>>         checkObject(container, name, content)
> 
> I doubt constraints based on __setitem__ and __parent__ work in Zope 2.

Well, they do in the sense that if you have them and we have this code, 
we'll get an exception. They don't work generally, tough, so this may be 
YAGNI. It was copied from Five's Adding implementation, so I figured it 
should be kept if someone's relying on it.

>>         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.

Yeah, that code's pretty nuts and doesn't make a lot of sense. I tried 
to refactor it when I copied it over, actually, and then found that I 
didn't really understand it so I left it alone.

> Creating the content I set a provisional id. In the 'add' method I just 
> use this line:
> 
>    name = chooser.chooseName(content.getId(), content)

+1

>>         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.

Ah, nice. Do you think it'd be feasible to backport this, i.e. copy the 
event handler somewhere in Plone so long as Plone's still using an older 
version of CMF? Or does the new event handler rely on other changes to 
CMF as well?

>> 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.

I'm CC'ing Daniel Nouri if he has an opinion. I think it should be 
unproblematic to test this stuff out in plone.z3cform and then have it 
depend on a new five.z3cform and just do convenience imports where required.

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

CMFCore shouldn't need to. CMFDefault may want to though.

>> 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.)

Ah yeah. I've solved this elsewhere by having a local adapter factory 
that's a persistent object. It stores the portal_type and has a 
__call__() method that takes (context, request) to return a new add view 
object that's instantiated with knowledge of the portal_type.

That's for a very general add view, though. I'm not sure all types would 
want this. Local components are better avoided if possible.

>> 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.

Agree. So does this mean we want a separate property for the add view 
name? Should it be a simple string or a TALES thing?

Martin

-- 
Author of `Professional Plone Development`, a book for developers who
want to work with Plone. See http://martinaspeli.net/plone-book