mraible commented on code in PR #244:
URL: https://github.com/apache/roller/pull/244#discussion_r4545053636
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileView.java:
##########
@@ -193,6 +193,19 @@ public String createNewDirectory() {
*
* @return String The result of the action.
*/
+ /**
+ * Returns the weblog's default media folder, creating it if it is missing.
+ * Weblogs created by old Roller versions may not have one.
+ */
+ private MediaFileDirectory defaultDirectory(MediaFileManager manager)
throws WebloggerException {
Review Comment:
This helper landed between `execute()`'s Javadoc ("Shows a page of the
library ... @return String The result of the action.") and `execute()`. That
leaves two doc comments stacked: `execute()` loses its docs and the helper
picks up a stray `@return`. Could it go above that comment or below `execute()`?
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileImageChooser.java:
##########
@@ -100,6 +100,11 @@ public String execute() {
if (directory == null) {
directory =
manager.getDefaultMediaFileDirectory(getActionWeblog());
}
+ if (directory == null) {
Review Comment:
"Get the default media folder, create it if missing, flush" now appears in
five places: MediaFileView, MediaFileImageChooser, EntryEdit, MediaFileAdd and
`JPAMediaFileManagerImpl` around line 792. Any new caller that skips the null
check will get an NPE on weblogs from older versions. Some existing paths still
don't handle it fully (`MetaWeblogAPIHandler` around 377, `MediaCollection`
around 129 and 304). It also means a plain GET to the chooser or library writes
rows. Would a single get-or-create method on `MediaFileManager` (or a one-time
migration step) work, with all callers using it?
##########
app/src/main/java/org/apache/roller/weblogger/ui/rendering/servlets/PageServlet.java:
##########
@@ -504,6 +505,17 @@ public void doGet(HttpServletRequest request,
HttpServletResponse response)
log.debug("Looking up renderer");
renderer = RendererManager.getRenderer(page,
pageRequest.getDeviceType());
+ } catch (RenderingException e) {
Review Comment:
`RendererManager.getRenderer` throws `RenderingException("No renderer
found")` for every no-renderer case, including when `VelocityRendererFactory`
has already logged an ERROR constructing `VelocityRenderer`, or
`VelocityRenderer` has logged a ResourceNotFound WARN. So the "template may
have no content" hint can point at the wrong cause, and empty templates still
log two lines rather than the one CHANGES.md mentions. This also leaves the old
`catch (Exception)` almost unreachable, and the reset/404 body is copied here
and in `PreviewServlet`. One option is to have the factory lookup return
null/Optional and log once, with the shared 404 handling moved into a helper.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
