I'm trying to put the SKU field in a field_group_table, and it turned out that Commerce uses #markup for the "extra" fields. This is bad practice and has implications for any module wanting to make changes using the render API.

This is the code it's about:

            $entity->content[$content_key] = array(
              '#markup' => theme($product_extra_field['theme'], $variables),
              '#attached' => array(
                'css' => array(drupal_get_path('module', 'commerce_product') . '/theme/commerce_product.theme.css'),
              ),
            );

Something along these lines would be better:

            $entity->content[$content_key] = array(
              '#theme' => $product_extra_field['theme'],
              '#title' => $product_extra_field['label'],
              '#sku' => $variables['sku'],
              '#attached' => array(
                'css' => array(drupal_get_path('module', 'commerce_product') . '/theme/commerce_product.theme.css'),
              ),
            );

Of course the above is hardcoded to the SKU field and needs work to be able to apply to the Title and Status fields as well.

A follow-up question is that I noticed that for these fields, no regular field formatter is used. Is there a specific reason not to? I'd like to be able to choose to hide the Field Label for example, like with other fields.

CommentFileSizeAuthor
#20 use_render_api_instead_of_custom_markup-1972700-20.patch2.44 KBa.ross
#17 use_render_api_instead-1972700-17.patch2.44 KBdiscipolo
#3 use_render_api_instead_of_custom_markup-1972700-3.patch2.43 KBa.ross

Comments

rszrama’s picture

rszrama commented

Sounds like a reasonable change to me.

Re: the label, though, the issue is that these are "extra fields" not "Field API fields." It's a bit confusing, but with extra fields there simply is no option to make those sorts of customizations through the Field UI.

a.ross’s picture

a.ross commented

Yeah, I was afraid of that. It would at least make sense to translate the current stuff to what would otherwise be a sensible default for displaying that field. As in, let's say the SKU field was a regular field, you'd have the label display inline and the value plaintext or something. With just the difference that you can't change it in the field UI (-:

a.ross’s picture

a.ross commented
Status: Active » Needs review
StatusFileSize
new use_render_api_instead_of_custom_markup-1972700-3.patch2.43 KB

Ok, I've been trying to work out a solution, but it's a bit more tricky than I hoped. I can get the fields to work with the regular field template but that would break compatibility. So all I've basically done with this patch is to delay rendering of the extra fields until the entire page is rendered. It's not ideal, albeit probably the best we can do.

a.ross’s picture

a.ross commented

Think this can go in? I'd be delighted

rszrama’s picture

rszrama commented

Ahh, I assumed from your description it was an incomplete solution. Reading it again I see that you said you didn't pursue an option because it was a breaking change. Makes sense, as these aren't fields anyways. (Fields and "Extra Fields" are confusingly entirely different concepts in Drupal core. : ) The patch inadvertently removes the : from the first label, but I can fix it in testing and try to get it in in a bit. Tearing through the Address Field queue at the moment. ; )

a.ross’s picture

a.ross commented

Alright, happy to hear that. Good catch, too.

And yeah I noticed they're entirely different :/. I just hoped a utopia was around the corner, where even field_extra_fields used the regular field theme. Guess not (-:

a.ross’s picture

a.ross commented

Wheh, Address Field's queue must be pretty long! ;-)

rszrama’s picture

rszrama commented

I've only closed out about 100 issues and 50 bugs so far...

a.ross’s picture

a.ross commented

Oh wow, I underestimated that by a tiny margin then o_0

rszrama’s picture

rszrama commented

hehe Well, it hadn't had a new release since May 2012. A lot had built up over time, but there were a lot of duplicate / mis-categorized issues as well. Still, it was a productive run... can't wait to close it out to a 1.0. : )

a.ross’s picture

a.ross commented

Bump

philsward’s picture

philsward commented

Is this issue directly related to the fact that you can't change the label's for the hard-coded stuff like title and SKU? In trying to setup a store, I want to show the title field but get rid of the word "title". Just wondering if I should follow this issue or kick up another one...

At this point, it looks like a CSS hack might be the only solution :-/

a.ross’s picture

a.ross commented

You should be able to change the output right now by either overriding the theme function or perhaps implementing a pre-render function for it. It's just that with this patch it won't render HTML code until the entire page is rendered, allowing any alter hook down the pipeline to use the Render API code.

roball’s picture

roball commented

philsward:

In trying to setup a store, I want to show the title field but get rid of the word "title"

I had the same problem. A Product Display content type always shows SKU and Title of the referenced product with exactly these wordings for the label, and the label is always set to be displayed Above. Inline and Hidden are not possible. The only built-in alternative is to not display these fields at all, by selecting the Formatter Hidden (instead of Visible). This is what I did. I created two custom fields of type Computed instead. This gives you complete freedom on what is displayed how.

roball’s picture

roball commented
Issue summary: View changes

Minor edit

zmove’s picture

zmove commented
Priority: Normal » Major

This issue is between the feature request and the bug report, but it sounds major to me as it will create bug with a lot of other modules that deal with fields.

Here is a little example : #1987494: Make field_group_table themable

That's just one example, each time I make a project using drupal commerce, I encounter some problems because of that.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Needs work 
+++ b/modules/cart/commerce_cart.module
@@ -2106,15 +2106,12 @@ function commerce_cart_add_to_cart_form_attributes_refresh($form, $form_state) {
-          'label' => $product_extra_field['label'] . ':',
...
+          '#label' => $product_extra_field['label'],

Just need to add that colon back. Otherwise nice work and I'd RTBC this patch:)

discipolo’s picture

discipolo commented
Status: Needs work » Needs review
StatusFileSize
new use_render_api_instead-1972700-17.patch2.44 KB

added back that colon

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Thank you @discipolo. RTBC!

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Reviewed & tested by the community » Needs work

Sorry spotted something on second glance.

+++ b/modules/cart/commerce_cart.module
@@ -2336,15 +2336,12 @@ function commerce_cart_add_to_cart_form_attributes_refresh($form, $form_state) {
+          'label' => $product_extra_field['label'] . ':',

+++ b/modules/product_reference/commerce_product_reference.module
@@ -436,14 +436,11 @@ function commerce_product_reference_entity_view($entity, $entity_type, $view_mod
+              '#label' => $product_extra_field['label'] . ':',

Missing # in front of first label.

a.ross’s picture

a.ross commented
StatusFileSize
new use_render_api_instead_of_custom_markup-1972700-20.patch2.44 KB

This should do it

a.ross’s picture

a.ross commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Thanks @a.ross, that should do the trick.

rszrama’s picture

rszrama commented

Updating the patch to remove variables from inside strings then committing.

rszrama’s picture

rszrama commented
Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

And committed! Thanks!

  • rszrama committed 753c73c on authored by a.ross 
    Issue #1972700 by a.ross, discipolo, rszrama, joelpittet: Use render API...

Status: Fixed » Closed (fixed)

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