Comments

rfay’s picture

subscribe

das-peter’s picture

Issue tags: +dcsprint5

tagging

rfay’s picture

Status: Needs review » Needs work
Issue tags: -dcsprint5

What fantastic work!

+++ modules/product/commerce_product_ui.module
@@ -431,6 +431,18 @@ function commerce_product_ui_form_commerce_product_ui_product_form_alter(&$form,
+  if (module_invoke('translation', 'enabled', 'commerce_product')) {

There's no need to use module_invoke() is there? Seems a rather obscure technique. We could also just wrap with if (module_exists()) or use:

if (function_exists('translation_enabled') && translation_enabled('commerce_product', TRUE)) {
+++ modules/product/commerce_product_ui.module
@@ -431,6 +431,18 @@ function commerce_product_ui_form_commerce_product_ui_product_form_alter(&$form,
+    array_unshift($form['actions']['submit']['#submit'], 'commerce_product_ui_product_form_translation_processing');

@@ -462,6 +475,33 @@ function commerce_product_ui_product_form_submit($form, &$form_state) {
+  if (module_invoke('translation', 'enabled', 'commerce_product')) {

Same issue again...

+++ modules/product/commerce_product_ui.module
@@ -431,6 +431,18 @@ function commerce_product_ui_form_commerce_product_ui_product_form_alter(&$form,
+  if (module_invoke('translation', 'enabled', 'commerce_product')) {

if (function_exists('translation_enabled') && translation_enabled('commerce_product', TRUE)) {

+++ modules/product/commerce_product_ui.module
@@ -431,6 +431,18 @@ function commerce_product_ui_form_commerce_product_ui_product_form_alter(&$form,
+    array_unshift($form['actions']['submit']['#submit'], 'commerce_product_ui_product_form_translation_processing');

@@ -462,6 +475,33 @@ function commerce_product_ui_product_form_submit($form, &$form_state) {
+  if (module_invoke('translation', 'enabled', 'commerce_product')) {
...
+    list(, , $bundle) = entity_extract_ids('commerce_product', $form_state['commerce_product']);

Does this have to go at the beginning of the submit handlers? Comment why, in that case.

+++ modules/product/commerce_product_ui.module
@@ -431,6 +431,18 @@ function commerce_product_ui_form_commerce_product_ui_product_form_alter(&$form,
+    array_unshift($form['actions']['submit']['#submit'], 'commerce_product_ui_product_form_translation_processing');

Does this have to go at the beginning of the submit handlers? If so, comment on why.

+++ modules/product/includes/translation.handler.commerce_product.inc
@@ -0,0 +1,47 @@
+  public function isRevision() {
+    return !empty($this->entity->revision);
+  }
+
+  public function getHumanReadableId() {
+    return $this->entity->title;
+  }
+
+  public function getLanguage() {
+    return $this->entity->language;
+  }
+
+  public function getAccess($op) {
+    return commerce_product_access($op, $this->entity);
+  }
+
+  protected function getStatus() {
+    return (boolean) $this->entity->status;
+  }

Please add comments for all of these, and explain why they're overridden.

+++ modules/product/includes/translation.handler.commerce_product.inc
@@ -0,0 +1,47 @@
+  public function getHumanReadableId() {
+    return $this->entity->title;
+  }

Is the title actually adequate for human readable ID?

Powered by Dreditor.

das-peter’s picture

Fixed some stupid issue and changed some stuff according to rfay's comments.
There's still the need to review the handler class itself.

das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new7.86 KB

Cleaned up translation handler class according to: #1032816: Is method getHumanReadableId() superfluous?
Added documentation to translation handler class.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

I ran through an install of this again today and it all seems to work fine.

bojanz’s picture

Status: Reviewed & tested by the community » Needs work
+  if (module_exists('translation') && translation_enabled('commerce_product')) {

translation is also the name of the core translation module. And that module doesn't have the translation_enabled() function.
You'd need to do module_exists('translation') && function_exists('translation_enabled') && translation_enabled('commerce_product').

You can see the tricks we tried in #1006176: Add support for field based translation, from field_is_translatable($entity_type, $field) to

    $entities = entity_get_info();
    $entity_tables = array();
    $has_translation_handlers = FALSE;
    foreach ($entities as $type => $entity_info) {
      $entity_tables[] = $entity_info['base table'];

      if (!empty($entity_info['translation'])) {
        $has_translation_handlers = TRUE;
      }
    }

    // Doesn't make sense to show a field setting here if we aren't querying
    // an entity base table. Also, we make sure that there's at least one
    // entity type with a translation handler attached.
    if (in_array($this->base_table, $entity_tables) && $has_translation_handlers) {

This is all because I tried to target the field translation API in general, instead of targeting the specific contrib Content Translation module (even though it will probably be the only solution...)

das-peter’s picture

Status: Needs work » Needs review
StatusFileSize
new7.94 KB

@bojanz: Thanks for the hint.
Affected conditions changed.

pcambra’s picture

Translation module is now Entity translation, I've updated this patch to the latest version of it. For this to work smoothly, you also need the patch for entity translation here #1098106: Translated fields aren't validated (or processed with presave and submit field_attach_ hooks)

Also we are going to want to integrate this with Title module, which requires this extra property in the entity info:

/**
 * Implements hook_entity_info_alter().
 */
function xxx_entity_info_alter(&$entity_info) {	
  $entity_info['commerce_product']['field replacement'] = array(
		'title' => array(
			'field' => array(
		    'type' => 'text',
		    'cardinality' => 1,
		    'translatable' => TRUE,
			),
			'instance' => array(
				'label' => t('Title'),
				'description' => t('A field replacing product title.'),
				'required' => TRUE,
				'settings' => array(
					'text_processing' => 0,
				),
				'widget' => array(
					'weight' => -5,
				),  
      ),
		),
	);	
}

I wonder if we should "spin off" all these changes in a contrib for commerce translation support.

pcambra’s picture

Status: Needs review » Needs work

This needs some more work, the product attributes aren't being displayed the right way

pcambra’s picture

I've added a entity_presave invoke in the save of the product because entity translation uses that an we'd probably need it anyways

This patch should be ok now

pcambra’s picture

Status: Needs work » Needs review
pcambra’s picture

These changes add multilanguage options for entity translation at product type level so the translate tab in the product translation screen is displayed correctly

rszrama’s picture

It looks like these if statements have redundant conditions:

if (module_exists('entity_translation') && function_exists('entity_translation_enabled') && entity_translation_enabled('commerce_product'))

If that module exists, that function will necessarily exist. Is there any reason you're checking for both?

Also, I'm not sure why you didn't declare the translation stuff for the product entity in commerce_product.module. It seems the UI module should purely instantiate this while the definition of translation stuff happens in the API module. See for example the fact that our product forms are defined in the API module and only instantiated in the UI module.

pcambra’s picture

I think that the "double check" of the function exists is an heritage from the entity translation module that uses that structure.
I've changed that not to be redundant, I've moved the api functions to commerce_product and also removed one of them that we're not using because we don't have the approach of having a product as translation of another but field translation.

rszrama’s picture

Status: Needs review » Needs work

Two things I noticed in this latest iteration:

  1. Your implementation of hook_translation_info() still references a path defined by the Product UI module. Is it not possible to leave this blank but then implement hook_translation_info_alter() in Product UI to add the actual path association? See for example how we're handling entity URIs.
  2. Your help text and saved variable name appear to be referencing content types still instead of product types.
pcambra’s picture

Status: Needs work » Needs review
Issue tags: -dcsprint5

Ok, changed the content_type references, but it looks that hook_translation_info is going to disappear really soon, see #1114410: Replace hook_translation_info() with hook_entity_info() so I put all the translation info directly into hook_entity_info of comerce_product and the ui parts into the hook_entity_info_alter of commerce_product_ui

Repo: git.drupal.org:sandbox/pcambra/1081200.git
Branch: 1032302
Diff: http://drupalcode.org/sandbox/pcambra/1081200.git/commitdiff/da3e87dacae...

rszrama’s picture

Status: Needs review » Needs work
StatusFileSize
new11.1 KB

This applies cleanly, and I've updated it a little bit. In order to test, you need to:

  1. Install the Entity Translation module.
  2. Enable an additional language at Administration > Configuration > Regional and language > Languages.
  3. Turn on translation for Products at Administration > Configuration > Regional and language > Entity translation.
  4. Enable multi-lingual support for your product type on its edit form.
  5. Edit a product and change its language from "Language neutral" to an enabled language.

However, two things stand out as needing addressed before we can commit this:

  1. We should hide entity translation for Commerce entities from the checkboxes list on the settings form for entities that we know can't be translated to avoid confusion. If there's a property we can use in the entity info array, great. Otherwise let's just alter that form.
  2. When I try to add a translation for a product, the only thing I can translate is the image field I had attached on it. I should be able to supply a translation for the product title as well. How do we enable this?
pcambra’s picture

1: What checkbox list are you referring to?
2: For title translations you need to enable the title module (http://drupal.org/project/title) that transforms the title into a field and then it gets translatable.

rszrama’s picture

The checkboxes list on the entity translation settings form where you select which entities should be translatable. It's at step 3 in my instructions.

pcambra’s picture

Status: Needs work » Needs review
StatusFileSize
new12.74 KB

I'm attaching a patch with the title support integrated. Note that I've added the "field replacement" property to the product entity info as we can't add that to the product entity info alter of commerce_product_ui, which would make more sense, but title module has a really low weight so the alter doesn't have effect.

Step by step with title module:

  1. Install the Entity Translation module. If you want the titles of the product also translated, you'll need to install Title module as well
  2. Enable an additional language at Administration > Configuration > Regional and language > Languages.
  3. Turn on translation for Products at Administration > Configuration > Regional and language > Entity translation.
  4. Enable multi-lingual support for your product type on its edit form.
  5. If you want the title replaced, go to Manage fields of the product, and click on replace link to get a translatable title field instead of the title property.
  6. Edit a product and change its language from "Language neutral" to an enabled language.

I've also added a form alter to hide line items, orders and customer profiles as entity translation uses the property "fieldable" for displaying an entity in the config screen.

I think we'd need to add tests to this.

rszrama’s picture

Status: Needs review » Postponed

Alrighty, I'm committing the changes from your patch and merging it all into dev, as it all appears to work. However, I think there are some general problems with the modules we're depending on that I hope to see sorted out... I was able to get to fatal errors thanks to the Title module when I attempted to add a Spanish translation of a node. Additionally, for some reason the Title module does not load the value of the title field it creates into the entity title on load.

However, by changing my user account language to Spanish, I was able to see that a referenced product was loaded in Spanish w/ its Spanish title.

I'll move this to postponed, as we certainly need tests, particularly for Title translation... but it may be a fool's errand until that module hits a stable release.

das-peter’s picture

Assigned: Unassigned » das-peter

Hey Ryan,

thank you for giving this a test.
I'll attend at the i18n sprint in Berlin, which brings hopefully more structure in the multi-language topic, and it's foreseeable that I've to enable multilingual support in our installation soon. Thus I assign this ticket to me, hopefully I'm able to push this a little bit ;)

plach’s picture

subscribe

plach’s picture

@rszrama:

However, I think there are some general problems with the modules we're depending on that I hope to see sorted out...

If they are reported be sure I'm working on them ;)

Additionally, for some reason the Title module does not load the value of the title field it creates into the entity title on load.

A patch for this is available at #1146724-7: Replacing field values are not always initialized.

rszrama’s picture

Great, thanks for chiming in. : )

guy_schneerson’s picture

StatusFileSize
new65.65 KB

Issue: Entity Title picked up by Cart instead on the new Title Field

Hi Guys
We are in the process of evaluating Drupal commerce for a multi-language /multi-currency e-commerce site (I can't see us using anything but commerce, you guys are doing an amazing job on it).

I have set my commerce site using the entity translation and title modules and I almost got it to work: the problem I am getting is that my Cart is showing an empty description for the line item title, to make things even more confusing, when I design the view it work fine.
While attempting to figure this out I entered test text into the empty “commerce_product” table “title” fields and it displays in my Cart.
I can get around this problem by updating all the views – joining in the product and displaying it's title instead of the line item, however this is not ideal.
As Ryan mentioned above this may be an issue with one of the other modules and as I know I can always get around it, we can wait to see if any of the updates to the other modules resolve this.

I am including an image with three views of my Cart: English French and the view in design mode.
The first product column is the one I added using the product relation and the second one is using the line item

Thanks Again
Guy

das-peter’s picture

@bbguy: I think you've discovered the same issue as I on the i18n sprint in Berlin, this is the related issue: #1157446: Integrate the module entitycache to enhance the support for title translation
It seems that when the cart is built the cache is flushed - I don't know why - and since the title module isn't notified about that it can't replace the original title values by the one from the new field again.

guy_schneerson’s picture

Thanks ill keep an eye on both issues

minff’s picture

just bookmarking..

aturetta’s picture

Ouch...
DC 1.0 managed to be released without it :-(

rszrama’s picture

@aturetta - Not sure what you're talking about (or why this would be a release blocker ; ), but the last patch was actually committed as it was. Please review the comment chain and update if you can - notice particularly comments 22 / 23.

aturetta’s picture

sorry, you are right. I thought postponed meant 'not fixed'.
plus, a post on IRC by the original poster yesterday led me to think this patch had to be applied manually....
Surely not a blocker, but for Multilingual sites Entity Translation is necessary

summit’s picture

Hi, @das-peter any news on this? Is it somehow possible now to translate products like nodes as such that you can see the different translations and the right translation is selected by product reference when the product node (display) is in the same language?

I want to have only one product-stock, and would love to be able to have my site on different sites (multidomain) in different languages (this issue I think).

Thanks a lot in advance for your update.
greetings, Martijn

plach’s picture

I'm not sure but I think this issue has been obsoleted by #1495570: Update Entity translation integration and could be marked as a duplicate.

bojanz’s picture

Status: Postponed » Closed (fixed)

Yep. The code from this issue was committed.