Create hook_help for Datetime module

Hey

Here the patch.

Thanks for the patch! It's a great start. I have a few comments:

a) In reading the first About sentence... The fact that the module provides a form element is only of interest to programmers, and the vast majority of the readers of help will not be programmers (nor will the vast majority of programmers read the help). So I think we need to make clear to the non-programmers what that means. Maybe the first sentence should talk about the field, and then there could be a second sentence saying something like "... also provides a Form API element called 'datetime' for use in programming modules'" or something like that.

b) Similarly, we want to refer to the field using the label that non-programmers see in the user interface, which is "Date". This applies throughout the help text. Also the formatters should be documented using their displayed names, which are "Plain" and "Default".

c) I'm also uncomfortable with saying it's a "idealized naive date"... What exactly is naive about it? How about just saying that the field can store either dates or dates and times -- people will understand that?

d) URLs need to be reformatted. See https://drupal.org/node/632280#url-note

e) Probably the Validating heading in Uses should end in "values" not "value"?

Hello

Many thanks for the support.

Here is a new patch following your advices.

Thanks! Looks better! A few things still to fix:

a) I forgot this in the previous review, but the link to the online documentation is not following our template:
https://drupal.org/node/632280

b) From #2:

b) Similarly, we want to refer to the field using the label that non-programmers see in the user interface, which is "Date". This applies throughout the help text. Also the formatters should be documented using their displayed names, which are "Plain" and "Default"."

This is still a problem. The first sentence in About refers to a "datetime" field, for instance -- it's not called "datetime" in the user interface. That needs to say "Date" instead. And probably in that same sentence, it should say it can store both dates and times. Then you can get rid of the last sentence in About. Similar changes need to be made in the rest of the text.

c) This sentence needs some grammatical editing:
The plain text which uses the ISO format and the defaut which support built-in human readable formats.
It's not a sentence -- remove the words "which" and it would be. Also "default" is misspelled, and "support" should be "supports". And if you are going to say "the", you need to say "The plain text format" not just "the plain text". Same for default. And human-readable needs to be hyphenated.

d) There is too much "built-in" and "by default". Just tell what the module does.

Thanks!

Patch addresses points raised in #5

I am still in doubt about the points Displaying dates and Validating dates in the Uses section. They do not contain very much information

Datetime module has no hook_help currently, so this is a Critical issue (violates Core Gates). Moving to that component so we can get the maintainers to approve the text.

Other review thoughts:

a)

The Datetime module allows you to create fields for storing dates and time.

Should probably be "dates and times"?

b)
In About, the link to "Field module help" has the word "module" in it, but "Field UI help" doesn't. Let's be consistent. (Oh no, is that just copy/pasted from all the other field module help pages?!?)

c) In About... it looks like this module creates both a "datetime" and "datelist" element, and it would also be more correct to say that they are for the "Form API" not "form API".

d) In the UI, the field type is "Date", not "date", so it should be referred to that way in the help. It might be useful to mention this in the About as well, because it says the field is for dates and times, so it might not be obvious that a field called "Date" is used for that.

e) Under Displaying, "fotmatter" is misspelled.

f) I don't think we need the "Validating" item under Uses. I think "Displaying" is fine, but maybe we should explain what the ISO format is or provide a link? And I wouldn't say that the ISO format is not "human readable" really. I find it quite easy to read myself. :) Maybe we can say something like that the "Default" choice lets you choose which format to use, and also provide a link to the date settings page where you can define the formats? I'm assuming it lets you choose from the date formats you have defined for your system...

g) Under Displaying... Before "which" you need to have a comma, and "human-readable" should be hyphenated, if we leave that in there.

Patches addresses points in #6:

a) done
b) done & oh yes, it is like that in the "template text" for field type modules
c) done
d) done
e) done
f) done
g) these were removed from the text

Maybe in About "allows storing" should be "stores" in "The Datetime module provides a Date field that allows storing dates and times."?

And then in that 2nd About sentence, you list two Form API elements, so it should say "It also provides the Form API elements datetime and datelist..." not "element".

Everything else looks great! This will need a manual review to makes sure the links work and formatting is OK.

Points mentioned in #8 fixed

The manual review (#8) still needs to be done

Looks great to me! Now ready for a manual test:
- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.

I'm not sure who the maintainers of this module are -- it's not in MAINTAINERS.txt that I can see -- but if someone is maintaining it and could review the patch, that would also be good.

I gave this a manual test and it looks good.

One thing that confused me though and I don't know the answer. When choosing the formatting options for a Date field on Manage Fields, I have a lot more options available besides the 3 that are on the Date and Time config page. This is not mentioned in the help and I have no idea what they are.

Do you know where they come from? Should we mention them in the help?

@jhodgdon not sure which fields you are referring. I can see 'Allowed number of values' with select and text field which is default option for all fields and doesn't need any documentation update.

RTBC as the help text looks good to me as well.

Adding screenshot for #13

Sorry for not being clear! In #12, I was talking about the *formatting* options (Manage Display). The help in the latest patch says:

If you choose the Default formatter, you can choose a format from a predefined list that can be managed on the Date and time formats page.

However, when I go to the date and time formats page, I see only 3 options, but on Manage Display, I have more options. I don't know where these are coming from, and the help is not accurate as it is.

erm, yeah, you mean that long list

i'll have a look

Unfortunately I cannot figure this out. They are constructed in the class class DateTimeDefaultFormatter (line 155)

$format_types = $this->dateStorage->loadMultiple();

Too much OO magic for me :(

Yes. I think we need the maintainers of the Datetime module to explain what is going on here and propose help wording to explain it, because I have no idea either.

Unfortunately I am not sure who those maintainers are. :(

We just had a discussion in IRC about this with Berdir and Gabor...

Apparently the list does come from the same place, but the Manage Display list includes "locked" formats, and the Date and Time Formats admin page does not currently show them. Plus, the names are not displayed the same way on the two pages.

I have filed an issue to address that:
#2191189: UI disconnect between Date Time Formats admin page and Manage Display for the field
and am postponing this issue until it is addressed, because I don't think we can write coherent help until that is decided and the UI may be changing.

It is loading all config entities of type 'date_format' (type defined in /core/modules/system/lib/Drupal/system/Entity/DateFormat.php). The config entities of this type(YML file with prefix 'system.date_format') can be defined from any module. For core, we got just system module(e.g. /core/modules/system/config/system.date_format.html_date.yml) implementing all the formats listed.

This should not be blocked on #2119903: Show locked date formats in the UI. We should add a hook_help() that explains how things work, and close this *critical*.

Then, if we choose to fix the other issue, we can update the help text.

I don't really see how we can write sensible help text until the UI is behaving in a sensible manner that can be explained to mere non-programming mortals, but OK.

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.

Here we go: reroll + Manual testing

I think this looks fine, except for the concerns above (starting in comment #12). This help does not match what the module is currently doing, and since postponing it until the UI makes sense is apparently not acceptable to other Core developers, we need to try to document what the UI is currently doing. I don't see how we can explain in help something that makes no sense, but ... that is apparently what we need to do. Then when the new patch that hopefully makes it more sensible gets in (it's at "needs review") they will have to make sure to update the help so that it explains what the UI does again.

Good luck with that.

We need to change this line:

If you choose the Default formatter, you can choose a format from a predefined list that can be managed on the Date and time formats page.

I do agree with @jhodgdon that it doesn't make any sense to change this instead of waiting for #2119903: Show locked date formats in the UI.

So should this issue be postponed then until that (RTBC) one goes in?

xjm: see #21 and take up your argument with tim.plunkett about postponing. To me that is the only thing that makes sense.

Committed the other issue.

yay! thanks @catch. The patch in #26 makes sense now and apply without any conflict.

26: 2091397-datetime-help-26.patch queued for re-testing.

Great! So this just needs a final manual test:
- 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.

Links are fine and the help page looks good.

Thanks for testing!

Committed and pushed to 8.x. Thanks!