[MAGNOLIA-5205] SetPropertyTask should be able to set property of any type Created: 26/Jul/13  Updated: 18/Dec/13  Resolved: 02/Aug/13

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: 4.5.9
Fix Version/s: 4.5.10, 5.0.2, 5.1

Type: Improvement Priority: Neutral
Reporter: Robert Šiška Assignee: Milan Divilek
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
causality
is causing MGNLRSSAGG-92 Fix RSSModuleFeedGeneratorTest Closed
relation
is related to MGNLFORM-150 Add in a version handler the delta be... Closed
is related to MAGNOLIA-5194 NewPropertyTask should be able to set... 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)
Date of First Response:

 Description   

Bootstrapped configs of freshly installed modules can bring properties of any type while update tasks can only create String properties. That causes inconsistency between updated and installed modules.



 Comments   
Comment by Jan Haderka [ 27/Jul/13 ]

I really don't like casting type to Object and then later checking for type and casting it back. Type checking and casting are very slow if nothing else.
Can't you for example add type parameter to the private constructor to avoid cast to object and later use this type parameter to know of what type the parameter is? Or even have multiple property fields each with different type and later you set the only one that is not null to avoid casting altogether.

Comment by Magnolia International [ 28/Jul/13 ]

I was going to reopen for the same reason but not with the same reasoning Personally I don't think performance is an issue at risk here. What bothers me is the code duplication - we have this logic of determining a property type by a value object duplicating in a few places already. Please avoid that.

Moreover, this will introduce unwanted bugs if existing properties is of a different type. NewPropertyTask was a good candidate for such a change, and so would CheckAndModifyPropertyValueTask - since it enforces a check on the current value before modifying it.

Comment by Magnolia International [ 28/Jul/13 ]

Missing fix version for 5.0.x and 5.1, missing commit in 5.0.x branch

Comment by Milan Divilek [ 01/Aug/13 ]

Reopen: Don't make constructor with Object public.

Comment by Milan Divilek [ 01/Aug/13 ]

Wrong ticket number was used in commit message.
git commit 4.5.x branch: https://git.magnolia-cms.com/gitweb/?p=magnolia_main.git;a=blobdiff;f=magnolia-core/src/main/java/info/magnolia/module/delta/SetPropertyTask.java;h=d2116ceab6076db36ea96275efb741ee1a723c87;hb=e0b32c7dded1a0de7e24c203400c5494ce6dec7c;hp=741300a87c3a9586f198302cdd14c4713283db4e;hpb=6a7557e3e7ddfb99425b2d46db118001a0cdfe7b
git commit 5.0.x branch: https://git.magnolia-cms.com/gitweb/?p=magnolia_main.git;a=blobdiff;f=magnolia-core/src/main/java/info/magnolia/module/delta/SetPropertyTask.java;h=d2116ceab6076db36ea96275efb741ee1a723c87;hb=d163aeddff0faf99b65593e443c7fcf2b26824c0;hp=741300a87c3a9586f198302cdd14c4713283db4e;hpb=3f5dd4a422813561e110834c4c606127c8528a4b
git commit master branch: https://git.magnolia-cms.com/gitweb/?p=magnolia_main.git;a=blobdiff;f=magnolia-core/src/main/java/info/magnolia/module/delta/SetPropertyTask.java;h=d2116ceab6076db36ea96275efb741ee1a723c87;hb=cda3765d3802019ff382858c2aca855c1e79c2a3;hp=741300a87c3a9586f198302cdd14c4713283db4e;hpb=42bbfd1e1410198d7bc5c6e1118b17c85ead0697

Comment by Milan Divilek [ 01/Aug/13 ]

Reopen: HierarchyManager.getContent(nodePath) support relative path from root node and absolute path. But after rewrite to session api only absolute path works. So backward compatibility is broken.

Generated at Mon Feb 12 04:02:58 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.