🇮🇳India @atul4drupal

looks like a oversight issue.

Fixed issue in the patch.
Trying to get this rolling...

Fixed issue and added test coverage for both block user and unblock user action.
Kindly review and suggest.

Some minor errors to fix.

Updated patch by modifying the description.

Thanks @rkoller for sharing the gist's from the discussion in Usability Meeting.

"consistency is a good thing", #24 I expressed my alignment with this (hopefully I am getting good at UX nuances :))
We may make the descriptions and labels consistent as suggested in #31.

For the field label use:
Description

For the field description use:
Display on [the place where the description will be shown]

Thanks @ smustgrave → for reviewing the patch. I have added updated patch to cover testing the logged message.

Earlier patch missed on this and for this reason it was passing irrespective of any change, hope this will address the scenario.
Do review and suggest.

Thanks.

@ _utsavsharma → Thanks for fixing the issue, though its always better to leave a suggestion for the original contributor to come and fix the issue, specially considering the recent activity on the thread.
As otherwise it appears deliberate attempt to trick the credit system by hijacking the issue by doing trivial changes.

I don't doubt your intent, however we should equally be respectful of others effort and time.

Thanks.

Adding a patch to add test for the BlockUser.php file.

One suggestion that I think of to frame the error message considering all the constraints and concerns raised in the thread is :

"If {account ID} is a valid and active account, an email will be sent with instructions to reset your password."

account ID will be the value entered by user in "username or Email" field.

Kindly share thoughts on using this... or some thing on this line.

I here do not agree with the direction this thread has taken from #7 📌 Make Description Field Labels Consistent Needs work .

I understand and support the IS as it was originally created.

The issue → refered at #7 has targeted the multi-line nature of description field and as obvious going through that thread it clearly seems that they had discussed about reducing length of description field to change it to textfield supporting single line description, however the change in label was taken as granted without any discussion/consideration. This thread wanted to rightly streamline the labelling for description field.

10 years ago this got missed and I agree with Chris Matthews → on this that the field label should be consistent, unless we have a strong case deviating from it.

Reasons to have consistency:
1) Go To
1.1) "admin/structure/types/add"
Notice description field label and the help text shown below the field.

1.2) "admin/structure/taxonomy/add"
Notice the description field label.

1.3) "admin/structure/menu/add"
Notice the description field label it simply stands out as "Administrative summary" why this in consistency.

If we were to indicate that this description will appear only at administrative page then for that clarification we have the provision to add
help text for the field, why change the field label altogether that too inconsistently.

2) Any mature application/software professes consistency and standardisation and I believe that this issue rightly progresses towards that.

Likewise at "block/add" page for description field why to have "Block Description" label when its so obvious, you are already at "Add content block" page just having "description" label is enough, for further clarification we may always add the help text.
On the contrary when you are at "admin/structure/block-content/add" the label for description field is just "description".

At "admin/structure/comment/types/add" also we have description label.

I think having consistent label is good and any meta about the field if needed can be provided as help text.

Updated the text per changes in block naming.

atul4drupal → made their first commit to this issue’s fork.

Have reverted changes in "core/lib/Drupal/Core/Validation/ConstraintManager.php" file to restore 'type' key per #18 📌 Clean up email validation constraint definition Needs work .
Of the 3 keys to set constraint definition label seemed optional, but left it as is for consistency and ease of understanding.

Kindly review and suggest.

Added patch for 11.x
Interdiff is not generated as the patch at #49 is not applying to have the interdiff generated (interdiff: Error applying patch1 to reconstructed file)

IS to be updated...

Hope IS is proper and explanatory.

Updated patch to fix the error.
Addressed #61 ✨ Ability to Customize Core Error Message Needs work

Kindly review and suggest.
Thanks.

re-rolled patch for 11.x.

Thanks @smustgrave this was what I thought initially, but isnt the cspell should also respect the corectly spelled words... in this case it was "redirections", just wondering.

updated patch to fix cspell issue for 11.x.

Bumped version to latest.
Added patch with tests.

Kindly review and suggest.

The changes look ok go go ahead with.
The MR pipeline was giving error most likely because it needed to be rebased, rebased and now it all looks ok (didnt realize that rebasing will add a comment in this thread too as a new MR)
RTBC +1 from my side, cant do it myself as I got 1 MR added.

atul4drupal → made their first commit to this issue’s fork.

1) core/modules/rest/tests/src/Functional/ResourceTestBase.php

protected static function recursiveKSort(array &$array) {
    // First, sort the main array.
    ksort($array);

    // Then check for child arrays.
    foreach ($array as $key => &$value) {
      if (is_array($value)) {
        static::recursiveKSort($value);
      }
    }

    return $array;
  }

}

This function still returns an array and I think it will not be appropriate to remove the @return comment for this function until the function is updated to not return anything (As this function is updating a variable passed by reference this may be updated to not return anything in a separate issue and comment may be updated accordingly too).

apologies for the noise... kindly ignore patch at #26 🐛 PrivateTempStore::set() will fail if $key is too long Needs review .

Changes introduced:
re-provisioned for the loss of characters owing to 'ascii_general_ci' collation, this error got introduced in patch at 26 so kindly ignore that.
Concern raised in #25 has been addressed.

Kindly review and suggest.

Thanks Liam Morland → for pointing out the scenario, I have update the patch to take care of the ID value (retured from getOwner) that is prefixed to key.

Kindly review and suggest.

Updated the patch to limit the code changes to "createKey" method only.
Realised this while updating the IS.

kindly review and suggest.

Thanks for clearing the air, adding updated patch:
1) Updated logic to preserve original key as much possible with hashed suffix.
2) Updated tests.

Kindly review and suggest.

#16 🐛 PrivateTempStore::set() will fail if $key is too long Needs review .2 - DONE

#16 🐛 PrivateTempStore::set() will fail if $key is too long Needs review .1 -
Here I get the point that we would like to preserve few initial words from the original key... Do not see any harm in doing/implementing that but at the same time I also do not see any value addition in doing that either (kindly help me in case I am missing some thing).

[just thinking aloud here] As in any case the user adding such long tempstore key will generally keep using the same key that is hashed and will always get the desired values. If we attempt to preserve some part from the original key how will that help or what value addition we intend to achieve here!

adding the updated patch:
1) Included hashing of key.
2) Updated test case for set and get methods.

Kindly review and suggest.

forgot top update the issue status.

Adding the updated patch:

1) Addition of constant for string length limit as suggested in #9.
2) Increased length limit to 120 characters as suggested in #9 (Though this limit may be debated and increased or decreased as per consensus arrived at).
3) Added PhpUnit test.

Kindly review and suggest.

atul4drupal → created an issue.

Adding patch to update changes as suggested in #191.

Still have a few open TODO items on the issue:

1) Decide on an approach to address concern raised in #17 (possibly without adding new token type). If new token type needed then this need to be discussed and agreed upon before moving forward with this approach.

2) IS to be updated
(leaving this for someone who has more hold on this issue thread, will revisit to update IS if not updated in some time).

I tried to read through the thread to understand it, and as far I understood for #191 ✨ Add a token for the site logo Needs work point 2:
@Berdir I believe is responding to the proposed suggestion for having seperate token type to which he disaggred, and that is something still not addressed as we see a new token type "site-logo" being introduced in the code.

Also @Berdir expressed for someone to verify if suggestions from #161 are implemented which I verifed have been incorporated and this was also verified in #180.

#191 point 1 (refering #17) this I think will relate to what approach we take with respect to the concern raised around introducing new token type, as in the current solution if we take off the new token type, #17 remain open.

It is a situation in hand, will spend some time on it however some direction around how to address #17 will be helpful, keeping in mind that people in general are not inclined towards introducing a new token type.