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

  1. Create Patch
  2. Review
  3. Commit

Steps to Reproduce

  1. Enable Interface Translation & lanaguage
  2. Enable one other language
  3. Go to Reports admin/reports/translations

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Title: Remove template_preprocess_locale_translation_update_info() and move logic to template » Move logic from SafeMarkup::format() from template_preprocess_locale_translation_update_info() to template
Issue summary: View changes
star-szr’s picture

Status: Needs review » Active

Would 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.

joelpittet’s picture

Status: Active » Needs review
FileSize
4.86 KB

Here'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.html
but 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.

joelpittet’s picture

cross 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.

Status: Needs review » Needs work

The last submitted patch, 4: move_logic_from-2565123-3.patch, failed testing.

The last submitted patch, 4: move_logic_from-2565123-3.patch, failed testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet

Working on the test failures.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
1.45 KB
4.81 KB

Hopefully 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.

joelpittet’s picture

Priority: Normal » Major

Because it's a child of a major, I'm bumping this.

joelpittet’s picture

lauriii’s picture

Status: Needs review » Needs work
FileSize
41.32 KB
41.96 KB

Removing the item-list seems to have effect for the visuals of that page.

Before:

After:

star-szr’s picture

Overall the code looks excellent. Just some small points:

  1. +++ b/core/modules/locale/locale.pages.inc
    @@ -52,52 +52,19 @@ function locale_translation_manual_status() {
    +    $variables['available_updates_list'] = array();
    

    Minor: Could use short array syntax here :)

  2. +++ b/core/modules/locale/locale.pages.inc
    @@ -52,52 +52,19 @@ function locale_translation_manual_status() {
    -    $details['available_updates_list'] = array(
    ...
    +        $variables['available_updates_list'][] = $release;
    

    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.

  3. +++ b/core/modules/locale/locale.pages.inc
    @@ -52,52 +52,19 @@ function locale_translation_manual_status() {
    +        // Format date for twig template.
    

    Minor: s/twig/Twig.

  4. +++ b/core/modules/locale/locale.pages.inc
    @@ -52,52 +52,19 @@ function locale_translation_manual_status() {
    +        $release['date'] = format_date($update['timestamp'], 'html_date');
    

    Since we're changing this stuff, format_date() is deprecated so we should probably swap it out for \Drupal::service('date.formatter')->format().

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.61 KB
5.59 KB

Thanks for your two reviews, here is a fix for all the things.

joelpittet’s picture

Issue summary: View changes
lauriii’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Tested 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 :)

LewisNyman’s picture

RTBC++

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

Can 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.

lauriii’s picture

Here's bunch of screenshots:

Before:

After:

Before:

After:

Before:

After:

Before:

After:

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks Committed c0d6bbc and pushed to 8.0.x. Thanks!

  • alexpott committed c0d6bbc on 8.0.x
    Issue #2565123 by joelpittet, lauriii: Move logic from SafeMarkup::...
star-szr’s picture

Status: Fixed » Closed (fixed)

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