Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

lauriii’s picture

Status: Active » Needs review
FileSize
1.55 KB

Hmm, somehow we would need to add the space before the link

dawehner’s picture

dawehner’s picture

Status: Needs review » Needs work
dawehner’s picture

Status: Needs work » Postponed

Let's be honest for now.

alexpott’s picture

Priority: Major » Critical

This is part of the critical meta.

dawehner’s picture

Status: Postponed » Needs review
FileSize
8.01 KB
1001 bytes

On top of the other issue I would go with

lauriii’s picture

Status: Needs review » Needs work

I guess there is some unrelated changes in the latest patch

dawehner’s picture

iMiksu’s picture

Assigned: Unassigned » iMiksu

I can create patch with #3 + #8 interdiff

iMiksu’s picture

Assigned: iMiksu » Unassigned

Can't do it. The first #3 patch conflicts with the #8 interdiff. Whats needs to done here actually?

iMiksu’s picture

Assigned: Unassigned » iMiksu

Talked in extended sprints, I'll re-roll this.

iMiksu’s picture

Assigned: iMiksu » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
899 bytes
1.54 KB
31.45 KB

During manual testing I spotted that no space was between the description and the link itself:

So I attached a new re-rolled patch using #3 as base and the #8 placeholder change.

joelpittet’s picture

Status: Needs review » Needs work

We don't need the array anymore I'm quite positive

iMiksu’s picture

Status: Needs work » Needs review
FileSize
1001 bytes

Re-rolled as #8 interdiff, simple as that :)

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This looks like the correct approach to the problem and also simplest.

Thank you

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I agree this is the simplest thing, the output of Drupal::l() is already marked safe... however...

+++ b/core/modules/user/src/AccountForm.php
@@ -144,11 +144,11 @@ public function form(array $form, FormStateInterface $form_state) {
         $request_new = $this->l($this->t('Reset your password'), new Url('user.pass',
           array(), array('attributes' => array('title' => $this->t('Send password reset instructions via e-mail.'))))
         );
-        $current_pass_description = $this->t('Required if you want to change the %mail or %pass below. !request_new.',
+        $current_pass_description = $this->t('Required if you want to change the %mail or %pass below. @request_new.',

Why are these string separate? we could join them up like this...

        $current_pass_description = $this->t('Required if you want to change the %mail or %pass below. <a href="!request_new_url">Send password reset instructions via e-mail.</a>.',
          array(
            '%mail' => $protected_values['mail'],
            '%pass' => $protected_values['pass'],
            '!request_new_url' => $this->url('user.pass'),
          )
        );

Yes we don't get rid of the ! here but this feels the correct change wrt to translator context.

There is another usage of the string Send password reset instructions via e-mail. but it is not a link which could be important for context.

    $items['request_password'] = \Drupal::l($this->t('Reset your password'), new Url('user.pass', array(), array(
      'attributes' => array(
        'title' => $this->t('Send password reset instructions via e-mail.'),
        'class' => array('request-password-link'),
      ),
    )));
iMiksu’s picture

Assigned: Unassigned » iMiksu
iMiksu’s picture

Assigned: iMiksu » Unassigned
Status: Needs work » Needs review
FileSize
2.34 KB
2.4 KB

EDIT: So I decided to add comments around the string you mentioned (cross reference).

lauriii’s picture

+++ b/core/modules/user/src/AccountForm.php
@@ -141,19 +141,15 @@ public function form(array $form, FormStateInterface $form_state) {
+        // in user login block, see
+        // \Drupal\user\Plugin\Block\UserLoginBlock::build.

+++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
@@ -112,6 +112,8 @@ public function build() {
+        // element, see \Drupal\user\AccountForm::form.

We could use @see here, @see documentation: https://www.drupal.org/coding-standards/docs#see

iMiksu’s picture

@see that ;)

iMiksu’s picture

We can actually forget this change and make end result to be a string.

- $current_pass_description = array();
+ $current_pass_description = '';
xjm’s picture

Title: Replace remaining !placeholder for Non-URL HTML outputs only in AccountForm » Change translatable string with !placeholder AccountForm to use a URL instead of a generated link and provide better context
  1. +++ b/core/modules/user/src/AccountForm.php
    @@ -134,7 +134,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -      $current_pass_description = array();
    +      $current_pass_description = '';
    

    Good catch!

    This made me wonder why the variable is being initialized there at all. Looking at the whole thing:

          $current_pass_description = '';
    
          // The user may only change their own password without their current
          // password if they logged in via a one-time login link.
          if (!$form_state->get('user_pass_reset')) {
            $protected_values['mail'] = $form['account']['mail']['#title'];
            $protected_values['pass'] = $this->t('Password');
            // Note that "Send password reset instructions via e-mail." text is used
            // in user login block.
            // @see \Drupal\user\Plugin\Block\UserLoginBlock::build()
            $current_pass_description = $this->t('Required if you want to change the %mail or %pass below. <a href="!request_new_url">Send password reset instructions via e-mail</a>.',
              array(
                '%mail' => $protected_values['mail'],
                '%pass' => $protected_values['pass'],
                '!request_new_url' => $this->url('user.pass'),
              )
            );
          }
    
          // The user must enter their current password to change to a new one.
          if ($user->id() == $account->id()) {
            $form['account']['current_pass'] = array(
              '#type' => 'password',
              '#title' => $this->t('Current password'),
              '#size' => 25,
              '#access' => !empty($protected_values),
              '#description' => $current_pass_description,
              '#weight' => -5,
              // Do not let web browsers remember this password, since we are
              // trying to confirm that the person submitting the form actually
              // knows the current one.
              '#attributes' => array('autocomplete' => 'off'),
            );
    

    I wonder why we don't just add $form['account']['current_pass'][#description] conditionally after that hunk? Because otherwise we're adding an empty #description some of the time which just seems weird.

  2. +++ b/core/modules/user/src/AccountForm.php
    @@ -141,14 +141,14 @@ public function form(array $form, FormStateInterface $form_state) {
    +        // Note that "Send password reset instructions via e-mail." text is used
    +        // in user login block.
    +        // @see \Drupal\user\Plugin\Block\UserLoginBlock::build()
    
    +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
    @@ -112,6 +112,9 @@ public function build() {
    +        // This translatable text is used also in account form's password
    +        // element.
    +        // @see \Drupal\user\AccountForm::form()
    

    So in general, it's a great idea to add documentation cross-referencing bits of the codebase when something is not immediately obvious, especially if we have to do some research or deliberate a bit to make the decision.

    In this particular case though I don't think it's necessary. Discussed with @alexpott and we'd both suggest removing these two comments. In general, the translator will not see these comments anyway, but if one is curious as to why there are two similar-but-different-strings, one could fairly easily search the codebase since it is just a string literal.

    Great instinct though; it's definitely better to err on the side of providing this kind of information when we make a change generally.

xjm’s picture

Status: Needs review » Needs work

For #24.

iMiksu’s picture

Assigned: Unassigned » iMiksu

Thanks! Will work further.

iMiksu’s picture

Assigned: iMiksu » Unassigned
Status: Needs work » Needs review
FileSize
2.79 KB
3.57 KB
2.79 KB
  • Removed comments
  • Moved the description declaration into its own code block
  • Stopped using $protected_values

So, while doing this I was trying to figure out what #access is actually does, and I couldn't find the reasoning behind that. It was added in #1499596: Introduce a basic entity form controller. However, if there's something we need to change related to that, we should create separate issue specifically for that as I don't see this issue part of this issue's scope.

EDIT: Whoops, accidentally double posted patch. Sry :)

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/user/src/AccountForm.php
@@ -133,42 +133,32 @@ public function form(array $form, FormStateInterface $form_state) {
+          $form['account']['current_pass']['#description'] = $this->t('Required if you want to change the %mail or %pass below. <a href="!request_new_url">Send password reset instructions via e-mail</a>.',
...
+              '!request_new_url' => $this->url('user.pass'),

! has to be converted to : after its been added to Drupal core. Otherwise this looks good.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/src/AccountForm.php
@@ -133,42 +133,32 @@ public function form(array $form, FormStateInterface $form_state) {
-          array(), array('attributes' => array('title' => $this->t('Send password reset instructions via e-mail.'))))
...
+          $form['account']['current_pass']['#description'] = $this->t('Required if you want to change the %mail or %pass below. <a href="!request_new_url">Send password reset instructions via e-mail</a>.',

The texts should be actually "Reset your password" and should has a title attribute "Send password reset instructions via e-mail."

iMiksu’s picture

Assigned: Unassigned » iMiksu

OK, the text was changed as alex suggested, but I agree that the shorter link with the original title is better.

alexpott’s picture

@iMiksu I made a mistake - sorry.

iMiksu’s picture

@alexpott no probs, hopefully this will get things further now. Fingrers crossed!

iMiksu’s picture

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

For #32

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/src/AccountForm.php
@@ -133,42 +133,32 @@ public function form(array $form, FormStateInterface $form_state) {
           '#weight' => -5,
...
+          $form['account']['current_pass']['#description'] = $this->t('Required if you want to change the %mail or %pass below. @request_new_url.', array(

Oh this means we're going back to just add translatable strings together. Let's not do that. We can just put the title attribute into this string too.

iMiksu’s picture

Status: Needs work » Needs review
FileSize
2.74 KB
1.84 KB

Thanks for feedback, here's new patch. As we discussed with @alexpott, I've also removed second statement in #access as it can't be FALSE. !$form_state->get('user_pass_reset') should work there just fine.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch all the old string bits are intact and in the new and better context for the translated string.

Thanks you @iMiksu for finishing this up.

We replace the !url with :url placeholders in another issue once that placeholder exists.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: 2569281-35.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Weird this didn't fail...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: 2569281-35.patch, failed testing.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community

It seems like Drupal CI is having a hard time..

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: 2569281-35.patch, failed testing.

The last submitted patch, 35: 2569281-35.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC as per #35

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks @iMiksu for sticking with this. Committed 13ae3e1 and pushed to 8.0.x. Thanks!

  • alexpott committed 13ae3e1 on 8.0.x
    Issue #2569281 by iMiksu, dawehner, lauriii, alexpott, joelpittet, xjm:...

Status: Fixed » Needs work

The last submitted patch, 35: 2569281-35.patch, failed testing.

alexpott’s picture

Status: Needs work » Fixed

Status: Fixed » Needs work

The last submitted patch, 35: 2569281-35.patch, failed testing.

alexpott’s picture

Status: Needs work » Fixed

Hey DrupalCI I already committed this :)

Status: Fixed » Closed (fixed)

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