[MGNLSTK-973] ThemeInstallTask will never update a mobile theme configuration, missing boostratp task Created: 18/Jun/12  Updated: 22/Apr/13  Resolved: 19/Apr/13

Status: Closed
Project: Magnolia Standard Templating Kit (closed)
Component/s: themepop
Affects Version/s: 2.0
Fix Version/s: 2.0.10

Type: Bug Priority: Critical
Reporter: Christian Ringele Assignee: Ondrej Chytil
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
relation
is related to MGNLSTK-972 ThemeInstallTask should not depend on... Closed
Template:
Acceptance criteria:
Empty
Date of First Response:

 Description   

The concept of the ThemeVersionHandler is to reinstall the whole theme on any update of the module.

But the ThemeInstallTask just bootstraps the base theme configuration, but not a mobile theme.
This didn't show up, because on module installation both configurations are bootstrapped by the BaseInstallTasks of the DefaultModuleVersionHandler (basicInstallTasks.add(new ModuleBootstrapTask()).

But on update, only the 'base' theme (the one defined in the themes module descriptor) is re-bootstrapped:

new BootstrapSingleResource("", "", "/mgnl-bootstrap/theme-" + themeName + "/config.modules.standard-templating-kit.config.themes." + themeName + ".xml", ImportUUIDBehavior.IMPORT_UUID_COLLISION_REPLACE_EXISTING).execute(ctx);

Solution:
I think most proper solution would be defining two properties within the module descriptor, one for the base theme, and one for the mobile theme.

  <properties>
    <property>
        <name>themeName</name>
        <value>pop</value>
    </property>
    <property>
        <name>mobileThemeName</name>
        <value>pop-mobile</value>
    </property>
  </properties>

Using an custom task which checks if the property 'mobileThemeName' is defined, and if so bootstraps it.
(For the case you're not using a mobile theme)



 Comments   
Comment by Christian Ringele [ 18/Jun/12 ]

Is related to it, because it also should check if the theme configuration exists on install time of the module.

Comment by Jan Haderka [ 10/Apr/13 ]

Firstly implemented solution is different then proposal in the ticket, so you should update the ticket as well.
Secondly there is no test.
And last, but not least, this implementation is overzealous and will bootstrap also incorrect files. The beginning of the file should always be config.modules.standard-templating-kit.config.themes. otherwise you might accidentally bootstrap also js or css or other files w/ theme name in it.

Comment by Ondrej Chytil [ 11/Apr/13 ]

File type is restricted to xml with check for name end.
Adding the beginning of file name - ok, but then still is possible to bootstrap some other file which meets the condition. Also whole thing is then quite hardcoded.

Comment by Jan Haderka [ 12/Apr/13 ]

Adding the prefix i suggested ensures that only files that are bootstrapped into config:/modules/standard-templating-kit/config/themes/ folder would be bootstrapped. So really just the theme related content. Whether theme is all in one bootstrap file or in many is irrelevant in this case. Since Magnolia will not recognize themes bootstrapped elsewhere it is actually correct to limit task functionality to this path only.

Comment by Ondrej Chytil [ 18/Apr/13 ]

Proposed solution was not used.
Fixed by implementing inner check for bootstrapped files to determine if they belong to theme or its variations based on module and theme name.

Comment by Jan Haderka [ 19/Apr/13 ]

OK, I'm being picky here, but really is there any reason why don't you set those params in constructor of the filter and forget about the whole set/get list of methods? It's not like you will modify those values ever after creation of the filter.

Generated at Mon Feb 12 07:32:10 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.