Background:
This issue is part of the task to update/create the hook_help texts of the modules for Drupal 8:
#1908570: [meta] Update or create hook_help() texts for D8 core modules

Tasks:
- write the hook_help function
- review d.o. docs at https://drupal.org/documentation/modules/telephone

Files: 
CommentFileSizeAuthor
#23 2078813-22-field-help.patch3.68 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 58,485 pass(es).
[ View ]
#20 2078813-20-field-help.patch2.89 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] 58,946 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#19 2078813-18-field-help.patch893 bytesmr.baileys
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#8 telephone-help-text-2078813-7.patch2.01 KBJulienD
PASSED: [[SimpleTest]]: [MySQL] 58,646 pass(es).
[ View ]
#6 telephone-help-text-2078813-6.patch2.02 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 58,732 pass(es).
[ View ]
#3 telephone-help-text-2078813-3.patch1.94 KBifrik
FAILED: [[SimpleTest]]: [MySQL] 59,036 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#3 interdiff-2078813-1-3.txt2.94 KBifrik
#1 telephone-help-text-2078813-1.patch1.91 KBifrik
PASSED: [[SimpleTest]]: [MySQL] 58,007 pass(es).
[ View ]

Comments

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

I've added hook_help into the module and wrote a first version of the help, following the example of #2028799: Improve help for link module for field modules.
Currently the display as a link only strips out spaces but leaves all other characters in. There are two related issues, so in case the functionality gets changed, the help text need to be updated as well: #2004144: tel: link is impropperly formated and #2004142: Vanity numbers may need conversion to numbers.

I've added this as issue for documentation, but if it should be listed for telephone.module then this needs to be changed.

Status:Needs review» Needs work

This says "link field" instead of "telephone field". Also, I don't think the link to the online docs is following our template.

Also, in the Uses section about making them links, I think it should be made clear that you do this from the Manage Display page.

Status:Needs work» Needs review
StatusFileSize
new2.94 KB
new1.94 KB
FAILED: [[SimpleTest]]: [MySQL] 59,036 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

fixed

See #2028799-25: Improve help for link module comments 25-28 ... probably applies here as well?

Status:Needs review» Needs work

The last submitted patch, telephone-help-text-2078813-3.patch, failed testing.

Component:documentation» telephone.module
Status:Needs work» Needs review
StatusFileSize
new2.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,732 pass(es).
[ View ]

Patch:

- fixes comment #4
- changes url() top \Drupal::url()
- changes @tokens to !tokens

Also changing component to get feedback from others

Status:Needs review» Needs work

This looks good. Just a couple of small things to fix:

a) There are about 5 extra blank lines after the new telephone_help() function.

b) Very last phrase - says that spaces will be stripped out of the link. Probably that just means that spaces will be stripped out of the URL (I'm assuming the link text would be more human-readable)?

Status:Needs work» Needs review
StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,646 pass(es).
[ View ]

- Removed the 5 extra blank lines at the end
- Changed the sentence in order to add the text word in it, this make more sense.

Any spaces will be stripped out of the link text.

Issue tags:+Needs manual testing

This looks good to me, thanks!

We need someone to give the patch a quick manual test to see if the links all work and go to where they should, and to verify that the formatting is fine. You may have to clear the cache and/or enable the Telephone module to see the help page.

Status:Needs review» Reviewed & tested by the community

I verified that all links work

Priority:Normal» Critical
Status:Reviewed & tested by the community» Fixed

Actually this should have been marked "critical", since there was no help at all for this module before.

Thanks all! Committed to 8.x.

Title:Create hook_help for telephone moduleHEAD BROKEN: Create hook_help for telephone module
Status:Fixed» Reviewed & tested by the community

This broke HEAD. Please roll back for now.

git revert c0f747d

Basically, FieldHelpTest has test coverage for "'Modules with field types that do not implement hook_help are not linked.'", and it used Telephone for that purpose.
This was added in #2099261: field_help() no longer lists field_type/widget modules and can throw undefined index notice., after the last test run for this issue's patch.

I'm not sure if that assertion is needed, or what module would satisfy it, but let's back this out first and then fix.

Title:HEAD BROKEN: Create hook_help for telephone moduleCreate hook_help for telephone module
Priority:Critical» Normal
Status:Reviewed & tested by the community» Needs work

Reverted. Wow. :) Impressed!

Priority:Normal» Critical

Oops.

OK. Sorry about that! I had no idea a hook_help() patch would break core. I'll try to remember to get an updated test run before the next commit.

Could you perhaps create a test module for purposes of that test instead of relying on core to be broken?

Assigned:Unassigned» mr.baileys

Assigned:mr.baileys» Unassigned

Honestly, I think we can remove the assertion altogether, but someone should check with mr.baileys and/or amateescu first.

Status:Needs work» Needs review
Issue tags:-Needs manual testing
StatusFileSize
new893 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Ok, removed the two offending assertions. We can re-add the tests with a test module in a separate issue.

StatusFileSize
new2.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,946 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Actually, I assume this should include the original patch from this issue since it was rolled back?

Status:Needs review» Needs work

The last submitted patch, 2078813-20-field-help.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,485 pass(es).
[ View ]

Random test failure (#2097537: Random failure in Drupal\block\Tests\Views\DisplayBlockTest->testBlockCategory()). After talking to webchick and tim.plunkett in IRC, we decided to combine this patch with the one from #2078813: Create hook_help for telephone module

Status:Needs review» Reviewed & tested by the community

This looks good mr.baileys. Thanks!

Should we maybe put a comment into field_test.module to say not to implement hook_help() because the absence of hook_help() is being specifically used in a test? I guess the test would fail if someone did that though... so probably not necessary and why would anyone do that anyway? :)

I think we're back to RTBC, since the telephone help part of the patch was reviewed previously, and the tests now pass so we probably won't break 8.x by committing this time.

Status:Reviewed & tested by the community» Fixed

Nice test fail :)

Committed/pushed #23 to 8.x, thanks!

Thanks everybody!

Status:Fixed» Closed (fixed)

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