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 #2564353: Remove !placeholder usage from SafeMarkup::format()
Problem/Motivation
Found by @alexpott in #2564353: Remove !placeholder usage from SafeMarkup::format()
Avoid using SafeMarkup::format()
like this, the concatenation is much more suited for the template:
SafeMarkup::format('@onefish @twofish')
Proposed resolution
Move concatenation with SafeMarkup::format()
in template_preprocess_locale_translation_update_info(
) to the template.
See parent issue for beta evalution.
Remaining tasks
- Create Patch
- Review
- Commit
Steps to Reproduce
- Enable Interface Translation & lanaguage
- Enable one other language
- Go to Reports admin/reports/translations
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#20 | Screen Shot 2015-09-19 at 21.34.17.png | 68.5 KB | lauriii |
#20 | Screen Shot 2015-09-19 at 21.34.15.png | 68.95 KB | lauriii |
#20 | Screen Shot 2015-09-19 at 21.32.51.png | 64.06 KB | lauriii |
#20 | Screen Shot 2015-09-19 at 21.32.49.png | 53.43 KB | lauriii |
#20 | Screen Shot 2015-09-19 at 21.30.09.png | 41.19 KB | lauriii |
Comments
Comment #2
joelpittetComment #3
star-szrWould this maybe be an item_list suggestion specific override? Or some other way of doing this? Because what's being built is being passed to item_list as far as I can tell.
Comment #4
joelpittetHere's a first stab at this. Almost removed the preprocess if we had a format_date() equivelent filter in Twig.
Twig comes with a
date
filter http://twig.sensiolabs.org/doc/filters/date.htmlbut it doesn't know about drupal's custom filters. We have an issue that talked about it in relation to something else but I can't seem to find it ATM.
Comment #5
joelpittetcross posted with @Cottser. Re #3 I went with my usual, "item list be damned" approach. Because that gives the template the greatest flexibility IMO for markup.
If we force an item-list to be used. We handcuff the variables to the preprocess. I've gotten away with this approach once so far for theme table in one of the conversions. I wonder if my reasoning will fly this time too.
My other thought was doing an include of the item list template and pumping the variables in the template, but we haven't really done that yet.
Comment #8
joelpittetWorking on the test failures.
Comment #9
joelpittetHopefully my removal of item-list doesn't have an effect on the current styles(though I doubt it).
Realized a removed the generated variable from preprocess but forgot to change it in the template.
This patch changes that variable to the one provided.
Comment #10
joelpittetComment #11
joelpittetBecause it's a child of a major, I'm bumping this.
Comment #12
joelpittetRe-roll due to conflict with #2564353: Remove !placeholder usage from SafeMarkup::format()
Comment #13
lauriiiRemoving the item-list seems to have effect for the visuals of that page.
Before:
After:
Comment #14
star-szrOverall the code looks excellent. Just some small points:
Minor: Could use short array syntax here :)
Just a thought but since we're changing this maybe just change the variable to 'available_updates'?
We also need to document this variable in the Twig template.
Minor: s/twig/Twig.
Since we're changing this stuff, format_date() is deprecated so we should probably swap it out for \Drupal::service('date.formatter')->format().
Comment #15
joelpittetThanks for your two reviews, here is a fix for all the things.
Comment #16
joelpittetComment #17
lauriiiTested the /admin/reports/translations page with available updates and no available updates and the visual regressions has been fixed. Also the code looks good for me :)
Comment #18
LewisNyman CreditAttribution: LewisNyman at Wunder commentedRTBC++
Comment #19
alexpottCan we get screenshots with more than one language and with and without available updates? The screenshots don't show text from all the changes made by the patch.
Comment #20
lauriiiHere's bunch of screenshots:
Before:
After:
Before:
After:
Before:
After:
Before:
After:
Comment #21
alexpottThanks Committed c0d6bbc and pushed to 8.0.x. Thanks!
Comment #23
star-szrThanks all, here is a small markup-related followup: #2571553: Followup: Remove unneeded item_list class from locale-translation-update-info.html.twig