Currently all the entity view API functions do not allow to specify the language fields are going do be displayed in, thus forcing the page content language to be used. This is a critical limitation that can be easily removed with a simple and harmless API change: simply adding a language parameter defaulting to NULL.

CommentFileSizeAuthor
#14 tf-922824-14.patch15.49 KBplach
#11 tf-922824-11.patch15.48 KBplach
#9 tf-922824-9.patch15.71 KBplach
#7 tf-922824-7.patch9.63 KBplach
#1 tf-922824-1.patch9.42 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Active » Needs review
FileSize
tf-922824-1.patch9.42 KB
marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented
Status: Needs review » Needs work

So this is only the API change, nothing is really done with the $langcode yet, right?

The @param $langcode documentation does not specify what kind of value $langcode should be. I assume this must be equal to $language->language in the language object, but can you make the documentation clearer?

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Priority: Critical » Major

This is very important, but not critical. Downgrading to major (and subscribing).

steinmb’s picture

steinmb CreditAttribution: steinmb commented
Priority: Major » Critical

Subsribing. If we have to do API changes is not this a betablocker an perhaps also critical?

edit: sorry did not mean to change the issue status back to critical

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Neither.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Priority: Critical » Major

@marcvangend:

So this is only the API change, nothing is really done with the $langcode yet, right?

Yes, in core the view API functions don't need to specify the language as they are used only in page contexts. But, for instance, Simplenews would need to specify the language a newsletter is being sent in.

The @param $langcode documentation does not specify what kind of value $langcode should be. I assume this must be equal to $language->language in the language object, but can you make the documentation clearer?

Yes, the parameter is a language code string. Everywhere in core we use a $language parameter to indicate a language object and $langcode to indicate a string holding a language code. I copied the parameter docs from Field API's ones, so if we change these we need to sync the documentation elsewhere (in a followup issue).

@Damien Tournoud, @steinmb:

This is very important, but not critical. Downgrading to major

If we have to do API changes is not this a betablocker an perhaps also critical?

Honestly I was tempted to mark this as a critical beta blocker, because I think after the first beta we won't have API changes (even API additions) at all. If this is not the case I agree this is major, otherwise this is a critical beta blocker issue that if not committed will render many use cases hard, if not impossible, to implement.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
tf-922824-7.patch9.63 KB

Improved PHP docs as per @marcvangend's request.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: No way to specify the language entities are being viewed in » No way to specify the language to view entities in
Issue tags: -API clean-up 
+++ modules/node/node.module	26 Sep 2010 08:59:03 -0000
@@ -1263,8 +1266,11 @@ function node_view($node, $view_mode = '
+ * @param $langcode
+ *   A string indicating the language field values are to be shown in. If no
+ *   language is provided the current content language is used.

(and everywhere else) Needs improvement - already suggested something along the lines of the following to @plach:

"(optional) A language code to use for rendering field values [is it only about fields?]. Defaults to the content language of the current request [page?]."

+++ modules/node/node.module	26 Sep 2010 08:59:03 -0000
@@ -1263,8 +1266,11 @@ function node_view($node, $view_mode = '
-function node_build_content($node, $view_mode = 'full') {
+function node_build_content($node, $view_mode = 'full', $langcode = NULL) {

@@ -1280,7 +1286,7 @@ function node_build_content($node, $view
-  $node->content += field_attach_view('node', $node, $view_mode);
+  $node->content += field_attach_view('node', $node, $view_mode, $langcode);

We should make sure that the passed $langcode ends up as $build['#language']

Powered by Dreditor.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
tf-922824-9.patch15.71 KB

If we pass $langcode into the $build arrays for consistency we need to pass it along for hook_*_view() invocations. Updated API documentation files accordingly. Moreover we cannot rely on field_attach_view() default but we have to explictly provide one.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work 
+++ modules/comment/comment.api.php	26 Sep 2010 14:06:37 -0000
@@ -60,14 +60,17 @@ function hook_comment_load($comments) {
- * The comment is being viewed. This hook can be used to add additional data to the comment before theming.
+ * The comment is being viewed. This hook can be used to add additional data to
+ * the comment before theming.

Please revert. A phpDoc summary is allowed to exceed 80 chars, it should be on one line.

+++ modules/comment/comment.module	26 Sep 2010 13:31:10 -0000
@@ -887,13 +887,20 @@ function comment_prepare_thread(&$commen
+  if (empty($langcode)) {

Should be !isset().

Powered by Dreditor.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
tf-922824-11.patch15.48 KB

Here it is :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, tf-922824-11.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs work » Needs review
FileSize
tf-922824-14.patch15.49 KB

rerolled...

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Reviewed this patch multiple times and it looks good now.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Thanks sun!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Priority: Major » Critical

I still think this should go in before the beta release. Setting to critical again in the hope to getting a committer feedback.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +beta blocker

Just to be sure this gets the needed attention in time, I'm marking it as beta blocker: as its status is RTBC committers just need to get it in or moving it back to the major queue if this can be committed later.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Priority: Critical » Major
Issue tags: -beta blocker

API changes alone are not a criteria for the critical priority.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: -API addition +API change

@Damien Tournoud:

Having a partially broken core subsystem that we won't be able to fix after the beta release IS a critical issue. This is not going to block anything, as it's RTBC, so what's the problem?

Preventing committers to evaluate this before it's too late is not helping making Drupal better.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: -API change +API addition
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: -API change +API addition

It's up to Dries and webchick to maintain the RTBC major queue, if they don't then that's a failing on their part, it's not a reason to start marking everything critical again.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Dries and webchick are following the priority order and are going to care about beta blocker critical issues first. The fact that they'll give a look at this after beta has been released does not mean they are not maintaining the major queue, it just means that they are following priorities.

IMO an issue that has to go in before beta is released meets the definition of beta blocker: committers might judge it not worth to be committed but they must have the chance to evaulate it for the priority it has.

I'd say this is a major beta blocker if this definition makes any sense.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Issue tags: +Needs committer feedback
Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

This patch looks good, has been reviewed extensively, and it is fairly important to get it in before beta. Committed to CVS HEAD.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

@Dries:

Thanks!

@Damien, @catch:

I apologize for misusing the beta blocker tag and for being somehow harsh, I didn't know about the Needs commiter feedback one. I'll be more careful next time :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Related issue: #1089174: Prepare view hooks do not receive the language parameter.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Related issues:

#1178480: field_view_field() does not pass language to hook_field_attach_view_alter() implementations (easy one)
#1178500: hook_field_*() prepare/view have no access to the requested language (yched's feedback welcome)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
Issue tags: -Needs committer feedback