I thought we have a priority problem even leading to a fatal error, but i'm unsure now about the origin.
Related #1694480: Implement hook_module_implements() instead of relying on the module weight
In entity_translation_menu_alter() you're iterating over entity_get_info() where entity_translation_enabled().
Then you're setting $edit_item to NULL
foreach ($et_info['path schemes'] as $scheme) {
$translate_item = NULL;
$edit_item = NULL;
Below you're doing unconditionally
$original_item = $edit_item;
...
$items["$edit_path/%entity_translation_language"] = array(
'type' => MENU_DEFAULT_LOCAL_TASK,
...
)
// We need to inherit the remaining menu item keys, mostly 'module'
// and 'file' to keep ajax callbacks working (see form_get_cache() and
// drupal_retrieve_form()).
+ $original_item;
// Add translation callback.
$add_path = "$edit_path/add/%entity_translation_language/%entity_translation_language";
$source_position = count($edit_path_parts) + 1;
$target_position = count($edit_path_parts) + 2;
$args = array($entity_type, $entity_position, $source_position, $target_position, $original_item);
$items[$add_path] = array(
'title callback' => 'Add translation',
...
) + $original_item;
}
There are conditions (and i'm in one of them) where origina_item is still NULL. PHP explodes.
PHP Fatal error: Unsupported operand types in /var/www/.../sites/all/modules/contrib/entity_translation/entity_translation.module on line 454
Comment | File | Size | Author |
---|---|---|---|
#23 | 1818440_status_published.patch | 619 bytes | miro_dietiker |
#20 | et-edit_item_fatal-1818440-18.interdiff.do_not_test.patch | 1.97 KB | bforchhammer |
#20 | et-edit_item_fatal-1818440-18.patch | 6.02 KB | bforchhammer |
#16 | et-edit_item_fatal-1818440-16.patch | 6.04 KB | bforchhammer |
#11 | et-edit_item_fatal-1818440-11.patch | 3.28 KB | plach |
Comments
Comment #1
miro_dietikerAttaching a patch that tries to catch this ugly situation. Protection seems needed in entity_translation although the origin might be an unclean other module.
Comment #2
miro_dietikerOopsie, better patch :-)
Comment #3
plachMakes sense to me. Benedikt?
Comment #4
bforchhammer CreditAttribution: bforchhammer commentedHm, we could use the provided patch for a quick fix, but I'm wondering whether there's actually something else going wrong... as far as I can see
$edit_item
(hence also$original_item
) is always set in the following lines of code:The only case I see where this wouldn't be the case is if the respective menu item for the edit path doesn't exist. IMO that is something that should be taken care of during
_entity_translation_validate_path_schemes()
...@miro_dietiker: I'm curious about the path/menu-item causing the error... could you try to find out the runtime values of
$scheme
,$edit_path_parts
, and possibly$items
for the item causing problems?Comment #5
bforchhammer CreditAttribution: bforchhammer commentedIs this related to #1818500: different edit path than entity module?
Comment #6
miro_dietikerIn my case this is NULL:
&$items[implode('/', $edit_path_parts)];
Because entity created
admin/config/services/sharemessage/manage/%entity_object/edit
and entity translation expects to see
admin/config/services/sharemessage/manage/%sharemessage/edit
And the loop immediately terminates.
If that missing "path wildcard" is the issue i don't see why entity_translation tries to query for "%sharemessage".
Yes it is related and that issue was the origin. However, there's no reason to produce a PHP fatal error in such an issue.
Comment #7
bforchhammer CreditAttribution: bforchhammer commentedWell, that's just the default value; we need something to work with, so if you don't specify it then we try to guess it; core uses wildcards like
%node
or%user
, so we use%[entity-type]
as the default pattern.So does adding a path wildcard solve this issue?
If yes, attached patch should help with avoiding the fatal error.
Comment #8
bforchhammer CreditAttribution: bforchhammer commentedSetting NR.
Comment #9
plachI've been able to reproduce this, but #7 does not seem to fix the issue. To see the fatal just change the taxonomy base path from
taxonomy/term/%taxonomy_term
totaxonomy/term/%term
. I think it's correct that this path passes validation because it's compatible with the actual one. The % replacement was introduced exactly to handle situations where the menu loader has been replaced but the router path structure remains the same. For instance when Views takes over thetaxonomy/term/%
path. We should make sure this works again as it's a regression wrt alpha2, I think.Comment #10
plachtagging
Comment #11
plachI tried this with Views replacing taxonomy listings + a bougs base path (
'taxonomy/term/%term'
) and things didn't break, except that the menu loader didn't exist and yielded to another fatal, but that's the expected behavior.Comment #12
bforchhammer CreditAttribution: bforchhammer commentedI'm still not quite sure what the views-taxonomy thing actually means; I'll try to reproduce and confirm the patch now.
Also, it's a slightly different issue, but I'm still thinking that the stricter path wildcard validation (patch #5) also makes sense, because we need the path wildcard in the path in order to be able to determine the position of the entity id (we have code like
$entity_position = array_search($wildcard, $path)
).Comment #13
plachActually Views doesn't break because it replaces only the base/view path, but if it replaced also the edit path we would have the same fatal reprted in the OP.
Comment #14
plachActually this is not a regression since replacing taxonomy listings with Views is still working. I'd like to release beta1 with this one fixed, but I won't consider this a blocker.
Comment #15
plachAny news here?
Comment #16
bforchhammer CreditAttribution: bforchhammer commentedHere's a new patch based on the previous two.
I added the code from patch #7, because the way
hook_menu_alter()
is written at the moment, the existence of a matching wildcard is definitely required -- if we want to change that, we also need to adjust respective bits inhook_menu_alter()
.I changed this back to 'edit path'. Mainly for consistency as we use the "non real" path for the other menu items as well; if there's a reason to use the real one instead we should probably document it and add some tests.
I also moved a few things around in the validation function in an attempt to make it easier to read through and understand (and to remove some duplicate code); added some comments as well.
Attached patch should still solve the issue of the OP, and also keep things working with the views-taxonomy listing.
Comment #17
bforchhammer CreditAttribution: bforchhammer commentedComment #18
plachThanks!
@miro_dietiker:
Can you confirm the issue is fixed?
Comment #19
plachThe bug is still there :(
I applied the patch, renamed the taxonomy base path from
taxonomy/term/%taxonomy_term
totaxonomy/term/%term
and cleared the caches. Boom.Comment #20
bforchhammer CreditAttribution: bforchhammer commentedHm, there was a typo in a variable name. Also cleaned up some whitespace.
Comment #21
plachLooks good, fixes the issue and works with Views replacing taxonomy listings!
Committed and pushed, thanks!
Comment #22
miro_dietikerMuch much better!
(But not perfect yet ;-) )
In addition i think for consistency, we should set the translation status to TRUE for all new nodes. not query for the node status.
Inconsistency example:
Compare case A
Create a node, unpublished
=> status of first translation is UNPUBLISHED
publish the node
=> status of first translation is PUBLISHED
unpublish the node
=> status of first translation is PUBLISHED
If we make the first translation record implicit, we rely on the node "published" state and we rely on the default (source) language.
I thus suggest to make this published as the source language is the initial reference.
The rest seems fine, test coverage is insufficient. Tests pass still without any change. :-)
Comment #23
miro_dietikerHmm... where is my patch?!
Comment #24
plachEr, wrong issue?
Comment #25
plachPosted #22-#23 in #1672710-28: published source node appears as not published in the translate tab. Back to fixed.