Through out the core we are seeing many situations (e.g. #571654: Revert node titles as fields) in which one might need to select the language of a field as it would be displayed. At the moment this is possible only by going through a field_attach_view()
call and picking the field built values, which has evident performance drawbacks and might also be just wrong.
We need a way to select the raw items in a $object->$field_name
data structure using the right language. This might be also needed by #612894: field_format() is just a pile of code that doesn't work to provide coherent results.
Comment | File | Size | Author |
---|---|---|---|
#47 | field_fallback-657972-47.patch | 48.12 KB | plach |
#45 | field_fallback-657972-45.patch | 48.57 KB | plach |
#43 | field_fallback-657972-43.patch | 48.29 KB | plach |
#41 | field_fallback-657972-41.patch | 48.44 KB | plach |
#39 | field_fallback-657972-39.patch | 41.8 KB | plach |
Comments
Comment #1
plachThe attached patch introduces a new
field_multilingual_fallback()
function which returns an array of language codes keyed by field name. It can work in single-field mode in which case it returns a single language code.Tests and documentation have been updated accordingly (an undocumented hook has also been added to fied.api.php).
Comment #2
yched CreditAttribution: yched commentedWe need this. Subscribe for now.
Comment #3
sunMost of the code in this patch seems to makes sense. However, I'd really like to see more inline comments that explain (especially for people who do not know this subsystem inside out) what the code is doing and why. The pasted example snippets are just examples.
I'm on crack. Are you, too?
Comment #4
plachImproved and augmented inline comments and PHP docs.
Comment #5
yched CreditAttribution: yched commentedSpent some time with the patch, here's what I came with so far.
Comment is misleading. In the code, I see no logic for the 'unless'. The logic is probably embedded in f_m_available_languages(), but in this case it should not be reflected in a comment inside _field_invoke() (or the comment should make it clear that the logic is done in the helper func)
Also applies to _field_invoke_multiple()
Hm, if I get this patch correctly, f_m_fallback() without a specific field name will return an array of language fallbacks for each field separately, so this doesn't look correct.
Besides, it's a bit strange that we call f_m_fallback() here, since the lang fallback logic will be taken care of in the _field_invoke() calls. AFAICT, this is only needed to populate the $context['language'] element provided to hook_field_attach_view_alter()at the end of the function. But does this info make any sense now ?
Correct me if I'm wrong but it seems that, if anything, we should provide the *originally* requested language. each $output[$field_name] array has a '#language' property that contains the actual language selected for the field.
PHPdoc still has the old name of the param.
The PHPdoc should mention that possibility
"Checks whether a field has language support" ?
According to the new doc standards, PHPdoc descriptions should be active: "Applies fallback rules...". Please fix the PHPdocs added or updated in the patch.
Incorrect english.
The name is cryptic.
This is *the* outside-facing API function, right ? Other functions in field.multilingual.inc are mainly used by the rest of Field API / locale ?
If so, I'd suggest
field_get_language($obj_type, $object, $langcode, $field_name)
More generally, all the function names in field.multilingual.inc are cryptic. I'm really not a fan of the field_multilingual_* namespace, really convoluted. The fact that functions are grouped in a 'multilingual.inc' file is one thing, this doesn't mean we need to use that as a namespace.
Similarly, field_multilingual_available_languages() could be field_available_languages(), for instance.
Could be fixed by a cleaner separation of which functions are internal helper functions and which functions are actual API functions ?
It's not clear that the function is being called by locale's hook_field_multilingual_fallback_alter().
Is it really worth as a separate function ? or could it be merged into locale_field_multilingual_fallback_alter() ?
At least it should be made clearer in the PHPdoc.
Same pattern of 'it's unclear what is an API function and what is a helper function, BTW.
Please add a comment that we're adding a new field in addition to the one in setUp(). The tests could use a couple more comments to provide an outline of what is being done. Easier debugging when some patch triggers a fail in there - and multilinguial code and logic is hairy enough as is ;-)
Do we really need those ? would make the scope of the test clearer if properties that are not strictly required are omitted.
Same goes for the current FieldTranslationsTestCase::setUp(), BTW.
This doesn't really test that the fallback language is the one expected, right ?
Why is this hook implemented in field_test.field.inc while hook_field_langages_alter() is in field_test.module ?
Already present in the code, but 'test_hook_in' is really ugly ;-).
Also, comments could be useful around the uses of this 'setting' in the .test file.
an array_search()/unset() would be cleaner
I'm on crack. Are you, too?
Comment #6
plachI'll implement the suggestions from #5 ASAP, meanwhile:
Probably I didn't make clear the approach I followed. To make language fallback actually reusable outside f_a_view() we need a mapping of the field items in $object to be used: as theorically every field might hold a different translation set (this is already true for translatable vs untranslatable fields), we need a per-field language suggestion. I implemented it as an array of language codes keyed by field name.
In
_field_invoke()
the whole array is needed to select to correct items that will be used to build the renderable array.Perhaps
$options['language']
is misleading, we might want to rename it to$options['language_suggestion']
, but it can hold either a single language code or an array.No, language fallback is a concept strictly tied to visualization: it makes no sense to apply it in contexts like 'insert' or 'update'. This is why I kept it outside
_field_invoke()
.The main advantage of this solution is it doesn't mess with renderable arrays anymore: f_a_view() just tells
_field_invoke()
which items to use to build the renderable array. No language alteration should be performed in hook_f_a_view_alter() implementations, I just forgot to remove$context['language']
:)Well, perhaps we need some discussion about this. I ain't sure it f_m_fallback() should be called directly outside the field API, IMO it would be better to encapsulate it into a function, say
field_get_items()
, actually selecting the raw items.No strong objections on this, but we need to understand if we can still do it and figure out how to, before actually coding.
IMO making any logic reusable, no matter if there is an actual need, can't hurt. In this case we have a function implementing the core field language fallback rules, which is called also in
field_test_field_fallback_alter()
. Moreover storing it in a separate file allows to save memory. Also, if we merged it inlocale_field_multilingual_fallback_alter()
, we would be tied to the translation handler check and the value of the'locale_field_fallback_view'
variable.I don't think PHPdocs should report who calls the documented function, in most cases it would be an incomplete information.
Comment #7
yched CreditAttribution: yched commented- f_a_view() / f_m_fallback(): OK, got confused. I get it now, but I really needed to give a close look at the code in field_invoke() / f_m_available_languages() / f_m_fallback(). I'm sorry to say that the multilingual workflow around field_invoke() and $options['language'] really needs to be documented better. If even I (sorry for the arrogance :-p) get confused about how TF work, then I strongly suspect this will only appear as inextricable black magic to 99% people trying to wrap their heads around TF.
Opened #664136: Enhance TF code documentation for this.
- The PHPdoc for f_m_fallback() needs to be more specific. "determines the actual language to be used for each field given a) the requested language b) the actual data available in the fields c) the current language fallback configuration." ?
+ Even if we cannot hardcode in the PHPdocs the exact places calling the function, saying that *for instance* f_a_view() uses this function to determine the language to display for each field of an object would make it less abstract what the function does.
-
IMO both field_get_language() (== f_m_fallback()) and field_get_items() make sense as API functions. Removing 'multilingual' in a couple existing functions can be done in a separate patch, but let's make sure the functions introduced here have clear, usable names.
- locale_field_fallback() / locale_field_multilingual_fallback_alter():
Memory savings from putting code in .inc files has been deemed not critical in D7. The logical code structure and readability should prevail - esp. for this small amount of code.
If locale_field_fallback() is useful as a reusable API function, then its PHPdoc should be clearer on what it does. It should also not be hidden inside a file that needs to be manually loaded. The PHPdoc for locale_field_fallback() really needs to at least say that this code is active through locale's hook_field_multilingual_fallback_alter(), or we cannot relate it to the rest of the workflow.
- hook_field_multilingual_fallback_alter() is actually not an alter hook, we don't build anything that modules can then alter. The data *is* what is returned by the hook implemantations. Having the hook named _alter makes you think there's some basic, canonical logic coded somewhere, and thus gives a misleading view on the actual mechanism.
Can't this check be made within the f_m_fallback() call ?
Not too accurate. I'd suggest
"Apply language fallback rules to determine the actual language to display for each field given the available languages in the field data."
It should also be clearer that $fallback is an array. Maybe $fallbacks ?
Needs a comment. When does this happen and why is this logic the right one ?
Similarly, can't this be included within f_m_fallback() ? (Maybe not, just asking).
This review is powered by Dreditor.
Comment #8
yched CreditAttribution: yched commentedthe revamped field_view_field() function in #612894: field_format() is just a pile of code that doesn't work got in, so we should now also make use of the new fallback mechanism over there.
Comment #9
plachOk, here is an attempt to improve readability, documentation and implement the suggestions from #5, #7, #8.
Brief summary:
field_multilingual_fallback()
is nowfield_display_language()
field_display_language()
actually initialize its returned values and then let hook implementations alter themf_m_available_languages()
does not handle suggestions anymore, I introduced a helper function named_field_language_suggestion()
to be able to reuse the code in_field_invoke()
and_field_invoke_multiple()
. I don't really like this solution, though.$options['language']
is now documentedfield_view_field()
has been updatedfield_get_items()
has been introducedComment #10
Dries CreditAttribution: Dries commentedI started looking at this patch but got interrupted. I'm not entirely sure why this patch is 'critical' -- it isn't a regression is it? In fact, it is still unclear what exactly this patch does.
For example, I spend the first 5 minutes trying to figure out the difference between 'available languages for a field' and 'display languages for a field'. It is not clear why we need both sets.
Anyway, the patch isn't obvious, so I'll need to invest more time in trying to understand it. Have to run now -- maybe we can make it more intuitive or simpler in the mean time.
The verb is not correct and does not match the noun.
The code doesn't explain how the suggested language is selected from the available languages.
This review is powered by Dreditor.
Comment #11
yched CreditAttribution: yched commented@Dries: the context of this patch is "I'm asked to display a node in spanish. field_a has values in spanish, but field_b only has values in english and french. What should I display for field_b ?".
This is determined by language fallback logic. Currently, the Field / TF API does this in two steps:
- let field_attach_view() build render arrays for values in spanish if they exist.
- in hook_field_attach_view_alter(), locale computes the fallback language according to its logic and current settings, and then computes and switches in the render array for the fallback language.
issues:
- convoluted flow
- more importantly, the language fallback logic is blackboxed as magic stuff during field_attach_view() workflow. Short of actually building the render array, there's currently no way to answer the simple question 'Given the requested language and current site settings, what language should be displayed for this field'. This ability is critical. Raised lots of foobar in #571654: Revert node titles as fields where there were attempts to build / render a fully themed 'field' just to access $node->title[$the_right_language].
This patch externalizes the language fallback logic to a public API function (field_display_language() in the last iteration), that field_attach_view() uses on the various fields.
Comment #12
plachImplemented suggestions from #10 and moved
_field_language_suggestion()
in field.multilingual.inc.@Dries:
IMO to make the behavior implemented by this patch understandable, we just need to make the above distinction extra-clear (I'll provide a thorough documentation in #664136: Enhance TF code documentation): all the very core of translatable fields is related to it.
Almost all
field_attach_X()
functions act on every available language. For each field available languages is the set of languages that can be assigned to the field data.LANGUAGE_NONE
.LANGUAGE_NONE
. This default can be altered by contrib modules.However there are two relevant exceptions which instead act in single-language mode:
field_attach_form()
just needs a single language code to specify in which language the field values will be submitted.field_attach_view()
needs a desired/requested language which is the language one wish to display the object in; however we can't be sure that a translation in the requested language is available for each field. We have two ways to deal with this: 1) we ignore missing translations, 2) we provide an alternative value in a different language (i.e. language fallback). The core behavior implemented here is inspecting the field data available in the object to be displayed and determining for each field a language for which a translation actually exists; this way we can display a value for each field.For each field the display language is the most fitting language code, according to the current language fallback configuration, for which field data is available. It can be equal to the desired/requested language, but it will differ every time field data for the requested language is not available.
Core language fallback rules inspect all the enabled languages ordered by their weight, but this behavior can be altered or disabled, thus letting one choose the approach #1.
Comment #16
yched CreditAttribution: yched commentedbot #fail
Comment #17
plachThe patch is trying to remove locale.field.inc. Probably this is messing up the bot. This one simply empties it. We'll need a manual deletion, perhaps.
Comment #18
plachRerolled
Comment #21
plachRerolled
Comment #22
plach#21: field_fallback-657972-19.patch queued for re-testing.
Comment #23
plachHere is a rerolled patch.
I also performed some benchmarks to quantify the performance gain the new approach is supposed to introduce. While performing the benchmark I discovered a critical bug in the HEAD which prevents the language fallback to be performed.
The tests were performed with a patched version of the HEAD to fix the bug above (see #710526: Improve multilingual field test coverage for details); the patch should have absolutely no performance impact, so the results should be quite realistic.
HEAD (10 nodes on front page, 10 fields for each node, locale not installed, ab -c1 -n100):
Patch (10 nodes on front page, 10 fields for each node, locale not installed, ab -c1 -n100):
HEAD (10 nodes on front page, 10 fields for each node, locale installed - 2 languages, ab -c1 -n100):
Patch (10 nodes on front page, 10 fields for each node, locale installed - 2 languages, ab -c1 -n100):
The numbers above show an average performance gain of more than 3% with the patch applied, and the performance gain should be O(n), where n is the number of fields attached to a node. Not mentioning the big gain on the API side.
Comment #24
plachThe rerolled patch...
Comment #25
plachOops, wrong patch.
Comment #26
plachComment #27
Berdir#25: field_fallback-657972-25.patch queued for re-testing.
Comment #29
BerdirOk, this was ugly to re-roll. All those object/entity renames caused failures all over the place. Hope I didn't miss anything and the apidocs probably don't follow the new standards.
Anyway, why I've found this issue..
In current HEAD, when viewing drupal in specific language (german in my case), field.module did not load fields that were marked as translatable but had only a LANGUAGE_NONE entry (aka: body), because it only tried to load the currently active language.
Beneath many other things (I assume, I have no clue about what the patch actually does), it does fix this. The body does now show up correctly.
Of course, I only found this after debugging my "hidden body" issue for hours and creating an ugly workaround for it.
Comment #31
BerdirRenamed too much :)
Comment #32
plachThe reroll looks good at a first look, I'll give it a more thorough review later.
The problem reported by Berdir is caused by #710526: Improve multilingual field test coverage.
Comment #33
plachPatch did not apply anymore.
Comment #34
plachincomplete patch...
Comment #36
plach#34: field_fallback-657972-34.patch queued for re-testing.
Cannot reproduce any error on my box.
Comment #37
yched CreditAttribution: yched commentedI have to admit this dropped off my radar, sorry. Will try to get back asap.
Comment #38
plachFYI: catch asked in IRC to rerun benchmarks with option -n1000.
Comment #39
plachPatch updated after #612894: field_format() is just a pile of code that doesn't work commit.
Comment #40
sunAt least the parameter names don't match.
Isn't this 'entity_type' now?
I didn't look up where this is invoked, but is it a wise idea to return FALSE? Why not an empty array?
Odd indentation here.
Hm - could we rename this to field_is_multilingual() or field_is_translatable() ?
Ditto, perhaps field_has_multilingual_handler() or similar?
148 critical left. Go review some!
Comment #41
plachThis one takes care of #40. It also suppresses almost entirely the
field_multilingual_*
namespace as yched asked in #5. Not sure this can still happen, as this theorically qualifies as an API change not justified by a critical reason, but actually I think (and hope) no one is using the Field translation API yet, so this shouldn't be that dangerous. However, if webchick doesn't give her ok it should be fairly easy to revert the function names changes.@sun:
field_get_items
is an API function not used anywhere in core, but talking with yched we agreed that it might turn useful. I don't think returning an empty array is the right choice because an empty array already represents an empty field value; a non-available translation simply does not appear in the$entity->{$field_name}
array, so I'd choose between FALSE and NULL.Comment #42
sunwow, just wow - this patch is lovely!
oh YES. Pretty please! Those "multilingual" prefixes were totally wonky.
I can't help, but this looks much much easier and grokable now :)
Minor: "Optional." should be "(optional)". (also elsewhere in this patch)
The second sentence can be shortened to "Defaults to the current language."
I wonder why this is called field_display_language() and not just simply field_language() ?
AFAICS, there is only one language "type" for fields, and the other functions are just for getting available, and validating languages.
aha, so now I realize that aforementioned shortening would lead to two almost identical hooks (singular/plural). Can we cope with that?
148 critical left. Go review some!
Comment #43
plachFixed #42: renamed
field_display_language()
tofield_language()
andhook_field_languages_alter()
tohook_field_available_languages_alter()
.Comment #44
sun"(optional)", all lower-case. See http://drupal.org/node/1354 for example snippets and detailed info.
Also: Not completely visible in this paste, all of the descriptions should continue without "If...", e.g.:
"(optional) The name of foo. Defaults to bar."
Is the static caching correct?
$display_languages[$entity_type][$id][$langcode]...
- is keyed by $entity_type, no $bundle_name, so the language for a field that appears in multiple bundles of the same entity is always the same...?
- is not keyed by revision...?
Not sure whether the static caching is a good idea. To me, this looks like we'd better avoid it for now and only introduce it if really required for performance.
yay! Nice solution :)
Shouldn't this be named locale_field_language_fallback() or would that be hook name clash?
148 critical left. Go review some!
Comment #45
plachWell, there is also the entity id as key, so AFAICT there should be no possibility that two distinct entities belonging to the same entity type accidentally share the same cache. All entity revisions will share the same translations set (not the same translation values!) per #629252: field_attach_form() should make available all field translations on submit, so a language valid for one revision should be valid for all revisions.
I don't know what performance gain actually it gives, but I can tell you it saves lots of hook invocations.
Comment #46
sunWhile being here, we can drop the superfluous isset(), the !empty() is sufficient. (!empty() == isset() && != FALSE)
1) The additional static cache here really doesn't seem to be necessary. $entity_info is cached statically already and this is merely iterating over the array.
2) Would it make sense to return an array of non-empty registered handlers instead of TRUE? This would equally equal TRUE when being tested elsewhere. (Just an idea, feel free to ignore)
Powered by Dreditor.
Comment #47
plachFixed #46.
Not sure about this: IIRC while coding the TF4 patch I never needed such a list, I just needed to know if a specific translation hanlder was enabled.
Comment #48
sunAwesome, thanks plach!
With this patch, it is much clearer what the common functions are that one should use with regard to field languages.
Comment #49
plachThanks, sun :)
Core committers should be aware that this patch is supposed to delete
modules/locale/locale.field.inc
and move its content intolocale.module
: I ain't sure that simply applying the patch will do the trick,locale.field.inc
might have to be deleted manually.Moreover #710526: Improve multilingual field test coverage should be commited first.
Comment #50
Dries CreditAttribution: Dries commentedSpent 30 minutes looking at this code, and decided to commit it to CVS HEAD. Looks like an important improvement.
Comment #51
plach@Dries:
Thanks for your patience with this issue!
Unfortunately
locale.field.inc
is still there although it's empty, would it be possible to delete it?Comment #52
yched CreditAttribution: yched commentedGreat ! thanks plach and sun for driving this home, sorry for my lack of availability :-/.
Comment #53
sunlocale.field.inc needs to be removed.
Comment #54
Dries CreditAttribution: Dries commentedRemoved locale.field.inc. Thanks.
Comment #56
boby_ui CreditAttribution: boby_ui commentedHi, I can not get it to work, the language is not falling back using entity translation.. and the patch is not working for me..
can someone confirm please, thank you