Background:
This issue is part of the task to update the hook_help texts of the Drupal 8 modules:
#1908570: [meta] Update or create hook_help() texts for D8 core modules
Tasks:
a) review / write the hook_help text according to help guidelines
b) The REST module maintainers need to review the text for accuracy.
c) (novice) Final manual testing:
1. Apply the patch.
2. Go to admin/help.
3. Click on the help page for this module.
4. Verify that the help page is OK:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff-2091415-27-32.txt | 2.14 KB | amitgoyal |
#32 | rest-hep-text-2091415-32.patch | 3.03 KB | amitgoyal |
Comments
Comment #1
pokurek CreditAttribution: pokurek commentedInsert an url to handbook as is, with no sanitization or formatting.
Comment #2
ifrikThe link format is correct, but the some more changes to the wording are needed.
(1) The link to the documentation page should be: "For more information, see the online documentation for the Foo module." according to the help text standard.
(2) " a framework for exposing Drupal's data structures": I think we are trying to avoid the use of "Drupal" in the help texts, so that the texts still appear relevant in other distributions as well. Maybe there is a way of phrasing this more generic?
Comment #3
jhodgdonRegarding (2), if that is the case then we would need to update a lot of other help texts! Many of them are saying Drupal. I wouldn't worry about that right at the moment.
Comment #4
batigolixpatch:
- addresses points raised in #2
- change reference to module (use exact moudle name)
- changes wording in 1st 2 phrases
Comment #5
jhodgdonThis mostly looks great!
However, it seemed a bit scary to me when I read this part: "This allows other applications to remotely read and update entity types like site content"... I don't think that just turning on this module would allow other applications to update your content! So maybe the phrasing here should be something more like "The framework can be used by other modules to allow other applications to remotely ..."?
Also, before "and" we need a comma in the long list of entity types.
Comment #6
batigolixNew patch that addresses point made in #5
Comment #7
batigolixComment #8
jhodgdonLooks great! The http://drupal.org link should be https: (sorry I missed that one before). And I think we should move this over to the REST module so the maintainers can comment.
Comment #9
batigolixhttp --> https
Comment #10
jhodgdonLooks good to me. I think we can safely skip the usual manual test in this case too.
Any comments from the REST module maintainers (klausi or Crell)?
If it looks good to you, please mark RTBC and move it back to the "documentation" component, and I or someone else can get it committed after the 23rd when we're off the "majors and criticals only week".
Comment #11
Crell CreditAttribution: Crell commentedI see 2 problems with #9:
1) Technically this module *does* expose entities on its own; no new module is needed for that. However, it is all opt-in. You need to edit a config file (or use a contrib UI module for now) to actually expose something.
2) The reference to "entity types" worries me. The way it reads, it sounds like you can update the entity type: Vis, change the configuration of the entity type. You can't do that; we deliberately aren't providing support for config files via REST (although one could write such support). What you're updating is the entities, and specifically "content entities". And that's just with REST itself. You can expose *any* resource type through REST module, and we actually offer watchdog logs, too, as an example.
We arguably don't need to go to that level of detail here, but it's at least a little misleading to say "entity types".
Comment #12
jhodgdonOK... Can you propose different text then? The text in #9 was just an attempt to make what was already in the hook_help() a bit more readable (really, it has exactly the same information in it, with slightly different wording, and an expanded list of entity types).
Comment #13
Crell CreditAttribution: Crell commentedSomething like this? (Incidentally, WHY are we not wordwrapping obscenely long strings when we get testy about comment lines that are 80.5 characters?)
Comment #14
jhodgdonI have no idea on wrapping the strings except that we definitely do want them to stay as all one string for translators. Out of scope for this issue anyway.
So... Here's the new proposed text from this patch:
There are several problems, I think:
a) The wording is awkward and I don't think we should use the phrase "out of the box" at all.
b) This now says that the REST module provides support itself for several types of entities and says they can be enabled, but doesn't say how to enable them. Do we need a Uses section?
c) "framwork" is misspelled.
d) We lost the text from the previous patch that said something to the effect that this module doesn't really do anything itself and you'll need to use a different module to actually provide the REST operations. Isn't that true? The text proposed left me thinking that just enabling the REST module would mean you could do REST operations on a site. Now I'm really confused.
Comment #15
BerdirRe #14:
b) It provides support for *content* entities. See also below re uses.
d) Yes, that is exactly what you can do, after manually changing a yml config file, or by using the contrib rest_ui module. The documentation has infos about that, see for example https://drupal.org/node/2096019. You don't need any other modules to integrate with content entities.
Comment #16
jhodgdonIn that case, we definitely need a "Uses" section that describes how to use this module. It should describe how to enable content type support (linking to the contrib module is great as well as saying there is a YML file), and then describing how to make a REST request.
Comment #17
Berdirhttp://lin-clark.com/blog/2014/01/22/setting-up-rest-drupal-8/ should cover what we need here pretty well :)
Edit: And according to that, the node resource is enabled by default, so you don't even have to do that (you still need to grant permissions).
Comment #18
jhodgdonGreat! Then someone just needs to write it up in a couple of concise Uses items. :)
Comment #19
jhodgdonWe just had a change to hook_help, on this issue: #2183113: Update hook_help signature to use route_name instead of path.
Here is the change record: https://drupal.org/node/2250345
This patch will need a reroll for this change.
Comment #20
mparker17Straight re-roll, so no interdiff.
Comment #21
Crell CreditAttribution: Crell commentedIt compiles, ship it.
Comment #22
jhodgdonThere were "needs work" comments starting after the previous patch in #13. I do not believe these have been addressed.
Comment #23
amitgoyal CreditAttribution: amitgoyal commentedI have made the following fixes,
14 a) "out of the box" has been removed.
14 c) "framwork" spelling corrected.
15 b) Added "content entities".
I am not sure what exactly should we add in Uses section as based on this http://drupalize.me/blog/201401/introduction-restful-web-services-drupal-8, there are lot of things.
Comment #24
jhodgdonThanks! That looks pretty good.
As far as other updates... I'm trying to make sense of this... According to https://drupal.org/node/2096019 and comments above, it looks like operations on Node content items are enabled by default, and that page explains how to enable other content entities (it is a sub-page of the main REST module help page).
So... Here's what I think we should do:
a) About -- after this patch, it says:
a.1) First sentence: Can we just take out the "both read and write" part? I don't think we need that.
a.2) In the second sentence, "that can be enabled" should probably say "; REST support for content items of the Node module is enabled by default, and support for other types of content entities can be enabled."
a.3) Third sentence "resource" => "resources".
a.4) I think we should in the 2nd sentence add "(see the (link)Entity module help page(endlink) for more information about entities)".
b) Uses - I think we should have:
b.1) Enabling supporting modules
In order to use REST on a web site, you need to install and enable modules that provide serialization and authentication services. You can use the Core module HAL for serialization and HTTP Basic Authentication for authentication, or install a contributed or custom module.
(those should link to the module help pages for hal and basic_auth)
b.1) Enabling REST support for an entity type
REST support for content items of the Node module is enabled by default, and support for other types of content entities can be enabled. To enable support, you can use a (link)process based on configuration editing(endlink) or the contributed Rest UI module
link to: https://drupal.org/project/restui and https://drupal.org/documentation/modules/rest
You will also need to grant anonymous users permission to perform each of the REST operations you want to be available, and set up authentication properly to authorize web requests.
[We might need some help here with figuring out what to write???]
Comment #25
amitgoyal CreditAttribution: amitgoyal commentedPlease see updated patch where I have added changes as per #24.
I have revised it as per a.1, a.2, a.3, a.4, b.1 and b.2. Do we also need to mention about "Test with a GET request" as per https://drupal.org/node/2096019?
Comment #26
jhodgdonThis looks pretty good to me. Can the maintainers of the REST module please review the text and see if it is accurate? I really am not sure.
Meanwhile, a small fix: In the line about "REST support for items of the...", in the t() arguments,
'!basic_auth' => \Drupal::url('help.page', array('name' => 'basic_auth'))
is not needed for this t() call.Comment #27
amitgoyal CreditAttribution: amitgoyal commentedHere is updated patch for minor fix in #26.
Comment #28
jhodgdonThanks!
I just updated the issue summary with steps needed:
- REST maintainers need to review for accuracy (klausi, Crell are listed in MAINTAINERS; if Lin says it is OK that would be fine too).
- Novice manual test
Comment #29
Crell CreditAttribution: Crell commentedI saw nothing incorrect in the latest patch.
Comment #30
mparker17I performed manual testing.
The code looks good. The terminology looks consistent with the rest of the UI.
I tested all the links. The link to the HTTP Basic Authentication help page returns a 404 if the HTTP Basic Authentication module is not enabled (it is intentionally possible to install REST without HTTP Basic Authentication). However, once HTTP Basic Authentication module is enabled, everything works. Don't know if this should mean "Needs Work" or not.
Comment #31
jhodgdonThanks for testing! Yes, it should be a "needs work".
In other help text, what we've done in cases like this is made the link go to '#' if a module is not installed. There's an example in the Menu module help where it checks for Block module being enabled when it's talking about the blocks for displaying menus. We need to do the same thing here, presumably for both HAL and Basic Auth? (It doesn't look like HAL is a dependency of REST either -- only Serialization, which at least in its info file does not have dependencies?) Thanks for catching that!
Comment #32
amitgoyal CreditAttribution: amitgoyal commentedThanks @mparker17 and @jhodgdon for your feedback!
I have updated the patch for fixes in #30 and tested the various combinations as well.
Comment #33
jhodgdonThat looks correct, and since Amit has tested, I think we can put this back to RTBC. Thanks a lot!
Comment #34
webchickYoink! :)
Committed and pushed to 8.x. Thanks!