Updated: Comment #N

Problem/Motivation

\Drupal\Tests\Component\Utility\UnicodeTest uses Windows-1250 strings to test \Drupal\Component\Utility\Unicode. However, mbstring does not support this character encoding and fails.

Steps to reproduce
  1. Clone/pull the most recent Drupal core.
  2. Install Drupal on a host with mbstring, but not iconv().
  3. Run PHPUnit using core's configuration and bootstrap files. There will be three test failures in \Drupal\Tests\Component\Utility\UnicodeTest::testConvertToUtf8()

Proposed resolution

Use another encoding, such as Windows-1252.

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#15 drupal_2149827_15.patch752 bytesXano
#8 drupal_2149827_8.patch894 bytesXano
#7 drupal_2149827_7.patch891 bytesXano
#4 2149827_4.patch1.36 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

It works fine for me.

Maybe you could paste the full failure.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

The three full failures.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

@Xano: I'd wager you don't have iconv, mbstring, or recode installed on your PHP.

Unicode::convertToUtf8() returns FALSE in that case, and the unit tests don't account for that.

Yay! You found the edge case! :-)

This needs to happen: http://phpunit.de/manual/3.7/en/incomplete-and-skipped-tests.html#incomp...

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Active » Needs review
FileSize
2149827_4.patch1.36 KB

A patch to solve the error, but not the testing problem.

This only checks if the conversion functions exist or not, and checks for FALSE if they don't.

The method we're checking here is this one: http://drupalcode.org/project/drupal.git/blob/HEAD:/core/lib/Drupal/Comp...

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

I wonder if this is a faulty test, or a missing requirement for Drupal core.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

It's both. :-) In this case, however, Drupal handles the case where the requirements aren't met, so we have to test that outcome.

That's why I just patched it so it would pass, with a @todo. It leads to an interesting question: Should unit tests fail if the requirements aren't met?

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
FileSize
drupal_2149827_7.patch891 bytes

What about this approach? We test Unicode::check() and make sure that passes before we test anything that depends on it.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
FileSize
drupal_2149827_8.patch894 bytes

The assert in the previous patch was faulty. This actually works on my system that (apparently) doesn't have any PHP Unicode extension.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

Well, that's kind of the question: Am I right in my assumptions? :-)

Make a phpinfo() page and find out if you have mbstring, iconv, and/or recode.

You can also do this as php -i at the command line and look to see if they're in your system.

Status: Needs review » Needs work

The last submitted patch, 8: drupal_2149827_8.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review

8: drupal_2149827_8.patch queued for re-testing.

I do have mbstring locally, and so do the testbots, so this shouldn't have failed at all.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

Err, never mind that last patch. The patch from #7 is good.

Status: Needs review » Needs work

The last submitted patch, 8: drupal_2149827_8.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

mb_convert_encoding() says Windows-1250 is an illegal character encoding.

Edit: mb_list_encodings() only returns Windows-1251, Windows-1252, and Windows-1254 as Windows encodings.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
drupal_2149827_15.patch752 bytes

This works locally. However, there is no way of knowing which encodings iconv() supports as that depends on its internal library, and I haven't been able to find out which encodings are supported by recode_string().

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Title: PHPUnit fails in \Drupal\Tests\Component\Utility\UnicodeTest » Windows-1250 character encoding not supported by PHP's mbstring extension
Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs review » Reviewed & tested by the community

I'm Mile23, and I approve this patch.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.