plone / plone.app.contenttypes

Dexterity-based content types for Plone
https://pypi.org/project/plone.app.contenttypes
14 stars 48 forks source link

Add Archetype-style getters and setters to all fields #318

Closed hvelarde closed 8 years ago

hvelarde commented 8 years ago

To lower the complexity bar on migrating code from Archetypes to Dexterity (and from Plone 4 to Plone 5) we must have a consistent API; I propose to add Archetype-style getters and setters to all fields in standard content types with a deprecation warning about removal of all of this code in Plone 6.

This has to be implemented in both branches, Plone 4 and Plone 5.

CC @plone/framework-team

thet commented 8 years ago

-1 if implemented per default.

+1 if done as an optional behavior (like IArchetypesCompatibilityBehavior), which generates the attributes dynamically - like Archetypes did and no one liked. This behavior should also handle some special cases, where attributes are named differently, like in plone.app.event. If this behavior isn't part of plone.app.contenttypes core, there is no need to deprecate it for Plone 6.

@hvelarde can you invest in implementing this?

hvelarde commented 8 years ago

@thet yes, the idea is that we want to work on this instead of messing around all of our packages.

we need deprecation warnings as we will get rid of Archetypes in Plone 6 and we would like to clean the code.

-1 on the behavior for simplicity; the problem is not that people liked the getters and setters or not, but that they are used in many parts of Plone and add-ons. what I want is not having to rewrite, for now, parts of my code to support both cases.

thet commented 8 years ago

@hvelarde doing it via an behavior, which generates setters/getters on the context is simple. You just add the behavior to all of your dexterity types and you're done.

instead of having this magic on dexterity again which i'm glad that it's gone and which eventually people indeed are going to use for new addons if they don't know better. we already have too much methods from Zope/OFS attached to content objects (obj.manage_* ).

let's see, how other FWT members are going to vote.

frapell commented 8 years ago

I Agree that this would be a nice addition, and +1 on doing it as a behavior (It even can be implemented as part of the migration routines in this package)

ebrehault commented 8 years ago

I am not sure I fully understand the purpose. When we migrate an add-on from Archetypes to Dexterity, we change a lot of things anyway, why would it be specifically useful to preserve the getter/setter thing?

In my experience, replacing the calls to the AT getters and setters with the Dexterity attribute notation was not the longest nor the most painful task (a big search&replace was enough). It took me more time to update the schema implementation for instance.

hvelarde commented 8 years ago

@ebrehault currently is hard to bring compatibility on add-ons; see an example:

https://github.com/collective/collective.cover/pull/482/files

I have to add this kind of stuff because of missing setImage, setFile, setText and friends:

https://github.com/collective/collective.cover/blob/dexterity-only/src/collective/cover/tests/utils.py

ebrehault commented 8 years ago

@hvelarde I think collective.cover is a specific case because it is an add-on which manipulates contents, so it does depend on how content-types are implemented.

But most part of the add-ons:

So they do not depend a lot on how content-types are implemented.

As it is is a specific case, I would prefer an optional behavior.

hvelarde commented 8 years ago

@ebrehault everybody use this on tests; without tests is impossible to maintain packages; take a look again at the code I mentioned above.

as I said, I don't want to deal with that in all of our packages; same use case has to be pretty common among developers.

a behavior is fine, and I don care if it's optional; we just have to document how to enable it on test fixtures.

jensens commented 8 years ago

I'am -100 on this as part of core. If there is an addon one can use this is a different thing.

hvelarde commented 8 years ago

@jensens this is already on the core partially: title and description fields do have setters and getters; what I want is to have a consistent API.

jensens commented 8 years ago

@hvelarde you mix things up, thats CMFCore API for DublinCore, not Archetypes. CMF dont work w/o it.

davilima6 commented 8 years ago

+1 for the behavior, with deprecation docs to discourage use on Dexterity-only projects. Cover is indeed a specific case but it fits the common CMS requirement of defining visual/hierarchical relationships among multiple content types - and we should still have a lot of Archetypes around for some time.

pbauer commented 8 years ago

I think an addon that adds these getters and setters might be welcome. This way devs who do not have the resources to migrate custom code and addons to support Dexterity can add it as a compatibility-layer. But I'm against adding such code to the core.

Dexterity and plone.app.contenttypes have been around for many years, are pretty much battle-tested and most relevant addons are already compatible with it. We could have discussed adding a layer of at-compatible methods in 2012 but adding Archetypes-code and deprecating it at the same time in 2016 feels very wrong.

There are many other ways to improve the usability of dexterity for developers. I'd rather focus on that.

hvelarde commented 8 years ago

@jensens well, in the past you where in favor (see #289) but it's always valid to change your mind.

I'm probably mixing things because I don't understand where those methods are magically created (here?); the problem is that, in order to make development easier and push compatibility with Plone 5, we would like to be able to have this getters and setters in place.

accessing, for instance, RichText, image and file fields is not very intuitive, needs a lot of boilerplate code and can be enhanced.

@pbauer for me it doesn't make sense having to maintain an add-on for this outside plone.app.contenttypes as it will add overhead; I will solve my problem without thinking on the community otherwise in case I need it (and don't worry, those things are going to live well beyond 2016).

yes, all of our add-ons are now compatible with plone.app.contenttypes, but writing tests is overly complex as I showed to you in collective.cover and that's taking a lot of time from us.

add-ons are a vital part of the Plone ecosystem as almost all Plone sites will need to install at least one. if we just throw away this proposal without giving alternatives in the core, we are going to delay Plone 5 adoption, at least in our market and area of influence as I don't have time/resources to deal with this in the near future and I don't want to expend N times the time needed to implement this enhancement here, fixing all of our packages with monkey patches.

take your time to think and make your decision; we're here to help in case you accept this proposal.

jensens commented 8 years ago

You call it an proposal and yes, this is an issue which needs to go through the official PLIP process.

To have it officially discussed by the @plone/framework-team please create a offical PLIP as described here: http://docs.plone.org/develop/coredev/docs/plips.html#how-to-submit-a-plip