LenyaVersion (src/java/org/apache/lenya/cms/workflow/LenyaVersion.java) and VersionImpl (src/modules-core/workflow/java/src/org/apache/lenya/workflow/impl/VersionImpl.java) are identical. LenyaVersion is used in src/java/org/apache/lenya/cms/workflow/DocumentWorkflowable.java and VersionImpl is used in src/modules-core/workflow/java/src/org/apache/lenya/workflow/impl/WorkflowEngineImpl.java. LenyaVersion or VersionImpl should be removed before the code freeze.
That's not trivial (unless I'm missing an obvious solution). Since it doesn't imply any functional problems, I'd say we defer it to 1.4.1.
can you explain what makes this duplication necessary? i can see how the core should not depend on a module, but i'm not sure about the details. would moving the DocumentWorkflowable into a module help? while we're at it: what other functionality should be modularized before 1.4? should we maybe get rid of the old "core" entirely?
Created attachment 20037 [details] Removes VersionImpl This patch causes the code to use LenyaVersion instead of VersionImpl. Builds and runs fine.
I'm not caught up enough to understand what exactly the src/java tree is. I figure the modules should be able to depend on it. My patch is based on that assumption. Alternatively, instead of removing a file that others may be using. Have VersionImpl extend LenyaVersion and wipe out VersionImpl's code. This will preserve the file, the methods presented, and operation while making any further updates happen in one place. This might be the preferred route to get this into the 1.4.0 release.
Thanks for the patch! But there's a little problem (which is the reason why I didn't make this change yet): +++ src/modules-core/workflow/java/src/org/apache/lenya/workflow/impl/WorkflowEngineImpl.java (working copy) @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.List; +import org.apache.lenya.cms.workflow.LenyaVersion; The code in org.apache.lenya.workflow is meant to be generic for SoC reasons, i.e. it shouldn't know anything about the CMS. That's why it shouldn't import anything from o.a.l.cms.workflow. IMO a clean solution would be to use an abstract factory to generate version objects. The o.a.l.workflow package uses versionFactory.createVersion(...) The o.a.l.cms.workflow package provides an implementation which generates LenyaVersion objects.
hmm. i see your point, but code duplication is the far greater evil imnsho. how about taking richard's patch with a loud comment on how it should eventually be refactored, and a low-priority enhancement bug?
(In reply to comment #6) > hmm. i see your point, but code duplication is the far greater evil imnsho. Code duplication is not really evil. Knowledge duplication is, but IMO this is not the case here. > how about taking richard's patch with a loud comment on how it should > eventually be refactored, and a low-priority enhancement bug? I'd rather take the time to implement a factory, or think of a better approach.
Thanks for explaining. My confusion came from the fact that both files import o.a.l.workflow.Version which is under the same file tree as o.a.l.cms.workflow.LenyaVersion. While there might be a separation of code in concept, I didn't see it reflected on the file system.