Is it possible to remove the "txt" file hack inside profile folder?

[idgserpro]

One of the most intriguing example of Plone "moving parts" to me is the need of a txt file in profile folder, "to be sure this is the correct profile to be applied". This seems to me as a really ugly hack that beginners to Plone will just copy over and over without really knowing why it's there in the first place, and the day they remove an (supposed) useless txt things will go hairy.

We even have this hack in new bobtemplates.plone, and new users of Plone 5 will need to use this workaround as well.

My question is: is it possible to fix this or are we stuck to GenericSetup implementation (and the only thing we could do is add an entry to docs)?

If not possible, @pbauer, I think we should at least add an explanation (or a link mentioning the plone docs, whatever) to the txt template in bobtemplates.plone and setuphandlers as well.

@mauritsvanrees and @hannosch, I'm mentioning you both since I think you can answer this tricky subject from your experience in the field. Thanks in advance!

[fulv]

+1 (it really is ugly, and it only gets uglier once you realize why it's needed)

On Wed, Sep 23, 2015 at 12:17 PM idgserpro notifications@github.com wrote:

One of the most intriguing example of Plone "moving parts" to me is the need of a txt file in profile folder, "to be sure this is the correct profile to be applied". This is seen to me as a really ugly hack that beginners to Plone will just copy over and over without really knowing why it's there in the first place, and the day they remove an (supposed) useless txt things will go hairy.

We even have this hack in new bobtemplates.plone https://github.com/plone/bobtemplates.plone/blob/4a4174ee26deb16c1c93e02b646628f6018ed0d1/bobtemplates/plone_addon/src/%2Bpackage.namespace%2B/%2Bpackage.name%2B/setuphandlers.py.bob#L20, and new users of Plone 5 will need to use this workaround as well.

My question is: is it possible to fix this or are we stuck to GenericSetup implementation (and the only thing we could do is add an entry to docs http://docs.plone.org/develop/addons/components/genericsetup.html?highlight=readdatafile#custom-installer-code-setuphandlers-py )?

If not possible, @pbauer https://github.com/pbauer I think we should, at least, add an explanation (or a link mentioning the plone docs http://docs.plone.org/develop/addons/components/genericsetup.html?highlight=readdatafile#custom-installer-code-setuphandlers-py, whatever) to the txt template in bobtemplates.plone https://github.com/plone/bobtemplates.plone/blob/7047805779115fa726e3ad70091e845f2d22e00e/bobtemplates/plone_addon/src/%2Bpackage.namespace%2B/%2Bpackage.name%2B/profiles/default/%2Bpackage.longname%2B_default.txt and setuphandlers as well.

@mauritsvanrees https://github.com/mauritsvanrees and @hannosch https://github.com/hannosch, I'm mentioning you both since I think you can answer this tricky subject from your experience in the field. Thanks in advance!

— Reply to this email directly or view it on GitHub https://github.com/plone/Products.CMFPlone/issues/1070.

[hannosch]

The problem here is that import and export steps are global. You can register them once, and then they'll get executed and tried whenever you install any profile at all.

All these steps then have to do some check to see if they actually should be doing anything for the specific profile that is currently being processed. Most of the import steps use a something.xml file and check for the presence of those files. If say no toolset.xml file is present, then the import step for tools won't do anything.

The txt file here serves the same purpose. But because it's really just a function that should be called, there is no corresponding xml file that could hold data. Instead a simple txt file is used as a "marker".

If you'd want to get rid of the marker file, you'd have to associate the function with the specific profile in some other way. Most likely you would extend the ZCML genericSetup:importStep directive that gets created at https://github.com/plone/bobtemplates.plone/blob/4a4174ee26deb16c1c93e02b646628f6018ed0d1/bobtemplates/plone_addon/src/%2Bpackage.namespace%2B/%2Bpackage.name%2B/configure.zcml.bob#L33

The definition of the importStep directive is over at https://github.com/zopefoundation/Products.GenericSetup/blob/master/Products/GenericSetup/meta.zcml#L19 and https://github.com/zopefoundation/Products.GenericSetup/blob/master/Products/GenericSetup/zcml.py#L125 which in the end really just calls registerStep on the import registry (https://github.com/zopefoundation/Products.GenericSetup/blob/master/Products/GenericSetup/registry.py#L374).

In addition to adding a new bit of information to the registry (likely some unique id for the profile), you'd then also have check for this profile and match it against the current context (profile_id) in all the different methods that actually execute and run these steps. Those are all over in https://github.com/zopefoundation/Products.GenericSetup/blob/master/Products/GenericSetup/tool.py and there's about 10 different ways to run them, list them, inspect them and compute their dependency order, all of which would have to respect this new condition.

I think that's doable, but it's far from trivial.

[mauritsvanrees]

An alternative that would work today is to register an event handler for the ProfileImportedEvent. That event is fired by GenericSetup here: https://github.com/zopefoundation/Products.GenericSetup/blob/dae0d90996328f1d445a7e18030814cdb25bc131/Products/GenericSetup/tool.py#L1275

Then add code like this:

@adapter(IProfileImportedEvent)
def handleProfileImportedEvent(event):
    if event.profile_id != 'my.package:default' or not event.full_import:
        return
    do_whatever_you_would_do_in_setuphandlers(...)

For a real live example, see CMFQuickInstaller. It registers a handler for this event to mark products as installed when their default profile is installed. See https://github.com/plone/Products.CMFQuickInstallerTool/blob/master/Products/CMFQuickInstallerTool/events.py#L71

Also, CMFPlone uses it: https://github.com/plone/Products.CMFPlone/blob/5.0rc3/Products/CMFPlone/events.py#L19

But when seeing this in a normal add-on I would probably think: "Why was this not done in a normal import step?"

[davisagli]

It'd be nice if you could just say handler="path.to.code" in a registerProfile directive and have GenericSetup take care of running it after that profile is imported (maybe it should actually register it as a step so it's possible to run just that step, but maybe it shouldn't so we don't clutter up the list of steps so much).

[mauritsvanrees]

That would be a nice indeed. It would avoid the need for hacks like the plone-final import step that depends on lots of other steps just so that it can be sure it is handled as (almost) last import step. I had to do a fix for that recently: https://github.com/plone/Products.CMFPlone/commit/2c3fb2386aa7327821742414ae966ae9e7edc20d

[jensens]

+1 to get rid of it. i'd say we should first develop best practice code, document the new way to deal with it and change bobtemplates.plone to generate the new best practice.

[gforcada]

+1

[idgserpro]

This is only my 0.02c since I started the discussion. Please, don't get me wrong here.

IMHO, I don't agree to change the way it's done today (txt) to another way that seems "cleaner", be it using an event, be it adding a handler to registerProfile, if it means a beginner has to remember to add this configuration. If we go that approach, now we will have two different ways of solving the problem and beginners will have the same confusion: "which way is correct, why we have packages using txt, and why do we have these ones using this handler in zcml"?

I agree with @hannosch analysis:

I think that's doable, but it's far from trivial.

What motivated this issue was "we need to remove the txt and the concept of the import step need to simply work out of the box" and, if not possible to achieve, document the "workaround". In simple terms, I think we should fix GenericSetup (huge, huge amounts of work and we're walking on eggs here) or add the workaround explanation link to the txt in bobtemplates.plone. Doing what is proposed here in the last comments is just changing the workaround.

Sorry if I didn't make myself clear in the first place. I'm just afraid that we're going to make the same mistake of having two (multiple) ways of doing the same thing in Plone, and I know how much work you're having to avoid that in Plone 5, removing zope.formlib, CMFFormController and such.

[mauritsvanrees]

In a zcml file we register the import step. GenericSetup has no way of knowing if this import step is only meant for this single profile, or if this is meant for multiple profiles, say an import step in plone.app.registry that reads registry.xml for any profile that gets imported.

I cannot think of anything that would let us get rid of the txt file without requiring something extra in zcml. Well, possibly by requiring an explicit name, for example the import step must be in setuphandlers.py and the method must be named import_various. Or probably better: the name of the import step in zcml must be package.dottedname-only. But I don't like that.

The way with the txt file will always be supported. It seems technically impossible to somehow disallow this. So we do not need to worry about needing to remove stuff.

But I would not mind adding an extra way that is easier to understand and also better, like this:

  <genericsetup:registerProfile
      name="default"
      ...
      post_handler=".setuphandler.post_install"

Best about this is that this is then guaranteed to be run after all other import steps have been run. We could add a pre_handler too.

But apart from changes like this, it is not possible to get rid of the txt file. So then we may need to explain this better in the docs and in bobtemplates.plone.

Well, one idea. This is from a pdb within an import step of a client project:

(Pdb) __file__
'/Users/mauritsvanrees/clients/mpi.intranet.buildout/src/intrampi.policy/intrampi/policy/setuphandlers.py'
(Pdb) os.path.dirname(__file__)
'/Users/mauritsvanrees/clients/mpi.intranet.buildout/src/intrampi.policy/intrampi/policy'
(Pdb) context
<Products.GenericSetup.context.DirectoryImportContext object at 0x10ec32c50>
(Pdb) context._profile_path
u'/Users/mauritsvanrees/clients/mpi.intranet.buildout/src/intrampi.policy/intrampi/policy/profiles/default'
(Pdb) context._profile_path.startswith(os.path.dirname(__file__))
True
(Pdb) context._profile_path == os.path.join(os.path.dirname(__file__), 'profiles', 'default')
True

That last check could come instead of the current check. So instead of this:

def setup_various(context):
    if context.readDataFile('intrampi.policy_various.txt') is None:
        return

you would have this:

def setup_various(context):
    if context._profile_path != os.path.join(os.path.dirname(__file__), 'profiles', 'default'):
        return

Note that this would not work when your setuphandlers.py is in a sub directory, but you could compensate for that in the condition.

This could be nice.

[gforcada]

@mauritsvanrees I like the post_handler approach, that's clean and self documented in itself. It does not handle the multiple profiles problem you described though...

[mauritsvanrees]

@gforcada What do you mean? I don't see that this would have problems with multiple profiles. When GenericSetup applies a profile, it would check if there is a post_install handler defined for this specific profile and run it. This handler will not be registered as import step, so it will never be called for other profiles.

[gforcada]

@mauritsvanrees I meant for you comment (probably got it wrong then):

In a zcml file we register the import step. GenericSetup has no way of knowing if this
import step is only meant for this single profile, or if this is meant for multiple
profiles, say an import step in plone.app.registry that reads registry.xml for any
profile that gets imported.

[mauritsvanrees]

That is true for import steps. My post_handler idea is a different concept.

In my reaction I first talked about import steps in general, then launched an idea about post_handler, then an idea for replacing the txt check with a different check. I write reactions that are too long. ;-)

[idgserpro]

@mauritsvanrees and what if the directive gs:importStep had the relative path of profile, as suggested by @hannosch? Something like:

<gs:importStep
    name="my-profile"
    handler=".setuphandler.post_install"
    title="Title"
    description="Description"
    profile_path="profiles/default"/>

So the test suggested by you in https://github.com/plone/Products.CMFPlone/issues/1070#issuecomment-142943599 could stay in the GS and not in our package.

[mauritsvanrees]

@idgserpro That looks a bit strange to me. It suggests that it is valid for all profiles (in all packages) that have a profiles/default directory. It would make more sense to me to explicitly mention the profile:

<gs:importStep
    name="my-profile"
    handler=".setuphandler.post_install"
    title="Title"
    description="Description"
    profile="my.package:default" />

With this one you still don't know when exactly it will get executed, unless you add lots of depends steps in it, as the plone-final import step does.

[idgserpro]

@mauritsvanrees assuming that the setuphandlers.py will always be at the "root" of the package, we could take its filesystem path and concatenate with profile_path to decide if I execute the step or not in GenericSetup. Something like (just an example since I'm not familiar to GS code):

if context._profile_path == os.path.join(module.__file__), profile_path):
    module.post_install(context)

wherein 'module' is the setuphandler. The idea is to make this test in GS and not in the package itself.

[mauritsvanrees]

Upgrade steps are registered for a profile, not a profile path. For import steps we should do the same, if we do it like that.

[idgserpro]

We agree. We only gave the _profile_path as an example because it's already on context: the profile itself is not. The correct way is indeed to go for the profile, but then we need to change the implementation to add it to context.

[idgserpro]

@mauritsvanrees There's already an intention to add a profile attribute to an upgradeStep directive as seen in https://github.com/zopefoundation/Products.GenericSetup/issues/7. Would it be a similar idea to add a profile attribute to an importStep? In that case, wouldn't it be the case to add an issue to Products.GenericSetup as well? @rpatterson

[mauritsvanrees]

You can and must already add a profile attribute attribute to each upgrade step. The issue you point to is that there is a theoretical chance that you get a ConflictError on startup in a corner case.

Most of the ideas above indeed require a change in GenericSetup.

[idgserpro]

Reminder: not sure if it will have implications in third party implementations like in transmogrifier contexts. See http://docs.plone.org/external/collective.transmogrifier/docs/source/genericsetup.html

[mauritsvanrees]

No, whatever we do for this issue, such a use case as you link to will always keep working. Whenever a profile gets imported, this transmogrifier import step looks for for a transmogrifier.txt file in the directory of that profile and uses the information in there to do stuff. That is basically the same as for example plone.app.registry that looks for and read a registry.xml in that profile directory, except that it is now a simple text file instead of xml. That will keep working just fine.

BTW, I might like to work on the post_handler approach that I suggested earlier, but don't hold your breath: there are lots of things that I would like to work on. :-)

[idgserpro]

@mauritsvanrees Thanks for the fast reply and explanation, that was our paranoid mode getting out of hand. :)

[fulv]

I don't mean to derail the conversation on this Issue, on the contrary I hope that perhaps this question might help in focusing it:

Why are import and export steps global?

The question could be unpacked as follows: Why are there no local import/export steps? And when is it important that import/export steps be global? I'm asking because in my pedestrian day-to-day plone development, I have only ever cared about the local effects. Thanks.

On Tue, Nov 24, 2015 at 10:44 AM idgserpro notifications@github.com wrote:

@mauritsvanrees https://github.com/mauritsvanrees Thanks for the fast reply and explanation, that was our paranoid mode getting out of hand. :)

— Reply to this email directly or view it on GitHub https://github.com/plone/Products.CMFPlone/issues/1070#issuecomment-159368779 .

[davisagli]

@fulv by global and local I guess you mean steps that are used for all profiles versus steps that are only used for a specified profile?

[fulv]

Exactly.

On Tue, 24 Nov 2015, 13:21 David Glick notifications@github.com wrote:

@fulv https://github.com/fulv by global and local I guess you mean steps that are used for all profiles versus steps that are only used for a specified profile?

— Reply to this email directly or view it on GitHub https://github.com/plone/Products.CMFPlone/issues/1070#issuecomment-159408382 .

[mauritsvanrees]

Import steps are global because then you can write a handler that checks if the profile that is currently being imported has for example a skins.xml and then your handler can apply this information. So you write a handler once and then multiple profiles can benefit from this by writing a bit of xml instead of writing their own handler.

Probably 95 percent of the import steps in most Plone Sites are the import steps defined in core Plone or CMF. Most of those are global, and there are probably a few that are only for one specific profile. And I am guessing maybe half of the third party add-ons have a local import step. 99.9 percent of add-ons will use at least one global import step. (I am making up numbers here, and may be wildly wrong, but this is what I would roughly expect.)

Export steps always export a feature of the current site, and they don't care about profiles. I guess you could call them global, but that does not really mean anything for export steps.

[mauritsvanrees]

With Products.GenericSetup 1.8.3 (to be included in upcoming Plone 4.3.8 and 5.0.3) you can add a pre_handler or post_handler to your profile registration. It should point to a function that accepts a single context argument, in which the portal_setup tool will be passed. That is an alternative to the mentioned txt file hack.