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"
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".
by default,
"By" needs a capital letter because it's the first word in the sentence.
(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.
so this module changes
"So" needs a capital letter because it's the start of a sentence.
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.
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.
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.
Comments
Comment #2
anantagoswami commentedthis will add hook_help() to this module.
Comment #3
andrewmacpherson commentedReview of patch #2:
modifies the behavior of the node system to return 404s instead of 403sThis 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"
if a user does not have permission to view a node pageThis 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".
by default,"By" needs a capital letter because it's the first word in the sentence.
(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.
so this module changes"So" needs a capital letter because it's the start of a sentence.
Comment #4
captainpants commentedThe simplicity of a module doesn't warrant a
hook_help()implementation. We can however revisit this issue if the module's functionality and complexity dramatically increases.Comment #5
andrewmacpherson commentedSimplicity 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.
Comment #7
gaurav.kapoor commentedSince 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.
Comment #9
gaurav.kapoor commentedComment #10
gaurav.kapoor commented