Mark Accent String Tests as incomplete if on a database that is not utf8 #12060
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
If this passes Jenkins I'm happy- no impact outside the test |
|
I'm not convinced. Isn't this a problem with the database creation? |
totten
added a commit
to totten/civicrm-core
that referenced
this pull request
May 1, 2018
…e of accents This is a follow-up to civicrm#12060. I'm submitting it for discussion -- but I'm not actually confident in the change, so please don't merge unless you affirmatively agree. Let me recount the context. PR civicrm#12060 aimed to address an issue where `testInternationalStrings()` failed on some systems (`test-ubu1604-1`) but succeeds on others (`test-ubu1204-5`). The content of `character_set_database` was suggested as a distinguishing characteristic, so civicrm#12060 basically says, "Don't test if we don't expect it to work." That's nice for developers because it reduces the noisiness of the test suite. But it also reduces the impact of the `testInternationalStrings()` -- because a non-working environment is considered passing. Is it legit to say "it's not the devteam's problem if some environments don't work"? Yeah. But then we should finish kicking the can over to the sysadmin so they can fix their system. That's what this patch does. Both 12060 and this patch are premised on the correctness of `supportStorageOfAccents()`. I'm not really confident in it, though. On my local system, `supportStorageOfAccents()` returns `FALSE`, but I'm able to create and view a contact with last name `Iñtërnâtiônàlizætiøn` -- via either web UI or `cv api contact.create`. That casts some doubt on `supportStorageOfAccents()`, but it's not decisive (maybe check is important for some other reason/use-case). In short -- if `supportStorageOfAccents()` is working correctly, then we should merge this and notify site-admins. If `supportStorageOfAccents()` is not working correctly, then we probably need something else to fix civicrm#12060.
totten
reviewed
May 1, 2018
| * @return bool | ||
| */ | ||
| public static function supportStorageOfAccents() { | ||
| $charSetDB = CRM_Core_DAO::executeQuery("SHOW VARIABLES LIKE 'character_set_database'")->fetchAll(); |
There was a problem hiding this comment.
I think the intent of the testInternationalStrings() was to raise a flag if one can't store accents. If this condition is really a prerequisite for storing accents, then we should generate a notice inCRM_Utils_Check_Component_Env?
@seamuslee001 @mlutfy @nganivet -- see followup #12063.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Storage of internationalised strings with accents requires utf8 encoding, At present on the ubunu1604 the default is latin1. This is as per https://stackoverflow.com/questions/17798119/write-to-mysql-db-from-bash-in-utf-8. This allows for the tests to still run on the boxes we got where the DB is utf8 but on the latin1 boxes it will just mark it as incomplete rather than completely skipping it.
Before
Tests completely fail on latin1 test boxes
After
Tests are marked as incomplete rather than skipped
ping @eileenmcnaughton @totten @mlutfy
A better solution IMO would be to set the charset to be utf8 on our test boxes and adding a status check to let people know their dbs may not be right.