$message = t('The breakpoints from theme %theme are imported and !grouplink.', array(
        '%theme' => check_plain($themes[$theme_key]->info['name']),
        '!grouplink' => l(t('a new group is created'), 'admin/config/media/breakpoints/groups/' . $theme_key),
      ));

The complete sentence should be in the first parameter to t(). This is because sentence fragments like 'a new group is created' do not carry enough context for translators. The correct way to make a link here is to put the A tag inside that too, and use a !url replacement token, and url() to make the URL from the path.

CommentFileSizeAuthor
#13 incorrect_use_of_t-2558573-13.patch926 bytesrakesh.gectcr
#10 incorrect_use_of_t-2558573-10.patch870 bytesrakesh.gectcr
#5 incorrect_use_of_t-2558573-5.patch870 bytesPravin Ajaaz
#2 incorrect_use_of_t-2558573-2.patch873 bytesPravin Ajaaz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

Pravin Ajaaz’s picture

Pravin Ajaaz CreditAttribution: Pravin Ajaaz as a volunteer and at Ameex-Drupal Geeks commented
FileSize
incorrect_use_of_t-2558573-2.patch873 bytes

A patch based on joachim's suggestion

Pravin Ajaaz’s picture

Pravin Ajaaz CreditAttribution: Pravin Ajaaz as a volunteer and at Ameex-Drupal Geeks commented
Status: Active » Needs review
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

Thanks for the patch! Just one thing though:

+++ b/breakpoints.module
@@ -92,9 +92,9 @@ function breakpoints_themes_enabled($theme_list) {
+        '!url' => url() . 'admin/config/media/breakpoints/groups/' . $theme_key,

The path should go in the url().

Pravin Ajaaz’s picture

Pravin Ajaaz CreditAttribution: Pravin Ajaaz as a volunteer and at Ameex-Drupal Geeks commented
Status: Needs work » Needs review
FileSize
incorrect_use_of_t-2558573-5.patch870 bytes

Okay here it is.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Reviewed & tested by the community

Looks good! Setting to RTBC.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/breakpoints.module
@@ -92,9 +92,9 @@ function breakpoints_themes_enabled($theme_list) {
+      $message = t('The breakpoints from theme %theme are imported and <a href="!url">a new group is created</a>.', array(
...
+        '!url' => url('admin/config/media/breakpoints/groups/' . $theme_key),

    This should really use @url not !url. And adding drupal_strip_dangerous_protocols() around the url wouldn't hurt much. 

  2. +++ b/breakpoints.module
@@ -92,9 +92,9 @@ function breakpoints_themes_enabled($theme_list) {
         '%theme' => check_plain($themes[$theme_key]->info['name']),

    This is also wrong. drupal_placeholder() will check_plain() so this will double escape.

joachim’s picture

joachim CreditAttribution: joachim commented

> '!url' => url('admin/config/media/breakpoints/groups/' . $theme_key),

> This should really use @url not !url

But the only non-hardcoded bit there is $theme_key, which isn't user input AFAICT.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

We did some serious thought around this in core in d8 core and that was the hold up for RC release in Amsterdam Edit: Barcelona.

It's recommended to use :url in d8 and @url in d7. :url is one further and equivalent to check_url()

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at Valuebound commented
FileSize
incorrect_use_of_t-2558573-10.patch870 bytes

@joachim and @joelpittet

I rolled back your comments here is the changes 

       breakpoints_breakpoint_group_save($breakpoint_group);
-      $message = t('The breakpoints from theme %theme are imported and !grouplink.', array(
+      $message = t('The breakpoints from theme %theme are imported and <a href="!url">a new group is created</a>.', array(
         '%theme' => check_plain($themes[$theme_key]->info['name']),
-        '!grouplink' => l(t('a new group is created'), 'admin/config/media/breakpoints/groups/' . $theme_key),
+        '@url' => url('admin/config/media/breakpoints/groups/' . $theme_key),
       ));
rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at Valuebound commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

Thank you @rakesh.gectcr, couple things still here:

  1. +++ b/breakpoints.module
@@ -92,9 +92,9 @@ function breakpoints_themes_enabled($theme_list) {
+      $message = t('The breakpoints from theme %theme are imported and <a href="!url">a new group is created</a>.', array(

    !url needs to be @url here too. 

  2. +++ b/breakpoints.module
@@ -92,9 +92,9 @@ function breakpoints_themes_enabled($theme_list) {
         '%theme' => check_plain($themes[$theme_key]->info['name']),

    So %theme doesn't need check_plain still.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at Valuebound commented
FileSize
incorrect_use_of_t-2558573-13.patch926 bytes

@joelpittet
I have done the changes you mentioned

-        '!grouplink' => l(t('a new group is created'), 'admin/config/media/breakpoints/groups/' . $theme_key),
+      $message = t('The breakpoints from theme %theme are imported and <a href="@url">a new group is created</a>.', array(
+        '%theme' => $themes[$theme_key]->info['name'],
+        '@url' => url('admin/config/media/breakpoints/groups/' . $theme_key),
       ));
rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at Valuebound commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

I'm reviewing from my phone but looks good from here

Thanks for the new patch:)

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at Valuebound commented
Pravin Ajaaz’s picture

Pravin Ajaaz CreditAttribution: Pravin Ajaaz as a volunteer and at Young Globes commented
attiks’s picture

attiks CreditAttribution: attiks at Attiks commented
Status: Reviewed & tested by the community » Fixed

Fixed, thanks

  • attiks committed c956ed0 on 7.x-1.x authored by rakesh.gectcr 
    Issue #2558573 by rakesh.gectcr, Pravin Ajaaz, joachim, joelpittet:...

Status: Fixed » Closed (fixed)

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