Problem/Motivation

The help page of this module is very important, but as far as I know, help pages are not commonly viewed.

Proposed resolution

Link the user to the help page on module install, regardless of how the module was installed - from an installation profile, through the UI, through Drush, or through the Drupal Console.

Remaining tasks

  1. On module install, link the user to the help page.

User interface changes

  • A link to the help page will be provided when the module is installed.

Comments

alexdmccabe created an issue. See original summary.

alexdmccabe’s picture

alexdmccabe’s picture

Issue tags: +Needs tests
krismaye’s picture

Assigned: Unassigned » krismaye
krismaye’s picture

StatusFileSize
new431 bytes

This will open the help page when the module is installed (as opposed to providing a link to click).

krismaye’s picture

Assigned: krismaye » Unassigned
Status: Active » Needs review
alexdmccabe’s picture

Status: Needs review » Needs work

Okay, cool, this is a really good start! I like that you actually included the docblocks in there! A lot of people skip that, in my experience.

I do see a couple of things that need tweaking, though:

  • Rather than redirect the user on install, we should probably just display a message, using the Messenger service - see the comments for some examples.
  • When linking to URLs in code, you should use routes, not the URL itself. This allows the URLs to change later, without needing to update the code in a thousand places, kind of like using a constant. The help pages are provided by the help module, so if you look at help.routing.yml, you can see the route name is help.page and the path is /admin/help/{name}, so that looks like the one we want. The tool you're looking for here is Url::fromRoute().
alexdmccabe’s picture

Keep it up though! And feel free to reach out, I'm not sure I was super clear with that last comment.

After we get the actual linking hammered out, we can look at adding tests to cover the change!

Thanks for the help!

krismaye’s picture

Assigned: Unassigned » krismaye
krismaye’s picture

StatusFileSize
new525 bytes
new549 bytes

This is a work in progress - installing the module causes the following error:
Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("name") to generate a URL for route "help.page"

alexdmccabe’s picture

First off, I'm sorry, this is a huge wall of text, but I hope it's useful to you.


Okay, so the error first: looking at the route, it says the controller/method is HelpController::helpPage.

If you open HelpController.php (in PHPStorm, you can use the Navigate menu, and then use the Class or File menu item, and then enter HelpController), you'll find HelpController::helpPage on line 114.

The docblock provides the bit we need:

@param string $name
  A module name to display a help page for.

So, in our case, the parameter we need to pass in is the name of the module, gender.

We can find out how to make that happen by reviewing the API docs for Url::fromRoute - when in doubt, start googling "drupal" plus the class and method names you're using and/or from the error. You've got the first method parameter in there, which is $route_name, but you're missing the second method parameter, $route_parameters.

The route parameters are found in the path on the route, which in this case is defined in help.routing.yml. The path is /admin/help/{name}. Sections in curly braces, like {name}, are the parameter(s). So this route has one parameter that needs to be passed in to the second argument of the method, as an array where the keys are the route parameter names, and the values are what you want to replace the parameter with.

It's important to note that the keys in the array must match the route parameter names, or it won't work.


The other thing I wanted to note is that you're creating a link and dropping it into the t() function.

Using the translation function is a bit tricky, so don't worry if you don't get it right away. I've got a couple of links here that are worth bookmarking, I use them as a reference a lot.

Dynamic strings with placeholders
Dynamic or static links and HTML in translatable strings

Honestly, as much as I don't want to just throw some links at you and say "go read this", it's a relatively complicated system and those pages do a really good job of explaining it. It's also worth nothing that these docs were written for Drupal 7, not 8, so some things like functions names will have changed.

I'm actually going to cheat a bit here, rather than making you read two lengthy docs (although they're worth a read), and just provide you with the answer for how that should look. I'm leaving it a bit up to you still to put all this together!

$messenger->addMessage(t('Please read the <a href="@url">help page</a>', ['@url' => $url]));

With that said, I will call out a couple of sections from those docs that are relevant:

From the first link:

If you pass on translatable strings with usernames in them, your translators would need to translate every single string with different usernames in them, so the number of variations for just this string could easily end up in the thousands. If you use placeholders, you reduce the number of strings to translate to one.

This is relevant for links, too. Since URLs can change, that would create new variations to be translated.

From the second link:

Our good examples keep the link markup in the text, so that translators are aware of what is happening, and the link text is also there to translate in the sentence flow. This lets them move the link around, if the language at hand requires it, move out text from the link or vice versa. It is also a best practice to use @ placeholders for URLs, since this makes sure that proper escaping happens on them. Even for internal links, if for example clean URLs are not enabled, an ampersand in the URL will surely need to be escaped. To make this format easy to remember, make sure to use url() when constructing links for translatable strings instead of l() and that would make you use HTML inside the text.

This section is why I put the link markup in the code, and why you can skip using Link::fromTextAndUrl().

It might also be worth digging through core a bit, if you look at node.module, line 114, there's an example of links to a help page, actually.


Sorry for the wall of text. I hope that helps, but if it doesn't, or if you still have questions, feel free to reach out again!

krismaye’s picture

StatusFileSize
new508 bytes
new563 bytes
krismaye’s picture

Assigned: krismaye » Unassigned
Status: Needs work » Needs review
alexdmccabe’s picture

Status: Needs review » Needs work

Looks good, just needs tests now. krismaye is working on that.

krismaye’s picture

Assigned: Unassigned » krismaye
sparklingrobots’s picture

Issue tags: +beta
krismaye’s picture

StatusFileSize
new2.75 KB
new1.94 KB
krismaye’s picture

Status: Needs work » Needs review
krismaye’s picture

StatusFileSize
new2.75 KB
new836 bytes

Fixed a few coding standard issues from the previous patch.

alexdmccabe’s picture

I think this look good!

My only comment is that this is a Functional test, since it extends the BrowserTestBase class, not the UnitTestCase class. That means it should be in the Functional directory/namespace alongside the existing tests, not in the Unit directory/namespace.

The docblock on BrowserTestBase explains:

Tests extending BrowserTestBase must exist in the Drupal\Tests\yourmodule\Functional namespace and live in the modules/yourmodule/tests/src/Functional directory.

Make that change, and I think we're good to go here!

As always, let me know if you have any questions.

alexdmccabe’s picture

Status: Needs review » Needs work
krismaye’s picture

StatusFileSize
new2.83 KB
new2.85 KB
krismaye’s picture

Status: Needs work » Needs review
krismaye’s picture

krismaye’s picture

StatusFileSize
new2.77 KB
new2.8 KB
krismaye’s picture

StatusFileSize
new2.77 KB
new340 bytes

  • alexdmccabe committed 581a825 on 8.x-1.x authored by krismaye
    Issue #2960890 by krismaye, alexdmccabe: Display message on install that...
alexdmccabe’s picture

Status: Needs review » Fixed

Done!

Thanks a lot, @krismaye!

alexdmccabe’s picture

Status: Fixed » Closed (fixed)

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