Diff module string review

Part of the nicety of "Show diff" was how short it was. "Show difference" makes the button a lot wider, with the possibility that it would either wrap to two lines, or simply make the table column wider than it needs to be (since it just houses two radio buttons).

"Compare" is a better word for this button for both the UI advantage (no wrapping, narrow column) and for UX / translatability.

+  drupal_set_title(t('Revisions for %title', array('%@title' => $node->title)));

This should be

+  drupal_set_title(t('Revisions for %title', array('%title' => $node->title)), PASS_THROUGH);

Bumped to 3.x as this is the latest version and for i18n users.

Line 320.

-    '#options' => array(0 => '< ' . t('No highlighting') . ' >'),
+    '#options' => array(0 => t('- No highlighting -')),

I'm not an i18n expert, but isn't the previous version better? Or change it to

+ '#options' => array(0 => '- ' . t('No highlighting') . ' -'),

I agree with #5, not sure about how to confirm for #4, if $node->title has already been passed through xss_filter / check_plain.

@hass is right. I just did a search through a Drupal 7 project, and it turned up lots of uses of '- None -', '- Select a value -', '- Please choose -' etc.

The only thing to confirm now is if #4 would open up a security hole or not.

You can use PASSTHROUGH because t() has already sanitized it.

...you can put variables like @name into your string, and t() will substitute their sanitized values at translation time...

FYI - There is a syntax error that was fix in #4 (%@title -> %title) too.

re #9, Thanks for that reassurance. I knew "%values", were wrapped in <em>, but didn't realize they were also sent through check_plain() until I clicked all the way through to drupal_placeholder().

I think that #1785742: Diff module string review should be restricted to 3.x branch as it is a string change. This would be problematic on existing 2.x i18n sites during what should be a minor upgrade.

PS: The only thing holding back a beta, release or even full release of 3.x is to find some time to simplify the configuration settings.

#14++ ! Well said.

I felt like there was inconsistency between where 'difference' and 'differences' were used. I also updated the Help text to something better (IMHO).

This is just an updated version of #11.

#16 changed "difference between" to "differences between", in the patch... however, there are other places in the code that were neglected that still say "difference between". Two examples:

'description' => t('The difference between the current revision and the previous revision of this node.'),
[...]
'description' => t('The difference between the current revision and the previous revision of this node, but comparison uses a markdowned state.'),

I'd propose that entire line be cleaned up, something like:

'description' => t('Show the differences between the current revision and the previous revision of this node.'),
[..]
'description' => t('Show the differences between the current revision and the previous revision of this node using markdown.'),
 

I'm primarily doing issue queue maintenance now, so my main contribution is:
Categorizing as task to indicate that this is on the development roadmap.
Raising priority to tell reviewers this needs a consensus soon.
---

Other than that, AFAICT, #11 is the latest/best patch, and interdiffs would be very helpful.
---

In case upstream standards are needed, see #1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text.

Too many things brought up in here for me to have a clear understanding of this issue. Are we simply dealing with checking all the translatable strings for consistency? Are we also dealing with the @ vs. ! placeholders?

Well, here's a modification of the patch in #11 that adds the comments from #17. I also looked through all the t( strings I could find. There are a few places that still refer to 'diff', but I feel like those are references to the module and not referring to 'differences' (comparison)

Should we not move that to a separate issue for clarity?

Ok, I'll take a look at it with that in mind now.

I'm re-rolling @twardnw's version in #21 with some additional wording cleanup/clarification, a spelling fix ("compatable").

One thing though:

+++ b/diff.pages.incundefined
@@ -194,14 +194,14 @@ function diff_diffs_show($node, $old_vid, $new_vid, $state = NULL) {
+    $next_link = l(t('next difference >'), 'node/' . $node->nid . '/revisions/view/' . $new_vid . '/' . $next_vid . $nav_suffix, array('absolute' => 1));

Why the word "difference" rather than "change"? It's really a set of differences, not that we want to complicate it by calling it that. But it seems we could do better than "difference" here.

It's still "difference" in this patch.

@hass #22, I went through all of the files in the module looking for illegitimate uses of !placeholder and couldn't find any.

Some of the files in the includes directory (e.g. includes/image.inc) look very suspicious, but as I understand it, the array they're generating gets passed to the DiffEngine which returns HTML encoded results (the actual diffs).

I went through the code paths to make almost all of the !placeholders come up with "<script>alert('foobar');</script>" in field/title/etc. content and all were fine.

Stable Diff is needed for Drupal.org D7 upgrade, and a stable Diff depends on this issue. Elevating to critical as the deadline is the end of this month.

The Diff engine expects raw HTML, so don't get to worried about that. The only reason this came up was the fact that the page title in the original patch had an issue. See #4.

And for other reviewers, many of these changes reflect that core now inserts the translated string directly rather than passing it in as a translated sub-string.

- %view_revisions
+ View revisions

i.e. searching the code with \'%[a-z]*\' \=\> t\(\' results in no results outside of tests. (All of these changes seemed redundant so I followed this up to discover this fact.)

cross post, retagging

Re-prioritizing as I don't think Alan D. meant to change priority (or did you?).

Sorry, no ;)

I will try to make time this weekend to help (In Oz, so Friday/Saturday in the US), as I am flooded at work and a personal project that has a Friday deadline here.

As per version / revision discussion, revision appears to be preferred term and I think we should not get side-tracked with this. Version is a concrete instance that may or may not be a revision; revision is an altered form of the original version.

We could use both like wikipedia, but the UI is inconsistent. This is the likely reason that the big players, (Google, Microsoft, etc) tend to only use one or the other. People from the VCS / MS camp probably would prefer version, and the others would lean towards revision. I think that it is an impossible debate to win.

Versions? Oh, are you referring to this?

-      return '<p>' . t('The revisions let you track differences between multiple versions of a post.') . '</p>';
+      return '<p>' . t('Revisions allow you to track differences between multiple versions of your content, and revert back to older versions.') . '</p>';

This string comes from core (Node module). I don't see anywhere else in Diff that "version" is used user-facing. Also, I don't think we should change "version" to "revision" here, because it would sound like a circular definition.

No, only a passing reference to the use of these terms overall. Sorry to cause confusion.

@hass
Are you referring to the strings like this?

t('This is only used if the "Enable <em>Title</em> field" is checked in the instance settings.')

I've removed the p tags from the #description mentioned. It would be nice to roll this soon.

-      '#description' => t("<p>You need to enable this view mode in the @type_label type's <em>Custom display settings</em> to control settings other than just what fields are shown. Once enabled you can configure many of the diff options used to compare these fields.</p><p><em>If it is not selected, all compatible fields will be compared using default settings.</em></p>", array(
+      '#description' => t("You need to enable this view mode in the @type_label type's <em>Custom display settings</em> to control settings other than just what fields are shown. Once enabled you can configure many of the diff options used to compare these fields. <em>If it is not selected, all compatible fields will be compared using default settings.</em>", array(

Missed on P tag

Thanks everyone, committed 126f317. I learned a bit about best practices here for my other projects.

Please create new issues about other string changes. Only reopen this thread if there are issues directly related to the changes committed here.

Another change, pinging this thread as someone may still be listening to it...

#2117133: Change "This is the published revision"

Currently this is unconditionally 'This is the published revision.'

Suggested is 'This is the current revision.',

We can probably conditionally show two messages here if we have context to $node. Suggesting either of these:

This is the current revision.
This is the published revision.

Or

This is the current revision.
This is the current revision (unpublished).