[MAGNOLIA-6309] AreaElement sets properties directly on AreaDefinition leading sometimes to unwanted behavior Created: 20/Jul/15  Updated: 06/Aug/15  Resolved: 30/Jul/15

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: 5.4
Fix Version/s: 5.4.1

Type: Bug Priority: Neutral
Reporter: Jaroslav Simak Assignee: Jaroslav Simak
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: 0d
Time Spent: 1d 4.75h
Original Estimate: Not Specified

Issue Links:
relation
is related to MGNLSITE-23 As an administrator, i don't experien... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Sprint: Sprint 4 (Kromeriz)
Story Points: 5

 Description   

I run into this when testing STK + MTK installed alongside (CE).

  • MTE installed first
  • STK installed second
  • opened travel demo (freemarker errors)
  • then opened STK demo .. this demo should work, but it didn't because:
    • opening the travel demo led to rendering of htmlHeader - AreaElement sets renderType to site and i18nBasename to info.magnolia.module.travel-demo.messages on the corresponding AreaDefinition
    • since we are setting these values on the AreaDefinition directly, everytime when we now obtain site's prototype, these values are there leading to incorrect behavior (wrong renderer resolved)
    • workaround to this problem is to open site we have configured in the site module as active

Erroneous code (info.magnolia.templating.elements.AreaElement):

// FIXME we shouldn't manipulate the area definition directly
// we should use merge with the proxy approach
if (areaDefinition instanceof ConfiguredAreaDefinition) {
    if (areaDefinition.getTemplateScript() == null) {
        ((ConfiguredAreaDefinition) areaDefinition).setRenderType("noscript");
    } else if (areaDefinition.getRenderType() == null) {
        ((ConfiguredAreaDefinition) areaDefinition).setRenderType(this.templateDefinition.getRenderType());
    }

    if (areaDefinition.getI18nBasename() == null) {
        ((ConfiguredAreaDefinition) areaDefinition).setI18nBasename(this.templateDefinition.getI18nBasename());
    }
}

As the FIXME comment suggest, we should use merging utility instead of setting the values directly

Possible fix (worked for me):

ConfiguredAreaDefinition definition = new ConfiguredAreaDefinition();
if (areaDefinition instanceof ConfiguredAreaDefinition) {
    if (areaDefinition.getTemplateScript() == null) {
        definition.setRenderType("noscript");
    } else if (areaDefinition.getRenderType() == null) {
        definition.setRenderType(templateDefinition.getRenderType());
    }

    if (areaDefinition.getI18nBasename() == null) {
        definition.setI18nBasename(templateDefinition.getI18nBasename());
    }
}
definition = BeanMergerUtil.merge(areaDefinition, definition);
// Use definition instead of areaDefinition


 Comments   
Comment by Philip Mundt [ 28/Jul/15 ]

Nice find! I have minor input though:

  • Shouldn't we be passing areaDefinition.getTemplateAvailability() to the constructor of info.magnolia.rendering.template.configured.ConfiguredAreaDefinition
    • to not use the deprecated ctor anymore
    • to actually have the template availability of the area
    • EDIT: the merge does it but we shouldn't introduce usage of deprecated code
  • What if templateScript AND renderType are null?
  • Please use NoScriptRenderer.NO_SCRIPT_RENDERER instead of String "noscript"
  • Could you add a simple test that shows the problem/fix
Generated at Mon Feb 12 04:13:17 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.