[MAGNOLIA-5783] Provide replacements for info.magnolia.nodebuilder.task and deprecate the outdated ones Created: 23/May/14  Updated: 04/Jun/14  Resolved: 30/May/14

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

Type: Task Priority: Neutral
Reporter: Daniel Lipp Assignee: Daniel Lipp
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
relation
is related to MAGNOLIA-5797 Align info.magnolia.jcr.nodebuilder.O... Open
is related to MAGNOLIA-3694 Provide Mock-Implementations for JCR-... Closed
Template:
Acceptance criteria:
Empty
Task DoR:
Empty
Date of First Response:

 Description   

Content API has been deprecated a long time ago. Hence info.magnolia.nodebuilder.task has to be replaced by code not exposing content API as well.



 Comments   
Comment by Magnolia International [ 27/May/14 ]

I made some minor Javadoc changes on the branch. Otherwise looks good, except for two things

1. NodeBuilderTask
I'm not sure why in NodeBuilderTask#getRootNode you do this

        final Session session = ctx.getJCRSession(workspaceName);
        if ("/".equals(rootPath)) {
            return session.getRootNode();
        }
        String tmpRoot = StringUtils.removeStart(rootPath, "/");
        return session.getRootNode().getNode(tmpRoot);

instead of simply this

        final Session session = ctx.getJCRSession(workspaceName);
        return session.getNode(rootPath);

The only difference I can see is that the former allows non-absolute paths, which would still be relative to the root (e.g. modules/core would be accepted)), which is misleading/wrong IMO. Do we have cases where we depend on this behavior ?

It sounds like the kind of things we were doing in our Content API and I'd rather not go back there, and rely on the more predictable behavior of the JCR API:

mgnl> s=MgnlContext.getJCRSession('config')

mgnl> s.getNode('/')
===> node /
mgnl> s.getRootNode()
===> node /
mgnl> s.getNode('/') == s.getRootNode()
===> true

mgnl> s.getNode('modules/cache')
RepositoryException: Not an absolute path: modules/cache
mgnl> s.getNode('/modules/cache')
===> node /modules/cache
mgnl> s.getNode('/modules/cache/')
===> node /modules/cache

2. Changes in Ops
Are fine but would IMO warrant a separate issue and commit. They also

  • re-introduce some content-api-ness that i'd rather see go away (let Ops.addNode(name) be consistent with the method of the same signature in javax.jcr.Node rather than reproduce our Content API smells)
  • aren't complete (there are still methods in the content-api-based version that haven't been ported
    • setProperty with expected value (which is IMO very useful in update tasks)
    • copyNode (we kept moveNode but not copyNode ?)
    • onChildNodes (debatable, and might be better off in a different class altogether)
    • info.magnolia.nodebuilder.ContentOps, which were not operations specific to the Content API but to "Content" as a general term (createParagraph, etc) - also debatable, but at the very least the deprecation message is wrong . Some of these could be useful, but I guess 1) they should be closer to the templating/rendering modules 2) they were an "idea" more than a real need - where the vision was maybe that other things could provide "Ops" (newDialog, etc)

I think that's it

Comment by Daniel Lipp [ 30/May/14 ]

1.) I dropped the unwanted Content-API-backwards compatibility

2.) the idea wasn't to fix all defects in Ops - I simply realized there's a bug in Ops#doExec so I went ahead and fixed it for this ticket. I now created a isolated ticket for it (MAGNOLIA-5797) and will modify the commit message of the Ops changes accordingly before merging to master

Glad if you could create a separate ticket listing the further shortcomings of the Ops class.

Comment by Magnolia International [ 30/May/14 ]

Re-reviewed, merged and squashed to master.
I left out the commit that modified the behavior of addNode, as well as the one adding remove(), since that's for MAGNOLIA-5797 (which I re-used and edited rather than created yet another ticket)

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