Create helper function for help_topics_modules_installed() [#3209139]

Problem/Motivation

On #3087218-71: Help searches fail if site is not fully indexed, and users do not know why, catch requested

The code in help_topics_modules_uninstalled() could be moved to a _helper_function(), and that would save having to explain what we're doing here.

This code is located in the file core/modules/help_topics/help_topics.module.

More explanation:

On the parent issue, we implemented hook_modules_installed(), hook_themes_installed(), hook_modules_uninstalled(), and hook_themes_uninstalled(). All 4 needed to call some of the same code. The way it was implemented, we put the necessary code in hook_modules_uninstalled() and then called this function from the other 3.

What catch is asking for instead is that we make a helper function whose name starts with _help_topics_ to do the common operations, and call that instead from all four functions.

Steps to reproduce

Proposed resolution

Move the common code for these 4 hooks into a new helper function in help_topics.module whose function name starts with _help_topics_.

Remaining tasks

1. Make a patch to do the above.
2. Review the patch.
3. Commit the patch.

User interface changes

None.

API changes

Not really. Functions starting with _ are not part of the public API, and the same code will still be executed for each of these hooks.

Data model changes

None.

Release notes snippet

Not necessary.

Comment	File	Size	Author
#18	3209139-18.patch	2.96 KB	andypost
#18	9.2.x: PHP 7.3 & MySQL 5.7 28,225 pass
#18	interdiff.txt	943 bytes	andypost
#13	interdiff-12-13.txt	2.13 KB	jhodgdon
#13	3209139-13.patch	2.94 KB	jhodgdon
#13	9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
#12	interdiff_8-12.txt	855 bytes	guilhermevp
#12	3209139-12.patch	2.82 KB	guilhermevp
#12	9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
#8	3209139-8.patch	2.74 KB	guilhermevp
#8	9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
#8	interdiff_6-8.txt	1.49 KB	guilhermevp
#6	interdiff_3-6.txt	1.52 KB	guilhermevp
#6	3209139-6.patch	2.72 KB	guilhermevp
#6	9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
#3	3209139-3.patch	2.85 KB	guilhermevp
#3	9.2.x: PHP 7.3 & MySQL 5.7 Custom Commands Failed

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

16 April 2021 at 14:54

jhodgdon created an issue. See original summary.

Comment #2

guilhermevp CreditAttribution: guilhermevp at CI&T commented 16 April 2021 at 15:26

Assigned:

Unassigned

» guilhermevp

Comment #3

guilhermevp CreditAttribution: guilhermevp at CI&T commented 16 April 2021 at 15:53

Assigned:	guilhermevp	» Unassigned
Status:	Active	» Needs review

File	Size
3209139-3.patch	2.85 KB
9.2.x: PHP 7.3 & MySQL 5.7 Custom Commands Failed

Sending patch. Please review.
Made the _helper_function() as requested by Catch and updated documentation as well.

Comment #4

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 16 April 2021 at 16:01

Status:

Needs review

» Needs work

+++ b/core/modules/help_topics/help_topics.module
@@ -60,45 +60,49 @@ function help_topics_theme() {
+  //Ensure that index state is updated when a module is installed.
...
+  //Ensure that index state is updated when a module is installed.
...
+  //Ensure that index state is updated when a module is installed.
...
+  //Ensure that index state is updated when a module is installed.
...
+
+function _helper_function(array $modules) {

1) this function needs proper naming for example _help_topics_search_update()

2) needs doc-block and removal of similar inline comments

Comment #5

guilhermevp CreditAttribution: guilhermevp at CI&T commented 16 April 2021 at 16:03

Assigned:

Unassigned

» guilhermevp

Comment #6

guilhermevp CreditAttribution: guilhermevp at CI&T commented 16 April 2021 at 17:11

Assigned:	guilhermevp	» Unassigned
Status:	Needs work	» Needs review

File	Size
3209139-6.patch	2.72 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
interdiff_3-6.txt	1.52 KB

1 file was hidden/shown/deleted

File	Size
3209139-3.patch	2.85 KB
9.2.x: PHP 7.3 & MySQL 5.7 Custom Commands Failed

@andypost thanks for the feedback!

Please review the new patch.

Comment #7

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 16 April 2021 at 17:52

Status:

Needs review

» Needs work

It should get array $extensions as argument but all calling places pass empty array

Comment #8

guilhermevp CreditAttribution: guilhermevp at CI&T commented 16 April 2021 at 18:10

Status:

Needs work

» Needs review

File	Size
interdiff_6-8.txt	1.49 KB
3209139-8.patch	2.74 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass

Sending new patch.

Thanks again for the time and patience.

Comment #9

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 16 April 2021 at 18:23

Status:

Needs review

» Needs work

2 files were hidden/shown/deleted

File	Size
3209139-6.patch	2.72 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
interdiff_3-6.txt	1.52 KB

Great! Mostly done)

_helper_topics_search_update() needs properly declared doc-block according to https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

Comment #10

guilhermevp CreditAttribution: guilhermevp at CI&T commented 16 April 2021 at 18:51

Is this the correct doc block format for this situation?

/**
 * Ensure that index state is updated when a module is installed.
 *
 * @param array $extensions
 *   Array containing the names of the extensions. 
 */
function _helper_topics_search_update(array $extensions) {

Comment #11

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 16 April 2021 at 20:08

As it used for modules and themes we call it extension in core 8+

Comment #12

guilhermevp CreditAttribution: guilhermevp at CI&T commented 19 April 2021 at 11:58

Status:

Needs work

» Needs review

File	Size
3209139-12.patch	2.82 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
interdiff_8-12.txt	855 bytes

Thanks for all the help. Sending patch for review.

Comment #13

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 19 April 2021 at 14:51

File	Size
3209139-13.patch	2.94 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
interdiff-12-13.txt	2.13 KB

4 files were hidden/shown/deleted

File	Size
interdiff_6-8.txt	1.49 KB
3209139-8.patch	2.74 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
3209139-12.patch	2.82 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,214 pass
interdiff_8-12.txt	855 bytes

Thanks for the patch! I was going to review it but it was easier to make an interdiff. One big functional point: The array of extensions should only be passed in when modules are being uninstalled. Not for the other 3 cases. Anyway here is a fixed-up patch with a bunch of changes.

Comment #14

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 19 April 2021 at 15:13

Status:

Needs review

» Reviewed & tested by the community

Thanks 👍 looks ready and should be green, OTOH array could use kinda EntensionInterface[] for doc block

Comment #15

19 April 2021 at 15:49

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 13: 3209139-13.patch, failed testing. View results

Comment #16

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 19 April 2021 at 16:03

Status:

Needs work

» Reviewed & tested by the community

Test fail was in QuickEditFileTest. Looks like a random glitch.

Comment #17

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 19 April 2021 at 16:07

I believe $extensions is an array of strings, not of extensions. It's not documented what it is here:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
But if you look at where it's invoked, you can see it's a plain array of strings:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

Comment #18

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 19 April 2021 at 19:35

Status:

Reviewed & tested by the community

» Needs review

File	Size
interdiff.txt	943 bytes
3209139-18.patch	2.96 KB
9.2.x: PHP 7.3 & MySQL 5.7 28,225 pass

Sorry, I was wrong so here fix - string[] and void to polish )

Comment #19

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 19 April 2021 at 22:04

Status:

Needs review

» Reviewed & tested by the community

Changes look good to me. You had already RTBC'd the previous patch, so I think it is OK for me to RTBC your changes.

Comment #20

26 April 2021 at 14:14

catch committed fd56500 on 9.2.x

Issue #3209139 by guilhermevp, andypost, jhodgdon: Create helper...

Comment #21

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 26 April 2021 at 14:14

Status:

Reviewed & tested by the community

» Fixed

Committed fd56500 and pushed to 9.2.x. Thanks!

Comment #22

10 May 2021 at 14:14

Status:

Fixed

» Closed (fixed)

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

Create helper function for help_topics_modules_installed()

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Parent issue

Thank you to these Drupal contributors

News items

Our community

Documentation

Drupal code base

Governance of community