This issue is a spin-off of #539110: TF #4: Translatable fields UI. If the translatable fields UI does not get committed it will live in contrib, but we absolutely need to complete the multilingual support for fields in core.
We need:
- integration between node multilingual support provided by locale and the multilingual capabilities of fields
- body translatable
- language negotiation support
- language fallback support
All this requirements are deeply interconnected and are difficult to treat in separate patches without breaking the tests so the following patch will provide a unique, coherent, solution. If don't solve the issues above translatable fields will be almost unusable.
Comment | File | Size | Author |
---|---|---|---|
#49 | multilingual_fields-565480-49.patch | 3.84 KB | plach |
#44 | multilingual_fields-565480-44.patch | 30.13 KB | plach |
#40 | multilingual_fields-565480-39.patch | 28.55 KB | plach |
#37 | multilingual_fields-565480-37.patch | 27.82 KB | plach |
#35 | multilingual_fields-565480-35.patch | 27.47 KB | plach |
Comments
Comment #1
plachThis is should be almost ok, but tests are not fixed.
Let's see what the test bot has to say.
Comment #3
peximo CreditAttribution: peximo commentedThe attached patch should fix the tests. It also introduces a new test case to cover the multilingual fields new code.
Comment #5
plachThere was a missing hunk in the previous patch.
Comment #6
sunsubscribing
Comment #7
plachThe current patch suppress
field_multilingual_get_items
as its behavior was deemed prone to failures in #557292-44: TF #3: Convert node title to fields.Comment #9
plachmmh, rerolled with eclipse
Comment #11
peximo CreditAttribution: peximo commentedRe-rolled
Comment #13
peximo CreditAttribution: peximo commentedRerolled with eclipse
Comment #14
peximo CreditAttribution: peximo commentedComment #16
plachRerolled. Now this depends on #590590: field_create_field ignores the 'translatable' property to work correctly.
Comment #18
plachComment #20
plachrerolled
Comment #21
plachwrong patch
Comment #22
bjaspan CreditAttribution: bjaspan commentedThis patch contains a fairly small change to modules/field/field.multilingual.inc and a much larger set of changes outside of module/field. Might you get more/better reviews if you re-categorize the issue?
Comment #23
plach@bjaspan: well TF is a pretty mixed matter, and most of the changes on locale are field related.
Let's see if we have better luck this way.
Comment #25
plachRerolled. Actually it's CNW.
Comment #27
webchickJust subscribing for now so I don't lose track of this.
Comment #28
plachIf the bot does not complaints we sholud be ready for review.
Comment #30
plachmmh, I have no problem to apply the patch, let's retry...
Comment #32
plachwow, let's see if tests are fixed
Comment #34
plachThis one should fix the tests.
"Help with rolling this patch into RTBC as quickly as possible would be highly appreciated."
Comment #35
plachthis one...
Comment #36
sunThis looks very good already.
Should we statically cache this?
Summary should not wrap at 80 chars.
We should invoke a hook here instead and move the cache flushing into field module instead.
We can use a single/unique hook name, like hook_language_settings_alter(), for example.
I'm not sure whether we shouldn't keep the old key name... "translation" on its own is not kinda descriptive..
We can go with "translation" though, in case we don't have a better idea now.
s/give/given/
s/else if/elseif/
1) The description of @return is a bit lacking.
2) Normally, we pass a fully loaded $type object instead of $type->type. So we should do this here as well.
3) Minor: Duplicate blank line after the function.
"Form submit handler for node_form()."
Should also have a PHPDoc description that explains what's happening here.
Is this concept of $recursion already explained elsewhere or a common pattern in field API as of now? If not, then we should squeeze some inline comments in here.
Like above, Form submit handler for node_form().
The second sentence should be moved into the PHPDoc description.
This review is powered by Dreditor.
Comment #37
plachThanks sun! Suggestions from #36 are implemented, except:
I ain't sure about this: conceptually it's right, but I ain't sure we usually use hooks in these situations.
I introduced this change while working at #539110: TF #4: Translatable fields UI: the main reason is I stored a complex data structure in it, which holds generic data used for the entity translation.
Comment #38
sunWill do a more in-depth re-review later, and just want to assure that we need to do the hook to provide other modules a single entry point for flushing any language-relevant caches. So Field module is not really the measure here, but we still want to move the clearing of caches into a hook implementation of Field module, just like do in http://api.drupal.org/api/function/field_modules_installed/7 already. Following this pattern ensures a sane API, other modules can plug into.
Comment #39
catchI wasn't subscribed to this :(
While the hook sounds like a good idea, we currently don't have that pattern in core, so I don't think it's essential for a commit here.
Comment #40
plachlet webchick choose :)
Comment #41
sunThis looks good from a code perspective now. I've to admit that I didn't test it (live), but if the code looks right, then there is little chance that things go wrong.
Comment #43
alexanderpas CreditAttribution: alexanderpas commenteddrual_static()?
we might make it more clear, we're getting a boolean from here.
I'm on crack. Are you, too?
Comment #44
plachrerolled
@alexanderpas: thanks for reviewing!
I didn't use it because we'll never need to reset the variable value during a single page request. Moreover it would invalidate the (small) benefit of avoiding a function call.
I don't get what you're suggesting: an inline comment?
Comment #45
webchickHere's a code review of this patch. All of my questions were answered on IRC. This is needs work for docs; I can't really find anything else to complain about, although I confess I don't 100% follow everything that's going on here.
First question is why aren't node titles made translatable as well? Answer is because it's being handled over here: #571654: Revert node titles as fields
Why would locale exist if we weren't a multilingual site?
A: If people are translating English into something slightly different.
Need to document hook_language_get_fallback_candidates_alter(). Especially since there doesn't seem to be a hook_language_get_fallback_candidates(). Would this be more appropriately called hook_language_list()?
A: No, because we're altering an ordered language list which also contains FIELD_LANGUAGE_NONE.
These docs need vast improvement. There needs to be answers to these questions:
1. How do I know when I should implement this hook?
2. How do I know when I should /invoke/ this hook? (since there are at least 3 places in this one patch that this happens)
3. What sort of stuff should I be doing in here?
Q: Is there a chance of this function being called multiple times on the same page? Should we add a $reset param/drupal_static?
A: No, because it's reset down at the bottom of the function.
Please add a line of PHPDoc here.
Please add a line of PHPDoc here.
This review is powered by Dreditor.
Comment #46
webchickComment #47
webchickSpoke with sun on IRC. He said that despite him not being able to give this a more thorough review:
And, well, that's good enough for me. ;)
Since the only thing left-over is docs, and docs are *not* bound by code freeze, *and* it's like bazillion o'clock plach's time, I went ahead and committed this to HEAD. plach promised me there would be a follow-up here tomorrow with the missing documentation. :)
Comment #48
yched CreditAttribution: yched commentedThis is largely over my head now, and I'm not where I can spend quality time with a patch anyway, but congratulations, folks !
Minor : could we use field_info_cache_clear() instead of the various direct cache_clear_all('field_info_types', 'cache_field'); calls ?
Comment #49
plach@webchick: here are the PHP docs I owed you ;)
The patch also makes two one-line changes: it implements yched's suggestion from #48 (thanks yched! have fun :) and changes
hook_language_fallback_get_candidates_alter
intohook_language_fallback_candidates_alter
.Comment #50
webchickExcellent. Thanks, plach! :)
Committed follow-up to HEAD.