CommentFileSizeAuthor
#2 3098966-2.patch1003 bytesanantagoswami

Comments

anantagoswami created an issue. See original summary.

anantagoswami’s picture

Status: Active » Needs review
StatusFileSize
new1003 bytes

this will add hook_help() to this module.

andrewmacpherson’s picture

Status: Needs review » Needs work

Review of patch #2:

  1. modifies the behavior of the node system to return 404s instead of 403s
    This would sound better if it was singular, and clarify that it means a HTTP status. Suggestion: "to return a HTTP 404 status instead of 403"
  2. if a user does not have permission to view a node page
    This isn't quite true. It only changes the HTTP status if the node is unpublished. However there may be other reasons why a user cannot view a particular node. This would be clearer if it said "if a user does not have permission to view an unpublished node page".
  3. by default,
    "By" needs a capital letter because it's the first word in the sentence.
  4. (i.e, only administrator is allowed to view that node)
    This assumption is too specific, and isn't always going to be true. A site may have several roles which can view unpublishnd content, not just the administrator role. I think this part can just be removed, and the sentence will still work.
  5. so this module changes
    "So" needs a capital letter because it's the start of a sentence.
captainpants’s picture

Status: Needs work » Postponed

The simplicity of a module doesn't warrant ahook_help() implementation. We can however revisit this issue if the module's functionality and complexity dramatically increases.

andrewmacpherson’s picture

Simplicity isn't a good argument against providing hook_help().

Unpublished_404 has NO user interface elements, routes, links, pages, permissions, or configuration. That's quite unusual among modules. A user who has just enabled the module may be wondering what to do next, without realizing they don't need to do anything at all.

A couple of core modules offer a precedent here: the Big Pipe and Internal Dynamic Page Cache modules also have no UI or configuration. These modules provide a short help page, which explains that the module requires no configuration.

  • anantagoswami authored 586024d on 8.x-1.x
    Issue #3098966 by anantagoswami: add a hook_help() to this module
    
gaurav.kapoor’s picture

Status: Postponed » Fixed

Since it's good to have the hook_help so that those who install the module are able to know what it does. For now i have pushed the patch as well as accommodated feedback provided in #3.

Thanks everyone.

  • anantagoswami authored 586024d on 8.x-1.x
    Issue #3098966 by anantagoswami: add a hook_help() to this module
    
gaurav.kapoor’s picture

gaurav.kapoor’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.