Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Follow-up to #922824: No way to specify the language to view entities in.
Somehow prepare view hooks did not get the $langcode
parameter, hence while performing entity view every hook is able to tell which language use except prepare view ones.
Comment | File | Size | Author |
---|---|---|---|
#15 | language-1089174-15.patch | 5.21 KB | plach |
#15 | language-1089174-15-test.patch | 4.36 KB | plach |
#18 | language-1089174-18.patch | 6.27 KB | plach |
#18 | language-1089174-18-test.patch | 5.48 KB | plach |
#4 | language-1089174-4.patch | 9.9 KB | plach |
Comments
Comment #1
plachSpoke with yched: he agrees we need
$langcode
also infield_attach_prepare_view()
.Comment #2
plachComment #3
yched CreditAttribution: yched commentedDefinite overlook from #922824: No way to specify the language to view entities in, agreed that this need to be fixed.
I'm not 100% sure the entity_prepare_view() func (and associated hook_entity_prepare_view() hook) need the $langcode param too. We might want to check with @catch.
Minor : can you move that $null declaration just above the _field_invoke_multiple() call, like in field_attach_view() ? It feels off standing on its own like that.
How come we are fixing this now ? Isn't that a separate bugfix ?
Ouch. Adding a $langcode param to node_preview() seems like a sweeping API change, which sounds like it needs some discussion of its own.
Besides, node_preview() seems odd : the function explicitly calls f_a_prepare_view(), but defers the corresponding f_a_view() call to :
theme('node_preview') -> node_view() (twice, 'full' and 'teaser') -> node_build_content().
node_build_content() does call both f_a_prepare_view() (*again*) and f_a_view(). So it looks like the first f_a_prepare_view() call in node_preview() is not needed.
And, btw, theme('node_preview') has no way to transfer a $langcode, which means that the current f_a_view() call provides no $langcode either (NULL), so it seems pointless to add a $langcode to node_preview() to begin with ?
Powered by Dreditor.
Comment #4
plachPartially addressed #3:
Well, I'm pretty sure we want it there either: I discovered this bug while working on Title because I was missing the
$langcode
parameter. However I dropped a line to @catch, hopefully he will chime in.Also this one is an overlook from the previous issue and since
hook_node_view()
is taking a$langcode
parameter I'd say the fix is ok. Just tell me if you actually prefer a separate issue, but IMO everything is here should already have been committed in the previous issue.Makes sense, reverted.
However, test results are not coming back (again) but the previous patch failed them badly. The cuplrit is the
f_a_prepare_view
hunk, which did not take into account the fact that multiple entities are passed in. This forced me to introduce an API change in_field_invoke_multiple
, which probably we would need anyway since the previous behavior was flawed:We need to have a field language suggestion for each entity, having a shared one simply makes no sense as field translations available in one entity might not be available in another one. This apparently looks scary but it is a backward compatible API change since language codes cannot be numeric: if
_field_invoke_multiple
is being called in some custom code using a single array of field language suggestions (unlikely) this will simply keep working as the$language
variable will be assigned$options['language']
. Moreover the only other place in core using this isf_a_load()
which does not use language suggestions as it needs to act on all the available languages.Comment #5
plachNote that this should not be a performance regression as
field_language
is static cached and is called for every entity anyway inf_a_view
(actually tested this).Edit: we should have a performace gain on multilingual sites instead, since prepare view hooks are run only on a single field translation instead that on all the available ones.
Comment #6
plach@yched:
Any pending concern?
Comment #7
plachBugs are fixed in the development version first, backported then.
The patch still applies.
Comment #8
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedSubscribing
Comment #9
sunThis patch looks ready for me. Quite an oversight in the API. Should definitely be part of 7.1.
Comment #10
yched CreditAttribution: yched commentedTruly sorry for letting this drop off my radar. The patch looks good to me too.
Comment #11
Dries CreditAttribution: Dries commentedCommitted to 8.x.
We'll want to backport this to 7.x as this is a big oversight. Moving to 7.x for @webchick.
Comment #12
webchickAgreed. This is an extra param, so shouldn't break anything.
Committed to 7.x. Thanks!
Comment #13
sunUnfortunately, this patch seems to cause PHP notices on existing sites:
Comment #14
plach@sun:
I actually tested that change while rolling the patch and never got those notices: are you able to describe how to reproduce them?
Comment #15
plachOk, I managed to reproduce notices. Pretty subtle: they appear only when viewing more than one entity, for each one at least one field implementing
hook_field_prepare_view()
is populated and field values are populated for different languages for different entities. Long story short:I didn't see notices before because I tested per-entity language suggestions without populating image fields, which implement
hook_field_prepare_view()
.Note that this is not exactly a regression, simply the correct behavior of per-entity language suggestions is now revealing another bug, which previously was unnoticeable. The attached patch should fix it and extend tests to capture it.
Comment #16
plach#15: language-1089174-15-test.patch queued for re-testing.
Comment #18
plachThe previous tests did not capture the bug reliably due to some randomness in language weights. The attached ones should always do.
Comment #20
plachcool!
Comment #21
xatoo CreditAttribution: xatoo commentedwhoops, wrong issue. Luckily a re-test won't hurt.
Comment #23
plachComment #24
webchickNoting as a release blocker so we don't ship 7.1 with notices.
Comment #25
webchickWell, wait though. Doesn't this need to be in 8.x first?
Comment #26
plachsure, patches have been rolled against 8.x actually
Comment #27
sunApplied the patch on the (otherwise untouched) site throwing notices and they disappeared.
The added condition looks correct to me. Didn't have a close look at the tests, but I trust @plach to get them right, and the additional test-only patch is excellent proof for me that this patch fixes the bug.
Comment #28
Dries CreditAttribution: Dries commentedCommitted the patch in #18 to 8.x. Moving back to 7.x for @webchick.
Comment #29
droplet CreditAttribution: droplet commentedacross from #1164240: Notice: Undefined offset: 188 in _field_invoke_multiple() (line 351 of /home/.../field.attach.inc)
can't it using empty() ??
Comment #30
plachThey are not equivalent: we need to ensure that only populated values for existing translations are actually set back into the entity.
Comment #31
webchickAwesome! Thanks for the expanded tests to prove that this newly identified bug stays fixed!
Committed to 7.x.
This all looked good, except for this:
Ew. Why this level of indirection? It's certainly not to save keystrokes; $null is longer than NULL. :) Can we get a quick fix in for that, or am I missing some context?
Powered by Dreditor.
Comment #32
plachThe parameters getting $null are expected to be a variable reference, passing NULL will cause PHP to complain.
Anyway, thanks!
Comment #33
plachComment #34
yched CreditAttribution: yched commentedAbout $null : yup, that is a not-too-beautiful pattern already present in other spots where _field_invoke() / _field_invoke_multiple() are called.
Comment #35
plach@yched:
Are there possible alternative approaches?
Comment #36
yched CreditAttribution: yched commented@plach : I tried but could not find any back when I was writing those $null vars :-(. Generic field ops invoker...
Comment #37
webchickOh, fascinating. Stupid PHP. :P Thanks!
Comment #38
yched CreditAttribution: yched commentedCould this be linked to #1169426: Field invoke multiple does not handle field language correctly ?
Comment #39
plach@yched:
Yes, unfortunately it seems that only
file_field_prepare_view()
triggers those notices, whileimage_field_prepare_view()
doesn't, and I tested everything with images.Really sorry about this :(
Comment #40
plach#1169426-7: Field invoke multiple does not handle field language correctly
sigh
Comment #41
fietserwinCan #1164230: Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() line 1429 of taxonomy.module be linked to this change or to the mentioned #1169426: Field invoke multiple does not handle field language correctly? As others, I do get the warning "Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() line 1429 of taxonomy.module" since my upgrade to 7.2. I tried debugging it, and came to the following ideas:
- Entities listed on the front page (or probably better: on any list that shows a set of teasers) that have none of their taxonomy fields filled in have a value of NULL in the $items array passed to function taxonomy_field_formatter_prepare_view().
- This NULL probably originates from the fact that the language suggestion results in 'und' instead of the language of the node.
- I traced changes in this part between 7.0 and 7.2 back to a commit related to this issue (http://drupalcode.org/project/drupal.git/commit/2a419f61aafc5ad490eef0be...)
What happens goes just to deep for me to follow, so I don't dare to suggest a patch for #1164230: Invalid argument supplied for foreach() in taxonomy_field_formatter_prepare_view() line 1429 of taxonomy.module, but will be cross linking all issues.
Comment #43
plach@yched: @sun:
I think we have a RTBC candidate for #38 on #1169426-22: Field invoke multiple does not handle field language correctly (which has become a release blocker), a review would be welcome.
Comment #44
henwu CreditAttribution: henwu commentedSubscribing
Comment #45
andypostJust linking #1178500: hook_field_*() prepare/view have no access to the requested language