Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to #2569279: Replace remaining !placeholder for Non-URL HTML outputs only in contextual.module
Problem/Motivation
See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only
https://www.drupal.org/node/2566503#comment-10325457
https://www.drupal.org/node/2566503#comment-10334587
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff.txt | 1.84 KB | iMiksu |
#35 | 2569281-35.patch | 2.74 KB | iMiksu |
| |||
#32 | interdiff.txt | 1.43 KB | iMiksu |
#32 | 2569281-32.patch | 2.88 KB | iMiksu |
#27 | 2569281-27.patch | 2.79 KB | iMiksu |
Comments
Comment #2
alexpottComment #3
lauriiiHmm, somehow we would need to add the space before the link
Comment #4
dawehnerWe could also just leverage #2568977: Replace SafeMarkup::format() in the link generator - it's a bad example to everyone once it in.
Comment #5
dawehnerComment #6
dawehnerLet's be honest for now.
Comment #7
alexpottThis is part of the critical meta.
Comment #8
dawehnerOn top of the other issue I would go with
Comment #9
lauriiiI guess there is some unrelated changes in the latest patch
Comment #10
dawehnerIts #2568977: Replace SafeMarkup::format() in the link generator - it's a bad example to everyone + the interdiff (the patch for this actual issue)
Comment #11
iMiksuI can create patch with #3 + #8 interdiff
Comment #12
iMiksuCan't do it. The first #3 patch conflicts with the #8 interdiff. Whats needs to done here actually?
Comment #13
iMiksuTalked in extended sprints, I'll re-roll this.
Comment #14
iMiksuDuring 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.
Comment #15
joelpittetWe don't need the array anymore I'm quite positive
Comment #16
iMiksuRe-rolled as #8 interdiff, simple as that :)
Comment #17
joelpittetThis looks like the correct approach to the problem and also simplest.
Thank you
Comment #18
alexpottI agree this is the simplest thing, the output of
Drupal::l()
is already marked safe... however...Why are these string separate? we could join them up like this...
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.Comment #19
iMiksuComment #20
iMiksuEDIT: So I decided to add comments around the string you mentioned (cross reference).
Comment #21
lauriiiWe could use @see here, @see documentation: https://www.drupal.org/coding-standards/docs#see
Comment #22
iMiksu@see that ;)
Comment #23
iMiksuWe can actually forget this change and make end result to be a string.
Comment #24
xjmGood catch!
This made me wonder why the variable is being initialized there at all. Looking at the whole thing:
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.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.
Comment #25
xjmFor #24.
Comment #26
iMiksuThanks! Will work further.
Comment #27
iMiksu$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 :)
Comment #28
lauriii! has to be converted to : after its been added to Drupal core. Otherwise this looks good.
Comment #29
lauriiiThe texts should be actually "Reset your password" and should has a title attribute "Send password reset instructions via e-mail."
Comment #30
iMiksuOK, the text was changed as alex suggested, but I agree that the shorter link with the original title is better.
Comment #31
alexpott@iMiksu I made a mistake - sorry.
Comment #32
iMiksu@alexpott no probs, hopefully this will get things further now. Fingrers crossed!
Comment #33
iMiksuFor #32
Comment #34
alexpottOh 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.
Comment #35
iMiksuThanks for feedback, here's new patch. As we discussed with @alexpott, I've also removed second statement in
#access
as it can't beFALSE
.!$form_state->get('user_pass_reset')
should work there just fine.Comment #36
joelpittetReviewed 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.
Comment #38
joelpittetWeird this didn't fail...
Comment #40
lauriiiIt seems like Drupal CI is having a hard time..
Comment #43
alexpottBack to RTBC as per #35
Comment #44
alexpottThanks @iMiksu for sticking with this. Committed 13ae3e1 and pushed to 8.0.x. Thanks!
Comment #47
alexpottComment #49
alexpottHey DrupalCI I already committed this :)