Fix documentation on hook_user_format_name_alter()

Working on this one in DrupalCon sprint

Applied patch core/modules/user/user.api.php cleanly

Hi,

The issue's patch in #4 applies to the latest D8 cleanly.
It's just a docks change so there are no functional regressions in it.
It confirms to the requirements in the task and D8 coding standards.

Question:
Is this documentation task only related to the hook_... documentation declaration in the .api file or it should update all the places where it is be-ing used or referenced, for example:
- user_hooks_test_user_format_name_alter
- AccountInterface::getDisplayName

BR,
Nikolay Dobromirov.

Hi, I think we only got to do the .api file, the doc on AccountInterface::getAccountName looks OK.
Would it be better to have the exact same description on AccountInterface::getAccountName?

thanks

Thanks @xxronis for working on this. Just some small nitpicks :)

1. +++ b/core/modules/user/user.api.php
   @@ -111,11 +111,14 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *   Can be auto-escaped if it is a string, and if it has trusted HTML content,
   

   The other case (trusted HTML content) can be separated from the former case with punctuation full stop. Also consider it putting it in next line.

2. +++ b/core/modules/user/user.api.php
   @@ -111,11 +111,14 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *   a SafeString object needs to be used.
   

   s/needs to be/should be

3. Coding standard - Parameter tags must be grouped together in a doc comment. See https://www.drupal.org/coding-standards/docs#param.

I see another issue which says Remove hook_user_format_name_alter() https://www.drupal.org/node/2575213. So does it make sense to put effort here?

Interesting. I am posting in #2575213: Deprecate hook_user_format_name_alter() regarding this issue.

Also changing to Drupal\Component\Render\MarkupInterface following commit #708ce0a

At this point, this should be a documentation-only patch for 8.0.x, and #2575213: Deprecate hook_user_format_name_alter() should be considered for 8.1.x since it's an API change.

Issue still relevant and needs a reroll.

I've uploaded the re rolled patch

Welcome AbhishekLal! Thanks for rerolling the patch.

The patch looks good, but there are some extra lines in it.

+++ b/core/modules/user/user.api.php
@@ -109,11 +114,19 @@ function hook_user_cancel_methods_alter(&$methods) {
+<<<<<<< 742d68957f421ffd2479139306dfb54fc0ff39a7
...
+=======
...
+>>>>>>> Applying patch

These need to be removed.

And, if you haven't already found it there is a handbook page for rerolling patches. Heaps of useful info in those handbook pages.

Adding a new patch.

Changed:
-The string that $account->getDisplayName() will return.
-Implementations are allowed to change it to either.

+Either a string that $account->getDisplayName() will return or a MarkupInterface object.

@gianani,

Thanks for the patch. It is getting applied properly.

Update #25 patch.I hope its helpful.
Please Review .

Thanks all, almost there.

1. +++ b/core/modules/user/user.api.php
   @@ -109,11 +109,14 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *   Either a string that $account->getDisplayName() will return or a
   + *   MarkupInterface object.
   + *   Can be auto-escaped if it is a string.
   + *   If $name has trusted HTML content, a MarkupInterface object should be used.
   

   The coding standards state that "Sentences should be separated by single spaces." So, this needs to be changed to replace the new lines with spaces and to adjust so it is still within 80 characters length.

2. +++ b/core/modules/user/user.api.php
   @@ -109,11 +109,14 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *   The account object passed to user_format_name().
   

   Not sure why this is added. user_format_name isn't used in this method.

@AbhishekLal, still a bit more. When making a patch please iinclude an interdiff, they do make reviews easier.

1. +++ b/core/modules/user/user.api.php
   @@ -1,5 +1,6 @@
   +use Drupal\Core\Entity\EntityInterface;
   +use Drupal\Component\Render\MarkupInterface;
   

   Why are these lines added to the patch?

2. +++ b/core/modules/user/user.api.php
   @@ -109,11 +110,14 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *   Implementations are allowed to change it to either. Can be auto-escaped if it is a string. If $name has trusted HTML content, a SafeString object should be used.
   

   Good, the newlines have been replaced. Just need this to be adjusted to that it doesn't exceed the 80 character column limit. What I meant to say was to move the new lines so that the sentences are separated by a single space and each line is no more than 80 characters long.

Thanks for the interdiff.

1. +++ b/core/modules/user/user.api.php
   @@ -109,11 +114,15 @@ function hook_user_cancel_methods_alter(&$methods) {
     *
   

   Delete this blank line, there are no blank lines betwee @param tag lines.

2. +++ b/core/modules/user/user.api.php
   @@ -109,11 +114,15 @@ function hook_user_cancel_methods_alter(&$methods) {
     * @param $account
     *   The account object the name belongs to.
   

   Delete these two lines. $account is declared below.

3. +++ b/core/modules/user/user.api.php
   @@ -109,11 +114,15 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *   Implementations are allowed to change it to either. Can be auto-escaped if it is a
   + * string. If $name has trusted HTML content, a SafeString object should be used.
   

   These two lines are part of the description of $name. They need to move after that and be adjusted to be < 80 characters per line and indented the same.

4. The Issue Summary states that "The $account argument should be type hinted as AccountInterface" This needs to be added as well.

@AbhishekLal, I don't know if you are using an IDE to edit these or not. But it will be worth exploring the settings to get a visual indicator for where column 80 is.

@hardik.p, thx for working on this.

+++ b/core/modules/user/user.api.php
@@ -109,11 +109,13 @@ function hook_user_cancel_methods_alter(&$methods) {
  *   The string that $account->getDisplayName() will return.
...
+ *   Implementations are allowed to change it to either. Can be auto-escaped if
+ *   it is a string. If $name has trusted HTML content, a SafeString object
+ *   should be used. ¶

This correctly makes a paragraph. But the new line after 'return.' should be replaced with a space and then followed by the sentence starting with 'Implementations. And then each line adjusted for the 80 character limit. I hope that makes sense. If not, just ask.

+++ b/core/modules/user/user.api.php
@@ -109,11 +109,13 @@ function hook_user_cancel_methods_alter(&$methods) {
+ *   should be used. ¶

Extra space at end of line

Hey, thanks for continuing to work on this. Still some indentation issues.

If you haven't already seen them the handbook has lots of information on the coding standards and there is a section just for the @param: Function parameters.

1. +++ b/core/modules/user/user.api.php
   @@ -109,11 +114,12 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *   The string that $account->getDisplayName() will return. Implementations are ¶
   

   Delete trailing space at end of line.

2. +++ b/core/modules/user/user.api.php
   @@ -109,11 +114,12 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *allowed to change it to either. Can be auto-escaped if it is a string. If $name ¶
   

   Indent 'allowed' 2 spaces and delete space at end of line. Note that the line will be > 80 characters after the change.

3. +++ b/core/modules/user/user.api.php
   @@ -109,11 +114,12 @@ function hook_user_cancel_methods_alter(&$methods) {
   + *has trusted HTML content, a SafeString object should be used.
   

   Indentation incorrect.

Awesome, indented and within 80 characters. Without that distracting me I can read the text and see it does not flow well. Should have done that earlier, sorry folks. (I'm working on improving my review skills). But one more change.

+++ b/core/modules/user/user.api.php
@@ -109,11 +109,12 @@ function hook_user_cancel_methods_alter(&$methods) {
+ *   The string that $account->getDisplayName() will return. Implementations are
+ *   allowed to change it to either. Can be auto-escaped if it is a string. If

'Implementation are allowed to change it to either' doesn't sound right. To either what? How about:

The value that @account->getDisplayName will return. If it is a string it will be auto-escaped. If it is a MarkupInterface object it is HTML escaped. Either is safe to be printed within HTML fragments.

Last patch seem ok to be commit

+++ b/core/modules/user/user.api.php
@@ -109,11 +109,12 @@ function hook_user_cancel_methods_alter(&$methods) {
+ * @param string|Drupal\Component\Render\MarkupInterface $name
+ *   The value that $account->getDisplayName will return. If it is a string it
+ *   will be auto-escaped. If it is a MarkupInterface object it is HTML
+ *   escaped. Either is safe to be printed within HTML fragments.

Hmmm... so this parameter is dual purpose. I think we should say that this is the value being altered. Or well take a completely different approach and don't mention the implementation so explicitly in the documentation. Also the documentation about escaping is not clear. If the method changes the output to be a MarkupInterface object it is the responsibility of the implementation to ensure that it is safe. If something is a MarkupInterface object it will not be auto-escaped by Twig so this is important. So something like:

The username that is displayed for a user. If a hook implementation changes this to an object implementing MarkupInterface it is the responsibility of the implementation to ensure the user's name is escaped properly. String values will be autoescaped.

Also we can an add @see sanitization

Patch to apply the suggested changes on #46.

Ups! I forget attaching the patch.

+++ b/core/modules/user/user.api.php
@@ -109,13 +109,19 @@ function hook_user_cancel_methods_alter(&$methods) {
+ *   The username that is displayed for a user. If a hook implementation changes
+ *   this to an object implementing MarkupInterface it is the responsibility of
+ *   the implementation to ensure the user's name is escaped properly. String
+ *   values will be autoescaped.
+ *   The value that $account->getDisplayName will return. If it is a string it
+ *   will be auto-escaped. If it is a MarkupInterface object it is HTML
+ *   escaped. Either is safe to be printed within HTML fragments.

Something seems duplicated here.

ok, reviewing

Great, the issues in #46 are fixed.

Committed and pushed 63cad2c to 8.3.x and a3b15c9 to 8.2.x. Thanks!