This repository was archived by the owner on Oct 29, 2020. It is now read-only.
Translated facts bug fix olé#5660
Merged
weerd merged 5 commits intoDoSomethingArchive:globalfrom Oct 29, 2015
weerd:translated-facts-bug-fix-olé
Hidden character warning
The head ref may contain hidden characters: "translated-facts-bug-fix-ol\u00e9"
Merged
Translated facts bug fix olé#5660weerd merged 5 commits intoDoSomethingArchive:globalfrom weerd:translated-facts-bug-fix-olé
weerd merged 5 commits intoDoSomethingArchive:globalfrom
weerd:translated-facts-bug-fix-olé
Conversation
added 5 commits
October 27, 2015 17:01
The function body that executes to conver the country to the appropriate language code requires the country to be in uppercase, so wrapped it in php strtoupper() so it always uppercases it and doesn't break or give false result because a lowercase country string was passed.
To minimize the potential for breaking lots of campaign related code, took a more subtle approach to fixing the translation bug related to Facts. The EntityDrupalWrapper class was causing problems with grabbing the correct facts related to regional content. Instead, created a separate function to handle grabbing that data from the node, without interrupting how the rest of the campaign node data is loaded when constructing the campaign object. It allows for grabbing content if there are translations and ones specific to the language defined by path (/us/ or /mx/ etc) or just grabbing the available content if no language specific options available, but there is definitely content in the field.
Contributor
There was a problem hiding this comment.
does global $language_content work?
https://api.drupal.org/api/drupal/developer%21globals.php/global/language_content/7
Contributor
There was a problem hiding this comment.
oh, also check out the helper function that @sergii-tkachenko just wrote in #5661
Contributor
Author
There was a problem hiding this comment.
Is global $language_content based on just the path prefix or does it take into account user preference, headers, etc?
I can take a look and see what it outputs in various url path scenarios...
Contributor
|
just some minor comments, but not needed for merge 👍 🎉 💃 |
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Fixes #5572
What's this PR do?
To minimize the potential for breaking lots of campaign related code, took a more subtle approach to fixing the translation bug related to Facts. The
EntityDrupalWrapperclass was causing problems with grabbing the correct facts related to regional content (translated, but as separate unique nodes and not nodes with translations as part of the node object).Instead, created a separate function to handle grabbing that data from the node, without interrupting how the rest of the campaign node data is loaded when constructing the campaign object. It allows for grabbing content if there are translations and ones specific to the language defined by the url path (
/us/or/mx/etc) or just grabbing the available content if no language specific options available, but there is definitely content in the field.Where should the reviewer start?
Probably in the dosomething_campaign.helpers.inc file at line 183 with the
@TODOexplanation and then follow the bread crumb trail of code... 🍞How should this be manually tested?
We'll test on Thor which has a bunch of language related content to make testing easier.
Any background context you want to provide?
To help with mitigating bugs and not break how the
dosomething_campaign_load()function loads and creates thecampaignstandard object, the aim with the custom function was to set it up in a way that it would return data back to thedosomething_campaign_load()function in the same format/structure that theEntityDrupalWrapperwould have, because that's the structure the rest of the code is relying on. Deviating from that structure could end up causing more bugs that we'd need thorough testing to actually find and then iron out. So went with the path of least resistance; but also why some of the code may see a little odd in places, in the effort to accommodate the above intentions! 🌎What are the relevant tickets?
#5572
Screenshots (if appropriate)
US specific content at
/us/node/1283:MX specific content at
/mx/node/1283:MX specific content, but none entered in the admin
/mx/node/1321:@angaither
cc: @mikefantini @namimody