Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ifrik’s picture

Status: Active » Needs review
FileSize
1.91 KB

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

jhodgdon’s picture

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.

ifrik’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
1.94 KB

fixed

jhodgdon’s picture

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.

batigolix’s picture

Component: documentation » telephone.module
Status: Needs work » Needs review
FileSize
2.02 KB

Patch:

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

Also changing component to get feedback from others

jhodgdon’s picture

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

JulienD’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

- 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.
jhodgdon’s picture

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.

batigolix’s picture

Status: Needs review » Reviewed & tested by the community

I verified that all links work

jhodgdon’s picture

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.

tim.plunkett’s picture

Title: Create hook_help for telephone module » HEAD 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

tim.plunkett’s picture

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.

webchick’s picture

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

Reverted. Wow. :) Impressed!

webchick’s picture

Priority: Normal » Critical

Oops.

jhodgdon’s picture

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?

mr.baileys’s picture

Assigned: Unassigned » mr.baileys
tim.plunkett’s picture

Assigned: mr.baileys » Unassigned

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

mr.baileys’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
893 bytes

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

mr.baileys’s picture

FileSize
2.89 KB

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.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
3.68 KB

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

jhodgdon’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Nice test fail :)

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

ifrik’s picture

Thanks everybody!

Status: Fixed » Closed (fixed)

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