Re: [drupal:dries] /modules contact.module
IMO this patch introduced even more inconsistencies in core than we already had.. Look at the way this patch dealed with HTML-tags inside the _help(). We never did that this way.. %category is IMO user submitted data, which is NOT translatable through drupal! It isn't now, and should never be wrapped through t(). I heared someone complain lately about bad reviewing of patches, but this isn't an excellent example either! C'mon Dries, why in Gods name did you applied this patch??? Steef Op 14-jan-2006, om 11:33 heeft drupal-devel@drupal.org het volgende geschreven:
User: dries Branch: HEAD Date: Sat, 14 Jan 2006 10:33:22 +0000
Modified files: /modules contact.module
Log message: - Patch #39135 by Zen: various contact form improvements/fixes.
+ Conversion to _validate + _submit model.. + Split replaced by explode - faster + Fixed typos: Recipient + Fixed weight defaulting to -10 + Popped mail subject formatting into a t() + Popped '--' formatting into a t() + Does a lot of documentation fixing/rewriting. + Renamed contact_user_mail form functions to contact_mail_user for consistency. + ...
Links: http://cvs.drupal.org/diff.php?path=drupal/modules/ contact.module&old=1.39&new=1.40
-- [ Drupal cvs list | http://list.drupal.org/ ]
--- Stefan Nagtegaal Drupal-Devel@iStyledThis.nl Drupal Development Mailinglist
On 14/01/06, Stefan <drupal-devel@istyledthis.nl> wrote:
IMO this patch introduced even more inconsistencies in core than we already had.. Look at the way this patch dealed with HTML-tags inside the _help(). We never did that this way..
%category is IMO user submitted data, which is NOT translatable through drupal! It isn't now, and should never be wrapped through t().
This is my patch. Please check: http://cvs.drupal.org/viewcvs/drupal/drupal/modules/contact.module?r1=1.39&r... You will notice that this is not introducing *more* inconsistencies in core.. These inconsitencies were already there. Please also check: http://drupal.org/node/39135#comment-65669 where I've stated that I don't understand at this point in time what the best practices are for the usage of url() and l(), and I'm also unsure of adding markup to t() calls - I'm told that it's accepted, but that I should keep it to a minimum. I've left all the URL calls in the _help t() calls as they are until I've done some reading up.. Thanks -K
Op 14-jan-2006, om 19:38 heeft Karthik het volgende geschreven:
On 14/01/06, Stefan <drupal-devel@istyledthis.nl> wrote:
IMO this patch introduced even more inconsistencies in core than we already had.. Look at the way this patch dealed with HTML-tags inside the _help(). We never did that this way..
%category is IMO user submitted data, which is NOT translatable through drupal! It isn't now, and should never be wrapped through t ().
This is my patch. Please check: http://cvs.drupal.org/viewcvs/drupal/drupal/modules/contact.module? r1=1.39&r2=1.40
You will notice that this is not introducing *more* inconsistencies in core.. These inconsitencies were already there.
Please also check: http://drupal.org/node/39135#comment-65669
where I've stated that I don't understand at this point in time what the best practices are for the usage of url() and l(), and I'm also unsure of adding markup to t() calls - I'm told that it's accepted, but that I should keep it to a minimum. I've left all the URL calls in the _help t() calls as they are until I've done some reading up.. Now, the contact.module is the *only* module which completely moves the <p>aragraph tags and <ul>, <li> tags right outside the whole translatable string. When I proposed this before, there was a lot of noise around this. It wasn't acceptable, logical and not the right way todo it.
I don't care how we handle with it, but we should handle it all the same. If we do this in contact.module, we do this everywhere otherwise I vote for reversing this patch.. This is not consistent with the rest of core drupal.. Steef --- Stefan Nagtegaal Drupal-Devel@iStyledThis.nl Drupal Development Mailinglist
Now, the contact.module is the *only* module which completely moves the <p>aragraph tags and <ul>, <li> tags right outside the whole translatable string. When I proposed this before, there was a lot of noise around this. It wasn't acceptable, logical and not the right way todo it.
So the idea is to allow for changes in mark-up via .po files? Also, I don't see the mark-up being consistently included inside t() calls in other modules either - a simple grep produced 112 lines (mostly in hook_help) that would be violating the <p> tags inside t() calls rule, and I'm sure there are more. So I'm not certain what the best practices are in this regard. Anyways, I really haven't looked into all this in detail - but if you can link me to some documentation on this, that will be excellent. I don't see anything related mentioned under "coding standards" in the handbook. Thanks in advance :) -K
So the idea is to allow for changes in mark-up via .po files?
Not at all. The main rule is to avoid markup in translatable strings, unless it would split up text that needs to be together for proper translation. Unfortunately there are some cases where people disagree. What everyone happily agrees on is links: bad: t("You may change permissions on the %accesscontrol page.") with %accesscontrol => l(t('access control'), 'admin/access', ...). good: t("You may change permissions on the <a href="%url">access control</a> page.") with %url => url('admin/access'); The link caption is part of the sentence, so it should be translated along with it. In some languages, the page title 'access control' would be in a nominative, while "on the access control page" would be in a dative (or equivalent). The same rule applies to similar short inline elements, like <strong> or <code>. For block level elements where the inside is a stand-alone string, there is also no debate: bad: t('<h2>Some title</h2>'); good: '<h2>'. t('Some title') .'</h2>'; The help texts are a different story. An important point is whether different paragraphs of a help text belong together in one t(...) or not. If they do, then the '<p>' tags must be part of the translatable string (otherwise it would contain unmatched '</p><p>' inside it). If they don't, each paragraph can have its own '<p>'. t(...) .'</p>' call. But unless I'm mistaken, Stefan's proposal a while ago was not just about moving the '<p>' outside the t() but also out of hook_help() and into the theme. This is a different issue and it was rejected because of multi-paragraph help; it would have to contain unmatched '</p><p>' as well. Steven Wittens
Steven, This is very useful :) I see what you mean about inline tags - so for e.g. (probably a poor one at that): Nominative: t('I am of the <em>opinion</em> that..'); Dative: t('<em>Methinks</em> that ..'); So, using tags outside the t() here would break the translation or make it kludgy. Is that right? And the use of the <p> tags are a lot more subjective, with some passages better off with the <p> tags embedded, thereby providing a little flexibility to the translator? Please confirm when you can. And thanks for taking the time to write this up :) Cheers, -K
Not at all. The main rule is to avoid markup in translatable strings, unless it would split up text that needs to be together for proper translation. Unfortunately there are some cases where people disagree.
What everyone happily agrees on is links: bad: t("You may change permissions on the %accesscontrol page.") with %accesscontrol => l(t('access control'), 'admin/access', ...). good: t("You may change permissions on the <a href="%url">access control</a> page.") with %url => url('admin/access');
The link caption is part of the sentence, so it should be translated along with it. In some languages, the page title 'access control' would be in a nominative, while "on the access control page" would be in a dative (or equivalent).
The same rule applies to similar short inline elements, like <strong> or <code>.
For block level elements where the inside is a stand-alone string, there is also no debate: bad: t('<h2>Some title</h2>'); good: '<h2>'. t('Some title') .'</h2>';
The help texts are a different story. An important point is whether different paragraphs of a help text belong together in one t(...) or not. If they do, then the '<p>' tags must be part of the translatable string (otherwise it would contain unmatched '</p><p>' inside it). If they don't, each paragraph can have its own '<p>'. t(...) .'</p>' call.
But unless I'm mistaken, Stefan's proposal a while ago was not just about moving the '<p>' outside the t() but also out of hook_help() and into the theme. This is a different issue and it was rejected because of multi-paragraph help; it would have to contain unmatched '</p><p>' as well.
Steven Wittens
participants (3)
-
Karthik -
Stefan -
Steven Wittens