[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #462: [WW-5021] Introduces a new constant to allow define a path to static content

[GitBox <gi...@apache.org>]

JCgH4164838Gh792C124B5 commented on a change in pull request #462:
URL: https://github.com/apache/struts/pull/462#discussion_r552245545



##########
File path: core/src/main/java/org/apache/struts2/components/UIBean.java
##########
@@ -523,6 +525,11 @@ public void setUIThemeExpansionToken(String uiThemeExpansionToken) {
         this.uiThemeExpansionToken = uiThemeExpansionToken;
     }
 
+    @Inject(StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH)
+    public void setUiStaticContentPath(String uiStaticContentPath) {
+        this.uiStaticContentPath = uiStaticContentPath;

Review comment:
       It looks like the _staticContentPath_ logic depends on the String not ending with/terminating with "/", nor being null or empty.  Maybe the setter should refuse to accept certain "unacceptable" values, or at least produce log warnings if they are encountered ?
   
   For example:
   ```
   if (uiStaticContentPath == null || uiStaticContentPath.isEmpty() || uiStaticContentPath.endsWith("/") {
       throw new IllegalArgumentException("Static content path cannot be null, empty or end with '/'.  Provided value:" + uiStaticContentPath);
       // or
       LOG.warn("Static content path cannot be null, empty or end with '/'.  Provided value:{0}", uiStaticContentPath);
   }
   ```
   Refusal via exception might be safer/cleaner, if it makes sense, since it cannot be ignored.  That behaviour could be unit tested as well.
   
   If exceptions are too disruptive, then maybe log a warning and fall-back (override) to the "/static" default path if an unacceptable value is provided to the setter ?

##########
File path: core/src/main/java/org/apache/struts2/config/entities/ConstantConfig.java
##########
@@ -1344,4 +1346,12 @@ public Integer getOgnlAutoGrowthCollectionLimit() {
     public void setOgnlAutoGrowthCollectionLimit(Integer ognlAutoGrowthCollectionLimit) {
         this.ognlAutoGrowthCollectionLimit = ognlAutoGrowthCollectionLimit;
     }
+
+    public String getStaticContentPath() {
+        return staticContentPath;
+    }
+
+    public void setStaticContentPath(String staticContentPath) {
+        this.staticContentPath = staticContentPath;

Review comment:
       If a guard or log makes sense for `UIBean`, it is the same consideration here.

##########
File path: core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java
##########
@@ -100,20 +110,23 @@
     /**
      * Modify state of StrutsConstants.STRUTS_SERVE_STATIC_CONTENT setting.
      *
-     * @param serveStaticContent
-     *            New setting
+     * @param serveStaticContent New setting
      */
     @Inject(StrutsConstants.STRUTS_SERVE_STATIC_CONTENT)
     public void setServeStaticContent(String serveStaticContent) {
         this.serveStatic = BooleanUtils.toBoolean(serveStaticContent);
     }
 
+    @Inject(StrutsConstants.STRUTS_UI_STATIC_CONTENT_PATH)
+    public void setStaticContentPath(String staticContentPath) {
+        this.staticContentPath = staticContentPath;

Review comment:
       If a guard or log makes sense for `UIBean` and `ConstantConfig`, it is the same consideration here.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org