[GitHub] [ofbiz-framework] danwatford opened a new pull request #217: Improved: WidgetWorker should not write generated html to Appendable (OFBIZ-11907)
Locked
 
|
danwatford opened a new pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217 Refactoring of WidgetWorker so that it generates URI and JSoup Element objects to represent created URLs, hidden forms and anchor tags. This replaces the previous approach where WidgetWorker would write string representations of the URLS, hidden forms and anchor tags directly to an Appendable object passed to it. Callers to WidgetWorker have been modified to render the new objects created by WidgetWorker to their relevant I/O. ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
JacquesLeRoux commented on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-663956951 Hi Daniel, Sorry "This branch has conflicts that must be resolved" as report GH ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
JacquesLeRoux edited a comment on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-663956951 Hi Daniel, Sorry "This branch has conflicts that must be resolved" as reports GH ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
JacquesLeRoux edited a comment on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-663956951 Hi Daniel, Sorry, "This branch has conflicts that must be resolved" as reports GH ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
danwatford commented on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-663977565 Hi @JacquesLeRoux , conflict has been fixed now. ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
danwatford commented on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-665192736 All fixed @JacquesLeRoux ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
JacquesLeRoux commented on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-665155521 Hi Daniel, Sorry again, "This branch has conflicts that must be resolved" as reports GH. I guess it will be easier to fix for you. TIA :) ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
danwatford edited a comment on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-665611006 I'm don't know how to resolve this @JacquesLeRoux . Before the most recent merge commit a few moments ago I attempted to apply the patch you linked to against trunk and saw the failures you mentioned. As far as I can tell they are due to a difference in whitespace in the trunk compared to the lines expected to be replaced by the patch. I then tried generated another patch using 'git format-patch trunk --stdout > OFBIZ-11907.patch', but saw the exact same problem with then applying the new patch to trunk. The issue appears to be with the whitespace is this part of the patch: ``` - String tokenValue = CsrfUtil.generateTokenForNonAjax(request, target); - if (UtilValidate.isNotEmpty(tokenValue)) { - String currentString = externalWriter.toString(); - if (currentString.startsWith("<form")) { - currentString = currentString.substring(currentString.lastIndexOf("\"")+1); - } - if (currentString.indexOf('?') == -1) { - externalWriter.append("?" + CsrfUtil.getTokenNameNonAjax() + "=" + tokenValue); - } else { - externalWriter.append("&" + CsrfUtil.getTokenNameNonAjax() + "=" + tokenValue); - } + additionalParameters.forEach(uriBuilder::addParameter); + + try { + return uriBuilder.build(); + } catch (URISyntaxException e) { + final String msg = "Syntax error when building URI: " + uriBuilder.toString(); + Debug.logError(e, msg, MODULE); + throw new RuntimeException(msg, e); ``` If a way to resolve this can be found then I think the patch will apply cleanly. Perhaps this is a reason to coordinate large formatting changes across the code base as this is the third time I've tried to fix the merge and we are now getting hit by some subtle diff/patch issue that I'm unable to definitively diagnose. ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
JacquesLeRoux commented on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-665512570 ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
danwatford commented on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-665611006 I'm don't know how to resolve this @JacquesLeRoux . Before the most recent merge commit a few moments ago I attempted to apply the patch you linked to against trunk and saw the failures you mentioned. As far as I can tell they are due to a difference in whitespace in the trunk compared to the lines expected to be replaced by the patch. I then tried generated another patch using 'git format-patch trunk --stdout > OFBIZ-11907.patch', but saw the exact same problem with then applying the new patch to trunk. The issue appears to be with the whitespace is this part of the patch: - String tokenValue = CsrfUtil.generateTokenForNonAjax(request, target); - if (UtilValidate.isNotEmpty(tokenValue)) { - String currentString = externalWriter.toString(); - if (currentString.startsWith("<form")) { - currentString = currentString.substring(currentString.lastIndexOf("\"")+1); - } - if (currentString.indexOf('?') == -1) { - externalWriter.append("?" + CsrfUtil.getTokenNameNonAjax() + "=" + tokenValue); - } else { - externalWriter.append("&" + CsrfUtil.getTokenNameNonAjax() + "=" + tokenValue); - } + additionalParameters.forEach(uriBuilder::addParameter); + + try { + return uriBuilder.build(); + } catch (URISyntaxException e) { + final String msg = "Syntax error when building URI: " + uriBuilder.toString(); + Debug.logError(e, msg, MODULE); + throw new RuntimeException(msg, e); If a way to resolve this can be found then I think the patch will apply cleanly. Perhaps this is a reason to coordinate large formatting changes across the code base as this is the third time I've tried to fix the merge and we are now getting hit by some subtle diff/patch issue that I'm unable to definitively diagnose. ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
JacquesLeRoux edited a comment on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-665591935 OK, GH is not able to show the zipped WidgetWorker.java.rej.zip file in preview above despite saying it's uploaded :/. Trying here rather... OK same "Error rendering preview" in Preview. Not sure how to see this file, trying a trick with changing extension to txt... Nothing works or I don't understand how it works :/ Maybe because I use an old FF version. Trying with Edge... [WidgetWorker.java.rej.txt](https://github.com/apache/ofbiz-framework/files/4994339/WidgetWorker.java.rej.txt) OK works :) ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
JacquesLeRoux commented on pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217#issuecomment-666229891 HI Daniel, That would be perfect, working and updating from trunk HEAD should work, TIA ---------------------------------------------------------------- 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: [hidden email] |
|
|
In reply to this post by GitBox
JacquesLeRoux closed pull request #217: URL: https://github.com/apache/ofbiz-framework/pull/217 ---------------------------------------------------------------- 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: [hidden email] |
«
Return to OFBiz - Notifications
|
1 view|%1 views
| Free forum by Nabble | Edit this page |