UX improvements

Wow! That was fast! I'll review this later. :)

This needs a reroll; Moving the admin settings form into salesforce_api.admin.inc has already been committed elsewhere. Several whitespace changes have also been committed. There are some issues about some of the changes:

Check out the code from CVS so that your patches are cleaner and don't include additions that drupal.org's packager makes to *.info files in the downloadable tarballs.

Setting then restoring salesforce_api variables is a hack, and prone to error, for example if the toolkit is unavailable. Instead, modify salesforce_api_connect() to take connection settings as an optional parameter. If the parameter has not been passed then salesforce_api_connect() should get the settings from variable_get(). Make the optional parameter NULL by default, but pass it an array, so that the function's signature and function documentation doesn't get ugly.

You'll need to re-add $form['#validate'][] = 'salesforce_api_settings_form_validate'; back into the settings form and patch.

salesforce_api_enable() is mostly good, though salesforce module usually refers to "Salesforce", not "SalesForce", and we should be consistent on this. We should address this issue too, though perhaps in a separate issue and patch.

The method you have used for t() doesn't allow the link text to be translated. For better i18n-compatibility t() should take a constant string as it's parameter, not a variable. The words "click here" are meaningless in links.

Suggestion to solve the above issues; drupal_set_message(t('Salesforce module requires you <a href="!url">enter your Salesforce connection details</a> before it can connect to Salesforce.com.', $vars));

salesforce_api_requirements() looks, though I didn't run it. I'm not sure if curly braces in switch commands is accepted in Drupal's coding standards, though I think they should be! What a great idea! I wouldn't use compact() for something like this since it adds unnecessary vagueness without saving many lines of code, which makes it harder for less-experienced programmers to read.

Links, clean urls, and aliases, do not cause problems because url() handles this at render-time taking all of that into consideration. !url gets substituted for url()'s return value at render-time too. In fact <a href="http://api.drupal.org/api/function/l/6">l()</a> uses url() to generate the URI that l() puts in <a href="">.

The patch still needs review, though feel free to correct this and re-submit.

url() and l() are not the same. l() is dependent on url()

url() returns a uri, e.g. http://drupal.org/node/12345
l() returns an html link, e.g.<a href="http://drupal.org/node/12345">Some node</a>

In order to make strings translatable one whole conmprehensible and meaningful phrases, including the parts inside and outside the <a>, we call t() ONCE with the whole phrase, and include minimal amounts of html in that translatable string.

Imagine trying to translate "Salesforce module requires you !link before it can connect to Salesforce.com." and "enter your Salesforce connection details" and as 2 disconnected and separate language strings, without the context of each other. Translation becomes difficult and the translation is likely to lose it's meaning.

Translation IS hard.

I'm not sure what is sub-optimal about t('Some <a href="!url">links</a> in a string', $vars); though? Because the translator has to look at some html? The translator MUST decide which text goes inside and outside the link – so there has to be SOME kind of placeholder. Arguably it could be simpler than <a href="!url">, though I don't think that is very complicated. If it were <em> instead I think it would be fine. Anything more complicated than one or two trivial attributes would probably start to be a problem though. In that case you could also do

  $attributes = array('href' => url($path), 'class' => 'foo bar');
  $vars = array('!attributes' => drupal_attributes($attributes));
  t('Some <a !attributes>links</a> in a string', $vars);

But that quite quickly starts to get more complicated in the PHP than necessary.

Fixed a simpler version in http://drupal.org/cvs?commit=245274