Adobe-Consulting-Services / acs-aem-commons

http://adobe-consulting-services.github.io/acs-aem-commons/
Apache License 2.0
451 stars 597 forks source link

Sharedcomponet Enhancement #1032

Closed kishore4784 closed 7 years ago

kishore4784 commented 7 years ago

My Requirement:

1) Shared and global dialogs have to be enabled on all the components on all pages under /content. 2) store the values of shared and global properties under /content/jcr:content/shared properties /content/jcr:content/global properties 3) And whenever we use wcm object tags for example : ${sharedProperties.titleText} it has to get the value from /content/jcr:content/shared properties ( I mean where we stored in step 2).

I am able to acheive requirement 1 and 2 by changing the ACS commons java code a little. But unable to retrieve the values from the location where i stored.

kishore4784 commented 7 years ago

I tried by giving page root as /content in pagerootprovider config, but not working.

davidjgonzalez commented 7 years ago

@HitmanInWis not sure if you have thoughts?

HitmanInWis commented 7 years ago

I think I need a bit more details. What code changes were needed to make #1 and #2 work? The issue may be that the PageRootProvider code assumes the page "root" would be a site homepage, and thus a cq:Page whereas /content is a folder.

kishore4784 commented 7 years ago

@HitmanInWis To acheive #1 and #2 i add the below code props.put("enabled", true); props.put("root", "/content"); props.put("components", Maps.transformValues(componentsWithSharedProperties, (Function<List, Object>) JSONArray::new));

at line number 134 in the SharedComponentPropertiesPageInfoProvider.java

HitmanInWis commented 7 years ago

Ok, that confirms my original thoughts. The reason you are finding yourself in need of adding those lines is likely that PageRootProvider is returning a null page as your content root because /content is a folder and not a page. If you truly need to use /content as the root for your properties, then I think instead of your 3 lines of code we'd need to update PageRootProvider since that is what's used for determining where the values are set (your #1 and #2) and how they get read by SharedComponentPropertiesBindingsValuesProvider (your #3).

Probably what we would need to do for /content (or any other non-page) to be a valid root for shared component properties would be to: 1) Add a getRootResource() function to PageRootProvider similar to getRootPage() but that returns a Resource that has no need of being a page. For resources that are a page, it may want to directly return the jcr:content node below. 2) Update SharedComponentPropertiesPageInfoProvider and SharedComponentPropertiesBindingsValuesProvider to work with getRootResource() instead of getRootPage().

Cautions about the approach include: 1) This disallows different sites to have different values when necessary. 2) Putting global properties on the /content folder versus a /content/mysite/ page makes it so that the properties cannot support internationalization (i18n). 3) To not subvert PageRootProvider for other functionality such as getting a link to the home page or for use by the Contextual Pathbrowser dialog widget (acs-commons/touchui-widgets/contextualpathbrowser) you would need to configure PageRootProvider with both "/content" and "/content/mysite/something" so that code could fallback to the second pattern when it truly wants the home page. This could be confusing.

Can you give some more insight into your use case? Another approach that could be considered is creating a new level of shared/global properties. We currently have "shared" for a single sling:resourceType within a single site and "global" for all sling:resourceType within a single site, but maybe we could use another level with "aemshared" and "aemglobal" that are similar to the site-specific counterparts but are instead shared across all of AEM. If we went that route, though, we still would want to determine a strategy for supporting i18n else we are really not very different from OSGi configurations (other than being authorable by folks other than an admin on the Felix console).

kishore4784 commented 7 years ago

@HitmanInWis I tried as you suggested it is showing me multiple errors. can you help me with this code.

@Override public Resource getRootResource(Resource resource) { String pagePath = getRootPagePath(resource.getPath());

    if (pagePath != null) {
       PageManager pageManager = resource.getResourceResolver().adaptTo(PageManager.class);
        Resource rootPage = pageManager.getPage(pagePath);
     if (rootPage == null) {
            log.debug("Page Root not found at [ {} ]", pagePath);
        } else if (!rootPage.isValid()) {
            log.debug("Page Root invalid at [ {} ]", pagePath);
        } else {
            return rootPage;
        }
    }

    return null;
}
HitmanInWis commented 7 years ago

I think this should work:

    @Override
    public Resource getRootResource(Resource resource) {
        String rootPath = getRootPagePath(resource.getPath());

        if (rootPath != null) {
            Resource rootResource = resource.getResourceResolver().getResource(rootPath);
            if (rootResource == null) {
                log.debug("Root resource not found at [ {} ]", rootPath);
            } else {
                return rootResource;
            }
        }

        return null;
    }

Be sure to add public Resource getRootResource(Resource resource) to the PageRootProvider interface, and then you still need to update SharedComponentPropertiesPageInfoProvider and SharedComponentPropertiesBindingsValuesProvider to work with getRootResource() instead of getRootPage().

davidjgonzalez commented 7 years ago

@HitmanInWis / @kishore4784 - can we close this issue? Is this something we'd want to add into SCP framework?

HitmanInWis commented 7 years ago

I think we should close this issue for now until we know more people are running into similar issues. If more similar use cases come up perhaps we can determine a comprehensive solution.

davidjgonzalez commented 7 years ago

Sounds good.

@anyone - if you stumble upon this thread and need this solution please reopen with comments, and we can discuss further.