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/history
Comment | File | Size | Author |
---|---|---|---|
#60 | create-help-text-history-2029767-60.patch | 1.89 KB | batigolix |
#60 | interdiff.txt | 1.75 KB | batigolix |
#58 | interdiff.txt | 1.9 KB | batigolix |
#58 | create-help-text-history-2029767-58.patch | 1.89 KB | batigolix |
#54 | create-help-text-history-2029767-54.patch | 2.04 KB | batigolix |
Comments
Comment #1
batigolixI cannot see where I can see this module in action. The module tracks node visits by users, but it is not clear if there is an UI.
See (#1120928: Move "history" table into separate History module)
Comment #2
catchComment #3
Marc DeLay CreditAttribution: Marc DeLay commentedStarting on a patch for the hook_help function in entity.module#midwest drupal summit
eek, wrong issue.
Comment #4
kscheirerI'll give it a shot
Comment #5
kscheirerComment #6
kscheirerComment #7
jhodgdonThis help needs to follow our standards (http://drupal.org/node/632280).
Comment #8
batigolixRewritten to be more in line with the standards.
I set the component to history.module so that the maintainers can have a look at it as well
Comment #9
jhodgdonThis help reads well and is clearly written. I'll let the maintainers of the module comment on its accuracy (for instance, is the "one month" expiration a setting or hard-wired?).
I noticed two small things that should, meanwhile, be fixed:
a) "The module does not provide a user interface For more information,..."
==> missing a . at end of sentence.
b) The link to the online docs is not following the standard at https://drupal.org/node/632280
Comment #10
batigolixPatch fixes a) and b) from #9
Still pending: check for accuracy (see #9)
Comment #12
batigolixseems i attached the wrong patch :(
new attempt
Comment #13
tlyngej CreditAttribution: tlyngej commentedLooks good to me.
Comment #14
webchickGreat work!
Committed and pushed to 8.x. Thanks!
Comment #15
markconroy CreditAttribution: markconroy commentedAdded link to views help page and edited link to history documentation page.
Comment #16
markconroy CreditAttribution: markconroy commentedadded link to views help page and also edited link to history documentation page.
Comment #18
markconroy CreditAttribution: markconroy commentedFixed links (comma missing in array list) and some typos as well as wording to online documentation
Comment #20
ifrik#18: create-help-text-history-2029767-18.patch queued for re-testing.
Comment #22
ifrikI've applied the patch in #18 - and it applied fine even though the test failed.
The wording appears accurate and according to the Help standard text. Since there is no user interface for this module, it does not require a "Uses" section.
The links work correctly, and they follow the new url() standard.
The link to the history module help page is listed on the general help page.
Comment #23
markconroy CreditAttribution: markconroy commentedJust realised (thanks ifrik) that I was patching a patch that had been committed to core.
Patch here is to core.
Comment #24
markconroy CreditAttribution: markconroy commentedchanging issue status
Comment #25
ifrikI applied the patch and it works fine.
The wording is according to Standards, and appears to be correct.
The link to the Views help doesn't work yet, because that Views hook_help is still missing. But it goes to the right location.
The links are in the correct format.
Tests is still running, but if that passes then in can be marked as reviewed and tested
Comment #26
ifrikComment #27
webchickAwesome work!
Committed and pushed to 8.x. Thanks!
Comment #28
markconroy CreditAttribution: markconroy commentedSuper.
Thanks ifrik for you patience and webchick for committing - my first every commit/patch (I'll get moving on more now).
Comment #29
jhodgdonI'm assuming that was an inadvertent status change.
Comment #30
jhodgdonThere is one minor typo -- very minor -- an extra space before the last sentence. A quick follow-up would be good? If we don't fix it now we will never be able to (it's a translated string and those get frozen).
Comment #31
markconroy CreditAttribution: markconroy commentedI'm not sure exactly what extra space there was. I removed the one at the very end of the last sentence not before the last sentence.
Comment #32
markconroy CreditAttribution: markconroy commentedNot sure which space I was to remove. I removed the one at the end of the last sentence, not the one before it.
Comment #33
markconroy CreditAttribution: markconroy commentedDuplicate, sorry.
Not sure which space I was to remove. I removed the one at the end of the last sentence, not the one before it.Comment #35
markconroy CreditAttribution: markconroy commentededited, re-submitting.
Comment #37
markconroy CreditAttribution: markconroy commentedCan someone review that patch manually? It's working fine on my local machine.
Thanks.
Comment #38
jhodgdonYou probably need to do a git pull to update your local repository. The patch does not apply for me either.
And by the way, the space that needs to be removed is here:
(between the . and For, there are two spaces instead of one)
Comment #39
markconroy CreditAttribution: markconroy commentedI'll try again!
Comment #40
jhodgdonActually, I just noticed there are a couple of places in that line with two spaces in a row. The other one I just noticed was:
Could you use your text editor to search for two spaces in a row in that text and remove all of them? Thanks!
Also, at this point it is not a critical issue, so devaluing the priority.
Comment #41
markconroy CreditAttribution: markconroy commentedShould be good now.
Comment #42
batigolixThink I annihilated the surplus spaces
Comment #43
jhodgdonWait. This patch is supposed to just be removing extra spaces within the hook_help() in the history module. But the patch in #42 has all kinds of other stuff in it.
Comment #44
ifrik@Mark Conroy:
The patch in #42 still has one last double space in it: Just before the "For more information..."
And if you are on it anyway: could you start the link to the online documentation earlier to include the whole phrase "online documentation for the History module" ?
@batigolix: There is indeed lots of other stuff in it, and also: the permissions got changed. That seems to happen more often - maybe you can change the setup? Or make interdiffs so that it's easier to spot?
Comment #45
batigolixSorry for that. This new patch *only* removes the double whitespace
Comment #46
ifrikThe link to the documentation page works, the link to views follows the default path pattern but currently breaks because the views module help page is missing.
I can't see any double spaces and the permissions are correct.
Comment #47
jhodgdonThanks! Moving back to documentation so that I'll see it when I'm next doing commits.
Comment #48
jhodgdonOh... Can we standardize the link to the on-line documentation so it matches our template? More should be inside the link text.
Comment #49
batigolixPatch standardizes link text to docs
Comment #50
jhodgdonOne more link accessibility issue. Can we move the link to Views to the first mention, and make sure that the link text is "Views module" not just "views"?
Comment #51
ifrikI've changed the reference to Views module as proposed in #50
Comment #52
jhodgdonThis looks pretty good.
I noticed one more oddity. The module (as stated) as no UI itself. However, it says in the About that it "marks" content as New or Updated. Maybe terminology like "keeps track of" would be clearer -- to me "mark" sounds visible?
Actually, that whole sentence saying "by choosing Plain text as Format on the Manage Display page." is kind of redundant with the first sentence of About, isn't it? ... Oh, I see, I think it is describing the field and filter available to the Views module. Why not say it provides a field called xyz that shows ... and a filter called abc that does ... then?
And maybe we can move the last sentence about not providing a user interface to the beginning, such as "The module does not provide its own user interface, but instead makes its data available through the Views module"?
Comment #53
jhodgdonComment #54
batigolixAttached patch addresses the points raised in #52
I could not find anything regarding
Was that a mistake, or am I missing something?
Comment #55
jhodgdonSorry, yes, that was a copy/paste error on my part.
Here's what I actually meant to say. The help text currently says (it's all in one paragraph but I've broken it up into numbered sentences so I can refer to them:
So... This is very poor writing -- it's choppy (sentences are overly short) and quite redundant Sentences #2/#7 say the same thing. #1 is basically repeated in #3, and then kind of repeated again in #5, and then #6 repeats what #5 says mostly.
So maybe we could combine #1/3/5/6 into one sentence (or maybe two if entirely necessary) at the start, and then combine #2/4/7 into another sentence, and then keep #8 at the end?
Comment #56
catch... so I don't keep wondering why this isn't critical.
Comment #57
jhodgdonGood idea. :)
Comment #58
batigolixPatch re-orders phrases as suggested in #55
Comment #59
jhodgdonMuch better! Now all we need is for the link to the online docs to conform to our standard (put "online documentation ..." all into the link text).
Comment #60
batigolixComment #61
batigolixComment #62
jhodgdonGreat! I think this one is ready for a quick 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.
And since we didn't change the substance of the text here (just copy editing), I don't think we necessarily need to pass this into component history.module.
Comment #63
lostkangaroo CreditAttribution: lostkangaroo commentedManual test compete. All links function as expected and everything else appears to be in order.
Comment #64
jhodgdonThanks!
We're in the middle of a "major/critical commits only" week in Drupal Core, so this cannot be committed until Jan 23rd. I'll get to it then.
Comment #65
jhodgdonSigh. Now there is an issue tagged "avoid commit conflicts" which has a patch that touches history.module. Even though it probably does not have a conflict with this patch for people using a normal Git workflow, I'm being extra-careful lately about commit conflicts and will wait to commit this until
#1996238: Replace hook_library_info() by *.libraries.yml file
is finished.
Comment #66
alexpott#1996238: Replace hook_library_info() by *.libraries.yml file is postponed atm so let's get this done. Committed 7833be2 and pushed to 8.x. Thanks!