Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#23 | 2078813-22-field-help.patch | 3.68 KB | mr.baileys |
#20 | 2078813-20-field-help.patch | 2.89 KB | mr.baileys |
#19 | 2078813-18-field-help.patch | 893 bytes | mr.baileys |
#8 | telephone-help-text-2078813-7.patch | 2.01 KB | JulienD |
#6 | telephone-help-text-2078813-6.patch | 2.02 KB | batigolix |
Comments
Comment #1
ifrikI'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 improperly formatted 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.
Comment #2
jhodgdonThis 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.
Comment #3
ifrikfixed
Comment #4
jhodgdonSee #2028799-25: Improve help for link module comments 25-28 ... probably applies here as well?
Comment #6
batigolixPatch:
- fixes comment #4
- changes url() top \Drupal::url()
- changes @tokens to !tokens
Also changing component to get feedback from others
Comment #7
jhodgdonThis 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)?
Comment #8
JulienD CreditAttribution: JulienD commented- 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.
Comment #9
jhodgdonThis 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.
Comment #10
batigolixI verified that all links work
Comment #11
jhodgdonActually this should have been marked "critical", since there was no help at all for this module before.
Thanks all! Committed to 8.x.
Comment #12
tim.plunkettThis broke HEAD. Please roll back for now.
git revert c0f747d
Comment #13
tim.plunkettBasically, 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.
Comment #14
webchickReverted. Wow. :) Impressed!
Comment #15
webchickOops.
Comment #16
jhodgdonOK. 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?
Comment #17
mr.baileysComment #18
tim.plunkettHonestly, I think we can remove the assertion altogether, but someone should check with mr.baileys and/or amateescu first.
Comment #19
mr.baileysOk, removed the two offending assertions. We can re-add the tests with a test module in a separate issue.
Comment #20
mr.baileysActually, I assume this should include the original patch from this issue since it was rolled back?
Comment #23
mr.baileysRandom 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
Comment #24
jhodgdonThis 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.
Comment #25
catchNice test fail :)
Committed/pushed #23 to 8.x, thanks!
Comment #26
ifrikThanks everybody!