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.

Comments

pokurek’s picture

Status:Active» Needs review
StatusFileSize
new1.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,889 pass(es).
[ View ]

Insert an url to handbook as is, with no sanitization or formatting.

ifrik’s picture

Status:Needs review» Needs work

The 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?

jhodgdon’s picture

Regarding (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.

batigolix’s picture

Issue summary:View changes
Status:Needs work» Needs review
Parent issue:» #1908570: [meta] Update or create hook_help() texts for D8 core modules
StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 60,140 pass(es).
[ View ]
new1.15 KB

patch:

- addresses points raised in #2
- change reference to module (use exact moudle name)
- changes wording in 1st 2 phrases

jhodgdon’s picture

Title:Create hook_help for Restful Web Services module» Update hook_help for Restful Web Services module
Status:Needs review» Needs work

This 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.

batigolix’s picture

StatusFileSize
new1.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,881 pass(es).
[ View ]
new1.25 KB

New patch that addresses point made in #5

batigolix’s picture

Status:Needs work» Needs review
jhodgdon’s picture

Component:documentation» rest.module
Status:Needs review» Needs work

Looks 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.

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new1.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,869 pass(es).
[ View ]

http --> https

jhodgdon’s picture

Looks 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".

Crell’s picture

I 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".

jhodgdon’s picture

Status:Needs review» Needs work

OK... 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).

Crell’s picture

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,858 pass(es).
[ View ]

Something like this? (Incidentally, WHY are we not wordwrapping obscenely long strings when we get testy about comment lines that are 80.5 characters?)

jhodgdon’s picture

Status:Needs review» Needs work

I 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:

The RESTful Web Services module provides a framwork for exposing REST resources on your site, both read and write. Out of the box it provides support for entities such as content, users, taxonomy terms, etc. that can be enabled. Other modules may add support for other types of REST resource. For more information, see the online documentation for the RESTful Web Services module.

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.

Berdir’s picture

Re #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.

jhodgdon’s picture

In 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.

Berdir’s picture

http://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).

jhodgdon’s picture

Great! Then someone just needs to write it up in a couple of concise Uses items. :)

jhodgdon’s picture

We 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.

mparker17’s picture

StatusFileSize
new1.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,760 pass(es).
[ View ]

Straight re-roll, so no interdiff.

Crell’s picture

Status:Needs work» Reviewed & tested by the community

It compiles, ship it.

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

There were "needs work" comments starting after the previous patch in #13. I do not believe these have been addressed.

amitgoyal’s picture

StatusFileSize
new1.26 KB
new1.27 KB

I 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.

jhodgdon’s picture

Thanks! 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:

The RESTful Web Services module provides a framework for exposing REST resources on your site, both read and write. It provides support for content entities such as content, users, taxonomy terms, etc. that can be enabled. Other modules may add support for other types of REST resource. For more information, see the online documentation for the RESTful Web Services module.

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???]

amitgoyal’s picture

StatusFileSize
new2.99 KB
new2.99 KB

Please 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?

jhodgdon’s picture

This 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.

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new2.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,276 pass(es).
[ View ]
new2.02 KB

Here is updated patch for minor fix in #26.

jhodgdon’s picture

Issue summary:View changes
Issue tags:+Novice, +Needs manual testing

Thanks!

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

Crell’s picture

Status:Needs review» Reviewed & tested by the community

I saw nothing incorrect in the latest patch.

mparker17’s picture

I 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.

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

Thanks 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!

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new3.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,777 pass(es).
[ View ]
new2.14 KB

Thanks @mparker17 and @jhodgdon for your feedback!

I have updated the patch for fixes in #30 and tested the various combinations as well.

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

That looks correct, and since Amit has tested, I think we can put this back to RTBC. Thanks a lot!

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Yoink! :)

Committed and pushed to 8.x. Thanks!

  • webchick committed d70d27c on 8.x
    Issue #2091415 by amitgoyal, mparker17, Crell, batigolix, pokurek |...

Status:Fixed» Closed (fixed)

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