I'm calling this from another module, on an entity type that doesn't have entity translation enabled.

I'm getting TRUE from entity_translation_enabled_bundle() every time. Because of the return empty($bundle_callback) line.

To me this looks like a straight up issue with the code, if an entity translation bundle callback is not defined, this means entity translation is not enabled on the bundle. But the return empty($bundle_callback) will return TRUE.

CommentFileSizeAuthor
#8 entity_translation-improve_documentation-for-bundle-callback_2185523-8448029.patch1.72 KBLeon Kessler
#6 entity_translation-add-documentation-to-entity_translation_enabled_bundle-2185523-8440353.patch964 bytesLeon Kessler
#1 entity_translation-fix_entity_translation_enabled_bundle_returning_false-2185523.patch1.11 KBLeon Kessler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented
Status: Active » Needs review
FileSize
entity_translation-fix_entity_translation_enabled_bundle_returning_false-2185523.patch1.11 KB

Patch attached.

plach’s picture

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

IIRC there can be entity types that do not define a bunde callback, but nonetheless can be eabled for translation. The current patch does not seem to support this use case.

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented

Okay, I didn't read the docs properly

In entity_translation.api.php:

 * - bundle callback: A callback to check whether the passed bundle has entity
 *   translation enabled. If empty all bundles are supposed to be enabled.

So this makes the original code correct, but it assumes that you have already made the check for translations being enabled for the entity.

I think entity_translation_enabled_bundle() should check that the entity itself is enabled first. Although I understand there would be objections to this, as it would mean calling entity_translation_enabled(), which itself calls entity_translation_enabled_bundle().

Otherwise, I think this should at least be documented.

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented
Title: entity_translation_enabled_bundle() returns TRUE even when it's disabled on that bundle » entity_translation_enabled_bundle() returns TRUE even when it's disabled on the entity
plach’s picture

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

Actually entity_translation_enabled() can perform also a bundle-level check if you specify it, so I guess we just need some docs improvements here. entity_translation_enabled_bundle() was a later addition and I agree the whole DX is not ideal. We should probably state that you usually want to use entity_translation_enabled() for this kind of checks.

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented
Status: Needs work » Needs review
FileSize
entity_translation-add-documentation-to-entity_translation_enabled_bundle-2185523-8440353.patch964 bytes

Okay how about this?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Title: entity_translation_enabled_bundle() returns TRUE even when it's disabled on the entity » Document that entity_translation_enabled_bundle() does not check whether the entity type is translatable
Component: Base system » Documentation
Category: Bug report » Task

Awesome, but since we are here what about updating also the docs of hook_entity_info() in entity_translation.api.php? Do you think there is anything we can add to help clarifying this?

Leon Kessler’s picture

Leon Kessler CreditAttribution: Leon Kessler commented
FileSize
entity_translation-improve_documentation-for-bundle-callback_2185523-8448029.patch1.72 KB

I think the documentation for bundle callback by hook_entity_info is okay. But I think the language of it's default behaviour could be updated to match the format from the rest of the documentation.

Have updated this in latest patch. What do you think?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

  • leon.nk authored 54f54f0 on 7.x-1.x 
    Issue #2185523 by leon.nk: Document that...
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed, sorry for the delay.

Status: Fixed » Closed (fixed)

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