Bootstrap sets C locale, but does not set UTF-8 character encoding

Drupal should not affect the locale except for the bare minimum like LC_NUMERIC to work around some PHP bugs.
First, let's see if this line is needed at all (eg : if tests about floats are here)

I do agree on assuming (and setting) an UTF-8 character encoding. I only disagree on the LC_ALL + C combination.
I'm sorry but I can not take the time to provide patches for the testsuite (for which tests are missing as bugs currently *exist* on this subject), as illustrated by drush ev 'echo iconv("utf8", "ascii//translit", " héllô") . "\n";'.

The patch in #2 is definitely wrong, which has been sufficiently discussed in #614124: Bootstrap should reset locale settings already.

Example bug report: #1502924: escapeshellarg() has problems with non-ASCII filenames

Clarifying the issue title.

I agree with the patch in #1.

But alas, it doesn't make a difference on my local Windows machine, which appears to retain UTF-8 correctly. Let's see what the testbot thinks.

Updated issue summary.

If this is the bugfix for #371495: Error message from drupal_http_request() not UTF8 encoded, this needs backport to D6, too.

According to this post, C.UTF-8 does not appear to be a valid locale : there's no non-ascii "POSIX" locale. The most standard being en_EN (http://sources.redhat.com/ml/libc-alpha/2009-09/msg00042.html)
Anyway :
php -r "setlocale(LC_CTYPE, 'C.UTF-8'); echo escapeshellarg('äöüß');"
'äöüß'
php -r "setlocale(LC_CTYPE, 'C'); echo escapeshellarg('äöüß');"
''

I'm personally fine with UTF-8 LC_ALL, but more theoretically, once again, why use LC_ALL if LC_CTYPE is sufficient.
To fix both this and #614124: Bootstrap should reset locale settings, I'd replace the setlocale() line by

 // fixes #1561214 : escapeshellarg()
setlocale(LC_CTYPE, "C.UTF-8");
// fixes #614124 : workaround float php bugs
if(  localeconv() [ "decimal_point" ] != "." )
  setlocale(LC_NUMERIC, "C");

Damn PHP...
It is 2012 outside and we still have this backward issues with stripping non-latin characters.
I vote for a PHP independent solution, like implementing our own escapeshellarg() (drupal_escapeshellarg()) to not couple with PHP anymore.

Just for reference, this is PHP implemenation of escapeshellarg:

/* {{{ php_escape_shell_arg
 */
PHPAPI char *php_escape_shell_arg(char *str)
{
	int x, y = 0, l = strlen(str);
	char *cmd;
	size_t estimate = (4 * l) + 3;

	TSRMLS_FETCH();

	cmd = safe_emalloc(4, l, 3); /* worst case */

#ifdef PHP_WIN32
	cmd[y++] = '"';
#else
	cmd[y++] = '\'';
#endif

	for (x = 0; x < l; x++) {
		int mb_len = php_mblen(str + x, (l - x));

		/* skip non-valid multibyte characters */
		if (mb_len < 0) {
			continue;
		} else if (mb_len > 1) {
			memcpy(cmd + y, str + x, mb_len);
			y += mb_len;
			x += mb_len - 1;
			continue;
		}

		switch (str[x]) {
#ifdef PHP_WIN32
		case '"':
		case '%':
			cmd[y++] = ' ';
			break;
#else
		case '\'':
			cmd[y++] = '\'';
			cmd[y++] = '\\';
			cmd[y++] = '\'';
#endif
			/* fall-through */
		default:
			cmd[y++] = str[x];
		}
	}
#ifdef PHP_WIN32
	cmd[y++] = '"';
#else
	cmd[y++] = '\'';
#endif
	cmd[y] = '\0';

	if ((estimate - y) > 4096) {
		/* realloc if the estimate was way overill
		 * Arbitrary cutoff point of 4096 */
		cmd = erealloc(cmd, y + 1);
	}
	return cmd;
}
/* }}} */

It is worth noting that on a Debian 6.0.6 system the C.UTF-8 locale is not provided by default, but can be generated using the following command.

sudo localedef -c -i POSIX -f UTF-8 C.UTF-8

This may be of issue on other systems, and may require picking a locale that is available e.g. en_GB.UTF-8.

Updated issue summary.

It has been 7 years since there has been discussion here.

Is this still relevant to Drupal 10?

Since we need more information to move forward with this issue, I am setting the status to Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

AFAIK this is still a valid issue as originally described. I created a merge request with the setlocale() change to see if tests pass. The test from the patch would still need to be ported, however.

Setting to needs work as the test still needs to be ported from patch to merge request.

I added a test so this is ready for review.

Since this issue was created, a C.UTF-8 locale was officially added so should be even more common across different environments.

However, I changed the setlocale() to use good ol' C as a fallback for older systems: setlocale(LC_ALL, 'C.UTF-8', 'C');

As a result there will be some behavioral differences across environments, but it seems like a net win to enable correct UTF-8 handling on newer systems, while still supporting older systems.

Hiding obsolete patch files

Got the issue summary started but seems original summary was written for D7 if that could be cleaned up some.

Wonder if this kind of change will require a CR or be disruptive?

Clearly an old issue so will keep on my radar

Updated the summary. I think this issue makes more sense now than when originally proposed, as C.UTF-8 has been much more widely standardized and adopted than it was back then. However, should still be thoroughly tested.

Doubt we are backporting to D6 or D7

Applied locally and did random things like create content, media, views, etc. But didn't see any issues. Not sure how else to test.

@smustgrave were you able to run the newly-added test on macOS - that would be the main thing worth testing.

It's a ddev setup on my mac but ran DrupalKernelTest without issue if that's what you mean.

Ok, but that might essentially be Linux if docker-based? Would be excellent if someone could test directly on raw macOS

We definitely should have a change record for this. I can run Kernel tests fine on a Mac.

Change record ready for review.

Since it was just a CR was needed going to put back into RTBC.

@smustgrave, The change record should be reviewed before setting to RTBC. Has that been done?

I reviewed the CR and the code, I did tweak the CR a little for clarity. I think this looks good!

I'll let @smustgrave respond to your question, but I've independently reviewed it so RTBC should be good either way.

Committed and pushed 79eed4acc5 to 11.x and dcfd0488b8 to 10.3.x. Thanks!

Also published the change record.