Proper multibyte support for string operations

Actually, I'm wondering if mb_substr() could just be dropped-in instead of substr() in the last line from the snippet above? Perhaps this has performance issues?

Even better!

Yeah, it's just the addition of "drupal_" to the "substr" part of line ~1060, making:

$max_len = $sf_field_definition['salesforce']['length'];
if ($max_len && drupal_strlen($object->$sf_fieldname) > $max_len) {
  $object->$sf_fieldname = drupal_substr($object->$sf_fieldname, 0, $max_len);
}

I wasn't sure if this needed a patch.... dealing with a patch just seems like more trouble than it's worth to change/test this. I just ran a test with some Chinese characters in a mapped field (that had a character limit) and it seems to work correctly with this change (the field got trimmed properly, etc.).

Not sure if it needs another tester, but I don't see much harm in the change either way, especially as drupal_substr() is advertised as using the same syntax/concept as substr():

http://api.drupal.org/api/drupal/includes--unicode.inc/function/drupal_s...

Makes me wonder if there are other places where substr() should be swapped-out with drupal_substr()... but that's another thing altogether.

Yes, and it looks like drupal_substr() uses mb_substr() when it knows it's dealing with unicode characters and will otherwise do a bunch of calculations to find a safe trimming point. So it looks like the Drupal version of this function is built to get around these kinds of issues (such as the one in this thread) with one go-to trimming function.

Also, I just noticed that running a module through coder under the 'minor' setting (detect even the most minor errors) will report all cases where it's recommended to "replace the string function with the drupal_ equivalent string functions". So it's looking like salesforce_api just needs to be put through coder.

I just want to check and see why this was closed. From what I can see in the repository there are still cases in the 6.x-2.x and 7.x-2.x branches where substr() is used when drupal_substr() may be a better choice. I can't be 100% sure if it's just a matter of swapping these functions in these handful of places, but the conversation above indicates some attention may still be necessary to properly support utf-8.

I'm assuming this is no longer a problem in at least 7.x-3.x, as it sounds like that's a total re-factor.

If I'm missing something and this is in fact fixed in pre 7.x-3.x, I apologize.

I'll make a patch for this momentarily... soon after opening this thread it looked like Evan was going to address this. I now see however that Evan is not so much involved on the development of this project anymore. Anyway, this can be quite a simple fix that may save people trouble.

Ok, patches for 6.x-2.x and 7.x-2.x are attached. I had a quick scan of all module files and though there are several places where substr() could probably be swapped with drupal_substr() there is only one case where I see that it's particularly problematic to not use drupal_substr(). This is the case that this patch addresses. Please see also comment #5 for these specifics.

Even if this does not get a community review I'd encourage one of the maintainers to have a look at this. It's a pretty innocuous change and is as much about making sure the module follows Drupal coding standards as it is about fixing the problems highlighted in this thread.

Also note that the patches are git-formatted patches with include my authorship info. If this gets committed, I'd certainly love it if those author details were also in the commit (http://drupal.org/node/1146430). Cheers!

Thanks Kosta for giving this such quick attention!