iham commented 5 years ago

PLIP (Plone Improvement Proposal)

Responsible Persons

Proposer: Markus Hilbert @iham

Seconder: Jens W. Klein @jensens

Abstract

Splitting up metadata/dublincore behaviors for higher flexibility and clean up their wrong implementations.

This PLIP targets Plone 6.

Motivation & Assumptions

1.) Wrong Implementation

the metadata behaviors seem to be implemented wrong as they do have a factory but not a marker-interface.
this leads to errors as neiter IDublinCore.providedBy(content_obj) nor IBasic.providedBy(content_obj) returns True

This needs to be fixed!

2.) Splitting Behaviors

plone.dublincore

Whenever you want to do anything on one of the most basic fields, you need to remove the plone.dublincore behavior from all standard contenttypes (plus additionals) and add those inside:

plone.basic
plone.ownership
plone.publication
plone.categorization

That is a lot of work just for a simple field exchange or small adjustments.

Even for bigger things, like adding a richtext description, interaction with the description (eg for cataloging) it is a lot of boilerplate.

plone.basic

This behavior deals with two cases, which - in our opinion - breaks with the basic idea of behaviors: Doing a small and (hopefully) simple task, being a single or even multiple fields, some functionality or something alike.

The construction of plone.basic "feels" locked-in. This led to a rather weird implementation of the File and Image contenttypes: Those have no plone.basic, but implement a XML schema with a title and description field. The title is not mandatory/required, which is the only difference to plone.basic.

Having plone.basic split up into - lets say - ITitle (plone.title) and IDescription (plone.description), would solve a lot of those "issues" as we gain flexibility, overidability and reconfigurability.

In addition to that, we could/should have a title which is not required, to get rid of that separated XML implementation of File and Image. Unfortunately there is no hook to configure a behavior in the FTI.

Therefore we would need two behaviors. So this would result in

IRequiredTitle -> plone.title.required
IOptionalTitle -> plone.title.optional

Which are our suggestions for distinct naming.

plone.categorization

This is another candidate (even it's not as important as above):

subjects aka tags (mixed up naming b/c Dublincore specification vs. user expectation) and
language

have nothing in common and don't interact besides sharing the same tab (fieldset).

Having plone.categorization split up into

plone.subjects (plone.tags?) and
plone.language

would be beneficial as described for plone.basic.

Proposal & Implementation

Fixing adapter/marker usage

Adapters and markers shall work as expected and as documented in plone.behavior and need to be fixed.

Splitting behaviors

replacements in the content types FTI's XML:


<element value="plone.dublincore" />

with


<element value="plone.basic" />

<element value="plone.ownership" />

<element value="plone.publication" />

<element value="plone.categorization" />

or - if replacing plone.basic ist also a topic - with:


<element value="plone.[title|required_title|optional_title]" />

<element value="plone.description" />

<element value="plone.ownership" />

<element value="plone.publication" />

<element value="plone.categorization" />

or - if replacing plone.categorization ist also a topic - with:


<element value="plone.[title|required_title|optional_title]" />

<element value="plone.description" />

<element value="plone.ownership" />

<element value="plone.publication" />

<element value="plone.[subjects|tags]" />

<element value="plone.language" />

The new behaviors need to be implemented,
backward compatible behaviors need to be in place, issuing a deprecation warning if used.
an upgrade step need to be provided.
documentation and training needs to be reviews an adapted.

BBB Code

There are probably several places in third party code where IDublinCore(obj) adaption is used, which is fine so far. There has to be be an adapter called IDublinCore to get all the fields provided by as is now. In future this is an adapter, not a behavior as this only delivers data. It will issue deprecation warnings follwoing Plone deprecation best practice when used.

Remark: Since IBasic(obj) and ICategorization(obj) is at the current state not working, there is no BBB adapter needed.

Deliverables

Plone Core

plone.app.contenttypes
plone.app.dexterity
plone.app.upgrade
documentation
training

Third party

bobtemplates.plone

Risks

Third party modules depending on the names or schema

plone.basic or IBasic,
plone.dublincore or IDublinCore,
plone.categorization or ICatgeorization

may break.

Since the Schema names are used by z3c.form as identifiers in the HTML code this may also affect custom styles.

Participants

@iham @jensens

MrTango commented 5 years ago

At least for this I'm all in, separated behaviors instead of dublincore is much cleaner and comon practis for a while now in addons:

Fixing adapter/marker usage
Splitting behaviors

I'm ok with the rest of the changes too but have not strong opinion on that.

ale-rt commented 5 years ago

While I agree with fixing the adapter/marker usage I personally do not like the idea of shipping Plone code with so many granular behaviors.

Also I would like to defend the plone.dublincore behavior: it makes very much sense to me to have it pulled in with a single line than with many.

Then I perfectly acknowledge the problems that lead to replace plone.dublincore with more behaviors in the File and Image FTIs definition, but IMHO this solution is not optimal.

It seems to me that we always want the File and Image object to have a title. That means we want them to obey to the default plone.duclincore schema definition.

The fact that the add and edit view do not require it is a completely different thing.

Talking MVCish:

the Model wants a Title (i.e. plone.dublincore)
the View does not care about the title
the Controller fills the gap by providing a fallback title if nothing was added

I hope you see my point here.

Then, as said in the beginning, I perfectly agree that the current behavior implementation needs some love.

MrTango commented 5 years ago

The behaviors are all there, but they where bundled in dublincore behavior, which it self had no function. There is no real benefit from using this behavior, but a couple of downsides. If we just activate the 4 behaviors directly, everybody understands whats going on and can adjust it if she/he does not want parts of this default behavior of Plone.

In case you are talking about the second approache, i still like it more than forcing people to override programatically behaviors, because they are not flexible.

But yeah, i would like to have a more generic solution there too. Maybe a beavior could read some settings from the registry HIDE :) ?

ale-rt commented 5 years ago

If I got this PLIP right, it does want to add new behaviors.

The function of the plone.dublincore behavior is to declare that the objects respects the dublincore and so provides the dedicated expected accessors for the dublincore fields. I see the point of having it. I agree, the same result can be achieved pulling in several behavior at a time, but it is much more elegant to do it with just one line that with many.

If we extremize the "customizability approach" we will end up creating a behavior for each field. Also I would discourage mixing the registry with the behaviors, we have XML model schemas or Python code for very custom things.

P.s.: I did not get what the HIDE acronym stands for :p

iham commented 5 years ago

@ale-rt i don't see where implicit is better than explicit.

the dublincore behavior - which btw is only a name, as it does not particular provide dublincore metadata, but "real" data used by plone - is only bundling behaviors and has no further functionality on its own. a simple wrapper so to say. the only functionality it provides, is to make customisability a true nightmare, which led to those "funny" implementations of file and image schema.xml's

i don't see any elegance in any of that.

wanting to replace the plain text description with a more sophisticated richtext-like field seems not to be a big thing at first sight, but by the "dublincore"-bundled behavior and additional schema xml it becomes a lot of deep diving work to get one simple field enhanced for all types plone provides.

i see the behaviors also as a set of features to use for custom types in addition. not having to add the "dublincore" bundle to avoid a "required" title in favour of an optional one seems rather unusual. i would be clear about features, if i add a title, a description, a categorization, ... as those are listed and described in the control panel for creating a custom type.

It seems to me that we always want the File and Image object to have a title. That means we want them to obey to the default plone.duclincore schema definition.

thats the point of the required vs optional title.* image and file don't force usage of the title field (namefromfile does fill that title field, but its not required by definition) and just to do so, it does not use dc, as the dc title is required. especially looking at the implementation of image and file shows how "bad" these coupled behaviors (plone.basic and on top plone.dublincore) is: just to get rid of the required title, you can't use plone.dc, you can't even use plone.basic! you need to fall back to a custom implementation of plone.basic, add your own fields (image/file, which are - as a behavior on its own pretty useful in other places btw), and register only parts of plone.dc. wrapping my head around all that is imho frustrating.

* the title behaviors - in favour of z3c.form directives and mosaic and such using a dotted name notation ("IWhateverBehavior.some_field"), can be added to separate files (required_title.py, optional_title.py) to make both interfaces be used the same way. -> ITitle. but be registered using named behaviors like "plone.requiredtitle" and "plone.optionaltitle".

directives.order_after(description='ITitle.title') this wouldn't work with a IRequiredTitle and a IOptionalTitle defined in the same file. -> don't ask me; z3c and how this directives work is an unsolveabel night... - erm - "mystery" to me. same for mosaic: <div data-tile="./@@plone.app.standardtiles.field?field=ITitle-title"></div> only works if the behavior interface has the same name, so you don't have to decide if the title is the required one or not.

ale-rt commented 4 years ago

@iham, @jensens we discussed this PLIP in today's framework team meeting. We basically agreed on fixing the behavior implementation (1st part of the PLIP) but not on the 2nd one (splitting behaviors). The suggestion that came from today's discussion is to split the PLIP (e.g. by making a new PLIP containing just the 2nd part). Also a possibility is to demonstrate the validity of the split behaviors approach preparing an add on that contains the fine tuned behavior.

jensens commented 4 years ago

Hi thanks for the feedback!

Also a possibility is to demonstrate the validity of the split behaviors approach preparing an add on that contains the fine tuned behavior.

I doubt it makes sense or even is possible to implement part two as a whole in an addon.

Furthermore I will split this up in three parts:

1) as part one of this PLIP (stays this PLIP) 2) splitting up plone.dublincore - which is in my opinion very important and mostly configuration, because we do not need a new behavior here. This is what I do often in my projects w/o any complications. 3) splitting up other existing behaviors further up - which is a nice to have - and can be done in an addon

What do you think?

ale-rt commented 4 years ago

Ok for the even more granular splitting of this PLIP!

About plone.dublincore behavior, my opinion is that it is still convenient to keep it. I would even embrace it more: all the default Plone content types should have the plone.dublincore behavior enabled, also Files and Images should have it.

I think the historical reason for the splitting was that using the plone.dublincore behavior was making the title required in the add and edit form and there is no straightforward way to make it not required. To overcome this limitation a solution (splitting the dublincore behavior in parts) was chosen. I think that was the origin for the confusion we see now. The title is de facto required for those content types and the plone.namefromfilename actually sets it (if title is not already provided) https://github.com/plone/plone.app.dexterity/blob/49211f43f6ef76d11fd83284042d9fc21b1949fb/plone/app/dexterity/behaviors/filename.py#L29-L30

Maybe keeping the plone.dublincore schema for those content types and just not requiring the title in the add/edit form would have been a better choice. That implementation could be even made automatically by adding some some logic, e.g. if you have the plone.namefromfilename behavior enabled the title widget should not be required.

Anyway this is rather philosophical: having the plone.dublincore behavior has a whole is a declaration that we want to give an easy way to provide some standard fields, providing one (or even more than one) behavior for field means that we want to use behaviors as the primary field provider source for our content types. I tend to see this more as an abuse of the technology.

About providing more granular behaviors: IMHO it is something which is nice to have as an add-on (you will never be able to cover every single case anyway but an add-on containing several behavior variants is a good start and source for inspiration for somebody who wants to make a new behavior).

Note that those are just my opinion, maybe biased by my use cases and past experiences, I will not oppose to anything if the general consensus is to move in this direction :)

iham commented 4 years ago

after talking to @ale-rt , @jensens and @pbauer during the alpine city sprint, we came to the conclussion, the dublincore behavior should stay.

but this should be just adding the ability to read and store dublin core data to content-objects, as the DefaultDublinCoreImpl in CMFPlone.DublinCore does. this one is used by dexterity Item and Container.

i still second to "split" the functionality in the behaviors and use them like this, so you know what you do and don't get additional fields.

    <element value="plone.basic" />
    <element value="plone.ownership" />
    <element value="plone.publication" />
    <element value="plone.categorization" />
    <element value="plone.dublincore" />

the first ones deliver the fields, the last the ability to convert them to dublin core standards. like the name - this would also work for the control panel, as you may click together some fields and wishing for a title AND dublincore representation. -> you don't run into errors that way.

imho the IDublinCore derived from IOwnership, IPublication, ICategorization, IBasic is plain wrong on that concept of behaviors. so is the behavior factory implementations of IOwnership, IPublication, ICategorization, IBasic as they don't @implement their behavior interface at all.

but i cant wrap my head around the DefaultDublinCoreImpl which holds all the stuff and acts like being the behaviorfactory of IDublinCore but isn't as this is not implementing the other metadata-behaviors and/or registers this as the behavior factory. which feels wrong again.

seems it is a bit of an inheritance hell ;)

i understand the DublinCore in metadata.py to collect all the fields from the others. ok. but this class is only mentioned once. in a test. why is it there?

i see some "errors" and tend to write a bug for those instead of pliping this part:

the marker interfaces are missing
the factories don't implement their interfaces
subclassing to collect them all is a mess as fields double and side-effects arise.

iham commented 4 years ago

after a longer call with @jensens

Reduced solves for IDublinCore in Plone 5

CMFPlone
- types.xml -> behavior plone.dublincore -> plone.basic, plone.publication, plone.ownership, plone.categorization
upgradestep
- change installed types in zmi factory behaviors.
"empty" marker definitons for
- IBasicMarker
- IPublicationMarker
- IOwnershipMarker
- ICategorizationMarker
@implementer() for IBasic, IPublication, IOwnership, ICategorization
plone.app.standardtiles field.py IDublinCore["title|description"]
plone.app.dexterity.metadata.IDublinCore (deprecate)
plone.app.dexterity.metadata.DublinCore (deprecate)
title="plone.dublincore (deprecated)"

risks

css might break in custom forms/addons
js in mockup/mosaic using "IDublinCore-title/description"

finalize in Plone 6

remove deprecations in Plone 6

tisto commented 4 months ago

I move this to stalled. No activity since 2020.

jensens commented 4 months ago

While this is still all true - I doubt it gets implemented. The current state seems to be "good enough".