Better error messages for potx errors / code reviews.

I think it is best if you suggest people to use the potx coder review to those who would like to do localization code checks and develop additional review items in coder or elsewhere. Since this code is and will be used for extraction and it already knows about the error conditions, it is best not to develop another review tool separately, while the potx tool itself can be used to do the review, so there is no question that it is up to date to the latest parsing modes, it is not required to keep two tools in sync :) I admit we can make the error messages better, we can put in more hints for coder to display, etc, I am totally open to these things!

As far as the actual error cited goes, t() needs the string literally, not through a variable. Marker errors are basically anything but a literal string used inside of t().

Since http://drupal.org/node/309875 is already about making the code review inside potx better (please do help out there!), let's make this an issue on better error messages from potx. I've made up a quick patch against the Drupal 5.x.-1.x branch. Since this module runs on four branches, this would need to be ported to four places, so it would be good to check whether this would be useful for your needs and then I can port the changes over. Although you could not try this patch out given the Drupal 5 branch does not have coder integration, I am hopeful you can review and provide feedback on the patch as-is.

Part of the feedback I am looking for is whether we should separate the error message itself and its explanation, so coder can report the error and provide the explanation on demand.

Ok, it was not hard to apply the same changes to the Drupal 6.2 version, so you can actually try it out with coder. Also fixed a style error, so now _potx_marker_error() generates the same all-on-one-line string as the other explanations. Note that http://drupal.org/node/309875 fixes coder integration issues due to the updated coder, so you might need to test the two in tandem.

I am eager for feedback.

Doh, already found an omission. _potx_find_t_calls() is used to catch $t(), st(), and in Drupal 5 even watchdog() calls, since these have the same rules as t(). So better use the $function_name used at that moment in the error message as well. Updated the error to reflect this. Should be good for review now.

Yeah, two of the already implemented checks at the page you linked to are duplicates of what potx already does. Commented on it at http://groups.drupal.org/node/15230#comment-51218

Checking t() in format_plural(), an issue you opened would again be a duplicate, so commented on it at http://drupal.org/node/312510#comment-1027254

I agree code reviews for missing t() calls is not done in potx (and it might even be a good idea to do it right here), but bogus calls to locale functionality is checked and reported.

Have you also applied http://drupal.org/node/309875 as I've suggested (I am looking for some fixing of the patch there, but it should work on an English site with Garland)?

Hm, well, I've used the potx coder review successfully with coder quite a few times before. I did not find good docs on how to write custom code reviews, so reverse engineered to find out how should I plug in to there. I think you might be more qualified to look into remaining issues with the integration. This bug is definitely tracked at http://drupal.org/node/309875 and should be fixed to go forward. That might also mark current potx to be useable only with older coder versions while post-commit with newer coder versions (or I did bad reverse engineering and the new stuff will work with both old and new).

Yay, great! We might be able to make the errors shorter with some text rework (some great modifications you have already done. We can omit the "invalid marker content" part and focus on more tailored concrete error messages. We can also do some refactoring here to not transfer file name and line number in the error message to coder, but only as meta info, which would lead to some refactoring of potx_status().

When used outside of coder module, we still need the file and line number for errors. When potx is used form the command line, we print errors to the command line, when potx is used under l10n_server, the l10n_server logs the errors in the database, when potx is used from the web interface, the errors are printed as drupal error messages. So there are at least three other options where we need the line number. This file name / line number refacoring could very well be part of http://drupal.org/node/309875 where this is sort of happening already although via reverse engineering the error message :)

Ok, refactoring patch up on http://drupal.org/node/309875

Ok, more to the point messages in this patch. The generic "Invalid localization code" part is now removed. Also, for the JS checks, we could know if t or formatPlural is the culprit, so we can split and use the right error message instead of a long one. This also has better error message for _perm() hooks, using the hook implementation function name found instead of the generic error.

Let's get this tested and reviewed.

Yeah, well, my assumption with including the code snippet in the error is that there might be multiple t() calls on one line, eg:

$value = $condition ? t('This is good and looonggg...') : t('This '. 'is'. ' bad');

and in this case, it might help to have the exact bogus call at hand, so you don't need to dig up the line. Granted, lines should be short and all, so we might as well skip the concrete error to another structured item in the array and not pass that to coder. What do you think?

Ok, will try to find some time to work on this soon. I am making a note that http://drupal.org/node/194962 is open to add a developer page which we could point to for more explanation on "how to write translatable source code" in the coder error messages. That page is still to be written.

sub

Please, add periods to sentences. An also often ignored and possibly unwritten rule...

Well, but I wrote the above as I saw missing periods in the above patch. So "patch needs work" is correct.

Hass, periods are already at the end of all sentences in all messages produced by this patch. Just run the patch and you will see.

This is incorrect. Here is only one example from the patch (3 or 4 of this bugs are inside):

Wrong:

t('The first paramter to @function() should be a literal string. There should be no variables, concatenation, constants or other non-literal strings there', array('@function' => $function_name)));

Correct:

t('The first paramter to @function() should be a literal string. There should be no variables, concatenation, constants or other non-literal strings there.', array('@function' => $function_name)));

Hass, let me repeat: Just run the patch and you will see. We are adding up the token information at the end of the message. We could of course just add a placeholder in the message and and it in ': %error_found.', but stella already requested to remove this from the message, so we are heading to where you think this patch should have been already. The messages when running out of coder module will be slightly longer this way though.

You may be correct that there comes a period in the output, but then we may have a context sensitive bug. If the period of a sentence written inside t() is outside of t() we have a context sensitive bug. I haven't read all the above... only taken a look to the patch and this one seems to have context sensitive bugs. No need to run the patch to find such bugs :-).

Anyway, here is a new patch, which to coder's need introduces the concept of the code excerpt. The code excerpt is now rendered outside of the helpful error message, so complete sentences for helpful error messages are a given now. The location info may contain the excerpt now if available. There are four different combination possibilities for location information details, so all have their t() strings and compositions, and the information ends up at the end of the string in 3 of the four modes. The structured mode stores the excerpt info, but the coder code throws it away, since it does not need that.

@stella, what do you think of this one?

Ps. I did find some code style issues in the module otherwise as well. Let's clean them up after we improved the coder integration!

Doh, the patch.

Once I fixed a glitch in line numbering and committed a follow up to http://drupal.org/node/309875 (see that issue), this looked great to me as well. :) Committed to 6.x-2.x.

Inspired by your above comments, I've also fixed coder errors on this branch: http://drupal.org/node/313789

Once we have docs page on best practices, we can expand on this by adding a read more link to the docs.

Well, I was checking the code excerpt, not the line number. The code excerpt was one off since it used the line number as an array index. So should coder get a $lineno which starts from 1, but its array for the source code lines should be indexed from 0?

What do you think Stella?

Double checked this. I've added two artificial hook_menu() errors to l10n_client module and then looked for potx coder review and built in coder review output. Looks like coder and potx reporting errors on the same line. Coder says t() should not be in hook_menu(), while potx says it should be a literal string. Both errors reported on the same line. I consider this works right with the current code.

Screenshots attached.

Since 5.x is not maintained, moving back to fixed. If the lines happen to be an actual problem, we can open a separate issue to fix. I remember checking line numbers with the 7.x port with coder, and that worked right.