*** Bug 110025 has been marked as a duplicate of this bug. ***

Fixed on the trunk in change 38262.

The fix is implemented as follows.

Quoting from change 38262:

  * Load FormSectionItem and FormItem into the "/forms/" section.

  * For any content type X, any content section S such that X is not in
    {FormSectionItem, FormItem} and S is not "/forms/", load X into S.

The literal string "forms" is hardcoded into two files (with two
additional files having indirect knowledge of it):

  //cms/dev/src/com/arsdigita/cms/enterprise.init#5
  //cms/dev/src/com/arsdigita/cms/contenttypes/AbstractContentTypeLoader.java#5

The following two files have indirect knowledge of the literal string
"forms" (via the symbolic constant
AbstractContentTypeLoader.FORM_SECTION):

 //cms/content-types/formitem/dev/src/com/arsdigita/cms/formbuilder/FormItemLoader.java#4
 //cms/content-types/formsectionitem/dev/src/com/arsdigita/cms/formbuilder/FormSectionItemLoader.java#4

The class AbstractContentTypeLoader has acquired one additional
method:

  protected boolean isLoadableInto(ContentSection section);

Note that the method is not abstract.

This is not QA_READY since it doesn't address the following point:

> Whichever method is chosen in the end though, there must be a config
> property to override the defualt of /content & /forms since neither
> section may exist.

The ticket was marked QA_READY because change 38262 eliminates the
out-of-box regression from 6.0.

As for addressing the more general issue, I have a strong feeling we
need to expend a little more mental energy on this.

> I suggest either two properties in AbstractContentTypeLoader, with
> default values to match previous 6.0 / 5.2 config, ie:
> 
>    com.arsdigita.cms.contenttypes.content_section=/content
>    com.arsdigita.cms.contenttypes.form_section=/forms


One of these values seems redundant.  The distinction seems to be that
FormSectionItem (FSI) and FormItem (FI) should go into the "forms"
section.  Any other content type should go into all other content
sections.  Therefore, it would seem sufficient to specify either the
"form_section" parameter or the "non_form_section" parameter, but not
both.

> Or alternatively let each individual content type provide a config
> property for its section and add an
> 
>    abstract String getContentSection();

This makes the assumption that each content type only wants to be
installed in a single section.  Are we willing to hard-code this
assumption?

The current implementation is that AbstractContentTypeLoader (ACTL)
has the method

  boolean isLoadableInto(ContentSection);

so that the createTypes method can do this:


  DataCollection sections = ssn.retrieve(ContentSection.TYPE);

  while (sections.next()) {
      ContentSection section = (ContentSection)
              DomainObjectFactory.newInstance(sections.getDataObject());
      if ( !isLoadableInto(section) ) { continue; }

      // do stuff
  }


The default implementation is

    protected boolean isLoadableInto(ContentSection section) {
        return  !"forms".equals(section.getName());
    }

My question is, how configurable do you want this to be?  One option
would be something like this:

    protected boolean isLoadableInto(ContentSection section) {
        String specialSection = getParam("form_section");
        return  !specialSection.equals(section.getName());
    }

That would be the default implementation.  The value of the
"form_section" parameter would be configurable.  FILoader and
FSILoader can override ACTL's implementation like so:

    protected boolean isLoadableInto(ContentSection section) {
        return !super.isLoadableInto(section);
    }

You can go one step further and say that ACTL defines this method:

public abstract class ACTL {
   public static interface CSDecider {
       boolean isLoadableInto(ContentSection)
   }


   public CSDecider getCSDecider() {
      CSDecider decider = getParameter("decider_class_name");
      return decider;
   }
}

and you can make the name of the particular implementation of this
interface configurable.  This would help you get around the rather
arbitrary, APLAWS-centric assumption that there is this single special
content section that is very different from all other sections.

Thoughts?

Dan & Vadim came to a consensus on IRC. Vadim plans to fix for Dec 5th.
If possible, can this conversation thread be posted to the ticket?

<vadim>
    justin,danpb_uml_monkey: have you had the time to look at
    https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=109947 ?
<danpb_uml_monkey>
    vadim: i think we're getting into slight overkill here
<danpb_uml_monkey>
    with having pluggable APIs for deciding what section to install into
<danpb_uml_monkey>
    could we simply make do with the following:
<danpb_uml_monkey>
    have a config property on AbstractContnetTypeLoader which is the name of
    the default content section to install types in
<danpb_uml_monkey>
    optionally have a config property per subclass which provides a list of
    content sections to put that particular type in - this overrides the
    default setting
<danpb_uml_monkey>
    so in simple case, an admin could set
<danpb_uml_monkey>
    cms.types.section=/foo/
<danpb_uml_monkey>
    and have it apply to all content types
<danpb_uml_monkey>
    if a particular type needed to go elsewhere they could set
<danpb_uml_monkey>
    cms.types.formitem.section=/bar
<danpb_uml_monkey>
    or even
<danpb_uml_monkey>
    cms.types.formitem.section=/bar,/wizz,/eek
<danpb_uml_monkey>
    if it needed to go in many places
<vadim>
    danpb_uml_monkey: I still don't have a clear picture in my head.  You just
    give too many options.
<justin>
    I'm not sure users wouldn't prefer, in general, to load non-special types
    in all sections and then opt-out of them via the section UI.  in other
    words, I think this level of config may go little used
<justin>
    but this is a little OT: vadim's claim seems to be that the form types are
    special, and need special handling
<vadim>
    so, ACTL has a parameter that specifies the (single) default section.  And
    individual type loaders have a (list) property that overrides the default
    setting?
<vadim>
    my claim is, based on what has been said, FI and FSI are the only two
    types that need special handling.
<danpb_uml_monkey>
    vadim: yes, the (list) per type could be a (single) too if we like
<danpb_uml_monkey>
    vadim: we've also got a couple of other form based types in the APLWS code
    base
<vadim>
    do they go into the same "forms" section that FI and FSI go into?
<danpb_uml_monkey>
    yeah --- ashah has changed the topic to:
http://www.faqs.org/docs/jargon/M/metasyntactic-variable.html
<justin>
    this question seems clearer to me if you say that the special thing is the
    "forms" section
<justin>
    it's a prepackaged section with prepackaged types
<danpb_uml_monkey>
    likewise 'content' section is a prepackaged section with a bunch of
    prepackaged types
<justin>
    every other section is, logically, not part of the distribution but part
    of the deployment
<danpb_uml_monkey>
    if we had sitemap UI to create sections on the section we wouldn't need to
    pre-populate anything really
<justin>
    right
<danpb_uml_monkey>
    justin: perhaps we're thinking about this entirely the wrong way
<danpb_uml_monkey>
    maybe a single map parameter containing
<danpb_uml_monkey>
    a content section ->
    list of types mapping is whats needed ?
<vadim>
    I'm trying to optimize for the most common case.  If someone doesn't like
    the default setting, they have to override it.  If the controlling
    parameter is "default_content_section", then you have to override for each
    of your 35 content types.  If the controlling parameter is "form_section",
    then you have to override for only 3 or 4 special content types.
<danpb_uml_monkey>
    vadim: you could always change 'default_content_section' param itself
<danpb_uml_monkey>
    thus affecting every single type at once
<vadim>
    yes, you're right
<justin>
    I think a section ->
    type mapping makes some sense.  trouble is, I think it gets us into the db
    vs config storage problem.  anyway, we shouldn't try to solve that now, I
    think we need to think expediently
<justin>
    or, rather, "local optimum"
<danpb_uml_monkey>
    oh yeah
<vadim>
    ok, so what's the local optimum?
<vadim>
      List getDefaultSections();
<vadim>
    where getDefaultSections() reads the configurable parameter value?
<vadim>
    and also:
<danpb_uml_monkey>
    yeah,
<vadim>
      List getTypeSpecificSections();
<vadim>
    or some such?
<vadim>
    if this return non-null, then getDefaultSections() is ignored?
<danpb_uml_monkey>
    vadim: i figure that if getDefaultSections returned null or empty list
<danpb_uml_monkey>
    then the general default section would be used
<vadim>
    ok, slow down a bit.  There is a "general section" and "default sections"?
<danpb_uml_monkey>

    sorry
<danpb_uml_monkey>
    'List getDefaultSections()' is something that looks at a property for a
    particular contnet type
<vadim>
    ok
<danpb_uml_monkey>
    if that is not set to anything
<danpb_uml_monkey>
    then it falls back to value of cms.types.default_section (or similar)
<justin>
    I'm headed to lunch.  I'll read the results when I get back
<vadim>
    the return value of getDefaultSections() is a list of strings?  or a list
    of content sections?
<danpb_uml_monkey>
    doesn't really matter
<vadim>
    the type of the "default_section" parameter is a String?  or a list of
    Strings?
<danpb_uml_monkey>
    a single String i reckon
<vadim>
    awrighty then

Created attachment 96270 [details]
transcript of an IRC chat with Dan

I tried posting this as a comment on the ticket, but it didn't stick
for some reason.  (Although I did get an email notification.) I'm
guessing the comment size was too big.	So there.

This is partially addressed in 38397.  Here's how the current setup
works.

All content type loaders currently extend AbstractContentTypeLoader
(ACTL). The latter has the following method.

  protected List getContentSections();

It returns an empty list.  If a custom content type loader chooses not
to override this method to return a non-empty list, then ACTL will
load the content type into content section specified by
ContentSection.getConfig().getDefaultContentSection().  The latter
returns the value of the
com.arsdigita.cms.ContentSectionConfig.default_content_section
parameter, defaulting to "content".

If the custom content type loader overrides the getContentSections()
method, then the value of the "default_content_section" parameter is
ignored.  The content type is loaded only into those sections that are
contained in the list returned by getContentSections().

There are currently two custom content type loaders that override the
method getContentSections().  These are FormSectionItemLoader
(hereinafter FSILoader) and FormItemLoader (FILoader).  Their
implementations are _nearly_identical_.  The only part that differs is
the parameter name.  Makes me wonder if it would be useful to provide
a mechanism for overriding a default parameter value on a case-by-case
basis.  The specific use case I have in mind is the following.

  Current setup:

    FSILoader and FILoader have nearly identical implementations of
    getContentSections().  The former returns the value of
    com.arsdigita.cms.formbuilder.FormSectionItem.sections and the
    latter com.arsdigita.cms.formbuilder.FormItem.sections.

  Desirable setup:

    Factor out the duplicate implementations out of FSILoader and
    FILoader and provide a single implementation in ACTL.  The ACTL
    implementation would return the value of
    com.arsdigita.cms.ContentSectionConfig.default_content_section.
    
    To customize this value for specific content types, we would
    create, say, FSILoader_parameter.properties that looks like so:

    formbuilder.FormSectionItem.sections = overrides
com.arsdigita.cms.ContentSectionConfig.default_content_section
    formbuilder.FormSectionItem.sections.default = forms

    In other words, all I want to do is override the value of a
    parameter on a per content-type basis.  It seems a little
    gratuitous to have to copy and paste lines of code in order to do
    this.  My proposal seems to be out in the left field.  Perhaps
    someone can think of a better solution.
       
Anyhow, the current solution is likely temporary because the current
setup is somewhat fragile.  Sections are currently created by
SectionInitializer backed by enterprise.init.  The literal strings
"content" and "forms" are hardcoded in
  
  //cms/dev/src/com/arsdigita/cms/enterprise.init
  //cms/dev/src/com/arsdigita/cms/ContentSectionConfig.java
  //cms/content-types/formsectionitem/dev/src/com/arsdigita/cms/formbuilder/FormSectionItemLoader.java
  //cms/content-types/formitem/dev/src/com/arsdigita/cms/formbuilder/FormItemLoader.java