Updated: Comment #80

Problem/Motivation

(needs better problem description) When vocab is set to show on full node, but not teaser, when previewing get an error because the preview shows both the full node and the teaser.

Notice: Undefined index: taxonomy_term in taxonomy_field_formatter_view() (line 1488 of .../modules/taxonomy/taxonomy.module).
EntityMalformedException: Missing bundle property on entity of type taxonomy_term. in entity_extract_ids() (line 7539 of .../includes/common.inc).
EntityMalformedException: Missing bundle property on entity of type taxonomy_term. en entity_extract_ids() (línea 7389 de /home/janipuni/webapps/web/includes/common.inc).

Steps to reproduce

Creating anew node, and selecting "preview". If you selects "save", there is no error. (from #13)

from #16 To recreate this bug on a fresh "standard" Drupal install:

  1. Change the article content type's "tags" field widget to "Select list" (/admin/structure/types/manage/article/fields/field_tags/widget-type)
  2. Change the article content type's "tags" field display format to "Hidden" in the Default view mode (/admin/structure/types/manage/article/display)
  3. Confirm that the field is NOT hidden in the Teaser view mode (/admin/structure/types/manage/article/display/teaser)
  4. Create a test term or two in the Tags vocabulary (/admin/structure/taxonomy/tags/add)
  5. Go to /node/add/article, enter a title, select a Tags term, and click 'Preview'.

Note, this bug doesn't happen if the widget for the field is Autocomplete. (hence step #1) Although #17 says can get error with autocomplete.

Proposed resolution

Patch in #54, noted in #55 to be just test coverage, was committed to 8.x in #66

(needs updating, needs the approach described in words)
#77 has a d7 patch, but might not be a complete solution for d7.

Workaround

from #15
I found out that the taxonomy field which gave the error was 'hidden' in the manage display of my content type. After enabling the field, the error was solved!

So be sure that in Structure > Content types > Name of your content type > manage display, your field that gives the error, is shown!

Remaining tasks

  • find the best approach for d7

User interface changes

?

API changes

no

Original report by @janipuni

Drupal 7.8
Rules 7.x-2.0-rc2

I created a simple content type (title and body) called "service".
I created a vocabulary.
I created a rule:
- Events: After save new content
- Conditions: Content is of type: node and content type to check for "service".
- Actions: Create a new entity, type: "taxonomy term". Value: [node:title]. Vocabulary: 2.

When I add new content to "service" I get this error:

EntityMalformedException: Missing bundle property on entity of type taxonomy_term. en entity_extract_ids() (línea 7389 de /home/janipuni/webapps/web/includes/common.inc).

This didn't happen with the 7.7 version of Drupal core.

Any ideas?
Thanks.

CommentFileSizeAuthor
#129 interdiff.txt2.03 KBDavid_Rothstein
#129 1289336-129.patch5.68 KBDavid_Rothstein
#129 1289336-129-TESTS-ONLY.patch5.24 KBDavid_Rothstein
#123 1289336-reroll-of-116.patch5.57 KBmatglas86
#116 1289336-reroll_of_25-drupal.org-do-not-test.patch5.57 KBdrumm
#112 1289336-112.patch5.81 KBDavid_Rothstein
#103 1289336_multiple-view-mode-with-test_v5.patch4.36 KBjthorson
#98 1289336_multiple-view-mode-with-test_v4.patch4.44 KBjthorson
#95 1289336_multiple-view-mode-with-test_v3.patch4.45 KBjthorson
#89 1289336_multiple-view-mode-with-test_v2.patch4.43 KBjthorson
#89 1289336_multiple-view-mode-test-only.patch2.87 KBjthorson
#88 1289336_multiple-view-mode-with-test_v2.patch0 bytesjthorson
#88 1289336_multiple-view-mode-test-only.patch2.87 KBjthorson
#86 1289336_multiple-view-mode-with-test.patch4.43 KBjthorson
#86 1289336_multiple-view-mode-test-only.patch2.87 KBjthorson
#81 1289336-reroll_of_25.patch5.55 KBjthorson
#77 1289336-preview-node-field-attach-test-and-fix-drupal7_reroll_v2.patch5.56 KBjthorson
#74 1289336-preview-node-field-attach-test-and-fix-drupal7_reroll.patch5.41 KBjthorson
#54 node-1289336-tests-d8.patch5.41 KBno_commit_credit
#54 interdiff.txt1.93 KBno_commit_credit
#36 1289336-preview-node-field-attach-test-only-drupal7.patch5.12 KBJimmyAx
#36 1289336-preview-node-field-attach-test-and-fix-drupal7.patch5.54 KBJimmyAx
#35 1289336-preview-node-field-attach-test-drupal8.patch5.3 KBJimmyAx
#32 drupal-node_preview_errors-1289336-test-only.patch2.87 KBJimmyAx
#32 drupal-node_preview_errors-1289336-test-and-fix.patch3.29 KBJimmyAx
#30 drupal-node_preview_errors-1289336-30-do-not-test.patch3.29 KBrooby
#25 preview_fails.patch3.44 KBmarcingy
#21 multiple_view_mode_test_only.patch3 KBmarcingy
#21 multiple_view_mode_with_test.patch4.66 KBmarcingy
#20 multiple_view_mode.patch1.66 KBmarcingy
#14 Modules _ Enabled.pdf1.07 MBVc Developer
#14 Article _ DS.pdf162.19 KBVc Developer
#7 node.txt7.49 KBJimmyAx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Status: Active » Postponed (maintainer needs more info)

It occurs because of #1067750: Let Field API fail in a tale-telling way on invalid $entity because you have a mailformed entity. The key question is what contrib modules etc do have installed as one of them is the likely cause.

DamienMcKenna’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)
minneapolisdan’s picture

Just to pile on here, I was struggling with this error today, and it wasn't because of File Entities or the Media Module, both of which I am using. And it wasn't because I needed to change the common.inc file. It was because I deleted a Taxonomy Term. After I did so, when viewing any Node that had previously been tagged with that Taxonomy Term threw this error, "EntityMalformedException: Missing bundle property on entity of type taxonomy_term. in entity_extract_ids()" .

I know this error can mean different things for different people, but to save someone else some time in the future, check to see if this is the cause. The solution was to re-save each of the Nodes in question.

I think this seems like a Drupal bug, though, doesn't it? Should this happen?

matt2000’s picture

Component: entity system » taxonomy.module
Status: Closed (duplicate) » Active

This is not a duplicate. The other issue dealt with creating terms programmatically. This user and I are both experiencing this issue using the node-edit form UI. In my case, I'm only using the core forum module. My full backtrace is below. note that only core function are present. Also, I do not have any modules running that would alter the node edit form.

This occurs on Node Preview.


#0 /home/webuser/public_html/mysite.com/includes/common.inc(7592): entity_extract_ids()
#1 /home/webuser/public_html/mysite.com/modules/taxonomy/taxonomy.module(1437): entity_uri()
#2 /home/webuser/public_html/mysite.com/modules/field/field.default.inc(210): taxonomy_field_formatter_view('taxonomy_term', NULL)
#3 /home/webuser/public_html/mysite.com/modules/field/field.attach.inc(198): field_default_view('taxonomy_term', NULL)
#4 /home/webuser/public_html/mysite.com/modules/field/field.attach.inc(375): _field_invoke()
#5 /home/webuser/public_html/mysite.com/modules/field/field.attach.inc(1169): _field_invoke_default()
#6 /home/webuser/public_html/mysite.com/modules/node/node.module(1361): field_attach_view()
#7 /home/webuser/public_html/mysite.com/modules/node/node.module(1285): node_build_content('node', Object(stdClass), Array, Array, 'und', Array, Array)
#8 /home/webuser/public_html/mysite.com/modules/node/node.pages.inc(382): node_view()
#9 /home/webuser/public_html/mysite.com/includes/theme.inc(1029): theme_node_preview('node', Object(stdClass), Array, Array, 'und', Array, 'teaser', NULL)
#10 /home/webuser/public_html/mysite.com/modules/node/node.pages.inc(357): theme()
#11 /home/webuser/public_html/mysite.com/modules/node/node.pages.inc(322): node_preview('view', 'node', Object(stdClass), 'teaser', NULL, Array)
#12 /home/webuser/public_html/mysite.com/includes/form.inc(1430): node_form_build_preview('view', 'node', Object(stdClass), 'teaser', NULL, Array)
#13 /home/webuser/public_html/mysite.com/includes/form.inc(844): form_execute_handlers('node', Object(stdClass), 'teaser', 'en')
#14 /home/webuser/public_html/mysite.com/includes/form.inc(364): drupal_process_form(Object(stdClass), 'teaser', 'en')
#15 /home/webuser/public_html/mysite.com/includes/form.inc(123): drupal_build_form(Object(stdClass), 'teaser')
#16 /home/webuser/public_html/mysite.com/modules/node/node.pages.inc(66): drupal_get_form()
#17 /home/webuser/public_html/mysite.com/includes/menu.inc(503): node_add(Array)
#18 /home/webuser/public_html/mysite.com/index.php(21): menu_execute_active_handler()

My full contrib list is:
Views (w/ Ctoosl dependency)
Admin_menu
Mollom
Paranoia
Security_review
Link
References (node_reference and user_reference)
fb (Drupal for Facebook) --Facebook Connect, Facebook Views, Facebook User Managment, and dependencies

I have not added any fields to the forum content type.

xjm’s picture

Aha. In #4, a NULL term entity is being passed to field_default_view().

Would it be possible to get a var_dump() of your node object?

xjm’s picture

Version: 7.8 » 7.x-dev

Also, please confirm which version of core you're using. If I recall correctly, there's a bug that was fixed in 7.10 that might be related to this.

JimmyAx’s picture

FileSize
7.49 KB

If I have the same bug then is not fixed in 7.10. I have the exact same error message, but the stacktrace is a little different.

I got this when trying to preview a forum node when editing it.

xjm’s picture

Awesome, thank you @JimmyAx for the full backtrace. It's very helpful.

  ["taxonomy_forums"]=>
  array(1) {
    ["und"]=>
    array(1) {
      [0]=>
      array(1) {
        ["tid"]=>
        string(1) "3"
      }
    }
  }
  ["taxonomy_vocabulary_8"]=>
  array(0) {
  }

Alright, so it looks here like there are two vocabularies on this node: the forum vocabulary, and then taxonomy_vocabulary_8, whatever that is. The latter is empty, which means that:

  1. _field_invoke() will set $items to be an empty array
  2. which gets passed through field_default_view() unchanged
  3. and the foreach in taxonomy_field_formatter_view() has nothing to loop over, so entity_uri() won't get called.

So I don't think that one is the problem. For the other, $items will get set to

   array(1) {
      [0]=>
      array(1) {
        ["tid"]=>
        string(1) "3"
      }
    }

which, when it gets down to taxonomy_field_formatter_view(), has no taxonomy_term key and therefore passes a NULL value to entity_uri().

So the problem is why the tid is the only value set there. It seems this is actually a different bug from in the other backtrace.

@JimmyAx : Can you check and see if a term with tid 3 actually exists? I have a suspicion where this is going.

xjm’s picture

Title: Missing bundle property on entity of type taxonomy_term. en entity_extract_ids() (line 7389) » Node preview for forum nodes gives missing bundle property on entity of type taxonomy_term error

Retitling to make this issue specifically about #4 and #7, which are reproduced similarly at least. There are lots of other ways for bad data to cause this error message, but let's try to find out if there's an actual bug related to forum previews.

And, as always, steps to reproduce from a clean install of Drupal core would be invaluable.

JimmyAx’s picture

@xjm: A term with the id 3 does exist. It's a forum and actually the forum the node belonged to. taxonomy_vocabulary_8 is a custom vocabulary. I use it for tagging but it's empty for this node.

I haven't been able to reproduce it so far and will look more closely into it.

Robin Millette’s picture

Create a new term. Add it to a node.
Now delete that term.

You're bound to trigger the error now, depending on modules in use, as I encountered in #1449858: EntityMalformedException triggered if a deleted term is still referenced by a node. Haven't had time to dig any deeper yet.

xjm’s picture

Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs steps to reproduce

Right, those steps don't reproduce the problem when I do them from a clean install. So, the steps should look like:

  1. Install Drupal 7.x using the Standard profile.
  2. Install module X.
  3. (Insert steps here.)
  4. Create a taxonomy term.
  5. Create a node tagged with this taxonomy term.
  6. Delete this taxonomy term.
  7. (Insert steps here.)

We need to know if and what contributed module causes the problem (and this issue should probably be moved to that module's queue).

boran’s picture

I'm also getting then when Creating anew node, and selecting "preview". If you selects "save", there is no error.

Its is a content type (not a forum as above) with a few taxonomy terms.
AFAIK I've not deleted any tax terms.
The site has been migrated from D6.

Added some debugs into entity_extract_ids(), giving:
entity_type=taxonomy_term
$info['entity keys']=
[id] => tid
[bundle] => vocabulary_machine_name
[label] => name
[revision] =>

Suggestions?
Site the site is now broken I need to add code to handle this case in common.inc. But its unclear to me what $bundle should be set to, I dont understand the flow enough :-( )
Can I just set, for this case:
$bundle = $entity_type;
$entity->{$info['entity keys']['bundle']} = $entity_type;
where entity_type=taxonomy_term?

Sean

Vc Developer’s picture

FileSize
162.19 KB
1.07 MB

Hi,

Found the problem and I can re-produce it every time! And, I agree with #1, its a contrib module causing the problem. For me its DS's added formats for the Taxonomy Field: "Rendered taxonomy term" and "Seperated".

After a fresh D7.14 install, I systematically enabled modules (Entity API, fieldgroup, Renderable Elements, DS) and tested taxonomy terms using "Preview" on an Article with the "Default" format "Link". It was not until I installed DS and changed it to either one of the DS formats when I get this error. It only does this when you enter a term, other wise having defined terms or not, when you enter a term will cause this error.

I'll send this over the DS.

hullabaloo’s picture

I encountered the same problem: a certain content type gave an error while previewing it. The error I got was a missing bundle property on entity of type taxonomy_term. I checked if there was any bundle information missing in my database, which wasn't the case.

After a lot of trail and error, I found out that the taxonomy field which gave the error was 'hidden' in the manage display of my content type. After enabling the field, the error was solved!

So be sure that in Structure > Content types > Name of your content type > manage display, your field that gives the error, is shown!

brockfanning’s picture

To follow up on #15: If you really don't want the taxonomy field to be displayed (which is presumably why you had it hidden...) check to make sure that it is also hidden in the 'Teaser' view mode. Because the Drupal "Preview" page displays both the Default and Teaser view modes, this error happens if the field is displayed in one but not the other.

To recreate this bug on a fresh "standard" Drupal install:

  1. Change the article content type's "tags" field widget to "Select list" (/admin/structure/types/manage/article/fields/field_tags/widget-type)
  2. Change the article content type's "tags" field display format to "Hidden" in the Default view mode (/admin/structure/types/manage/article/display)
  3. Confirm that the field is NOT hidden in the Teaser view mode (/admin/structure/types/manage/article/display/teaser)
  4. Create a test term or two in the Tags vocabulary (/admin/structure/taxonomy/tags/add)
  5. Go to /node/add/article, enter a title, select a Tags term, and click 'Preview'.

Note, this bug doesn't happen if the widget for the field is Autocomplete. (hence step #1)

JimmyAx’s picture

Status: Postponed (maintainer needs more info) » Needs work

Confirmed.
After reproducing the issue as described in #16 I changed the tags field widget to Autocomplete and it did also reproduce the issue.

Errors:

Notice: Undefined index: taxonomy_term in taxonomy_field_formatter_view() (line 1488 of .../modules/taxonomy/taxonomy.module).
EntityMalformedException: Missing bundle property on entity of type taxonomy_term. in entity_extract_ids() (line 7539 of .../includes/common.inc).
matt2000’s picture

Title: Node preview for forum nodes gives missing bundle property on entity of type taxonomy_term error » Node preview for nodes with tags gives missing bundle property on entity of type taxonomy_term error

Re-titled since we confirmed it as a general taxonomy problem, and not specific to Forums.

marcingy’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » marcingy

This bug is still present in d8.

And infact the way to create the issue is actually much simpler
* Add a select list
* Set default to hidden
* Set teaser to not hidden

This results in a value of

array (
  'tid' => '1',
)

being passed into preview vs a full term object being when the default is set to none hidden. I take a look at this over the next couple of days and hopefully find why teaser isn't loading an object, or we need to check if we just have a tid and load a term.....

The error appears to lie in call stack of field_attach_prepare_view this may well take a bit of diging.

marcingy’s picture

Title: Node preview for nodes with tags gives missing bundle property on entity of type taxonomy_term error » Calling node_view for the same node with muliple view modes on the same page does not correctly attach fields
Status: Needs work » Needs review
FileSize
1.66 KB

This is a patch without a test, I'll work on one of those next. Basically the issue is as follows

field_attach_prepare_view sets a flag to indicate if an entity has been processed for viewing. However if the node is viewed more than once on a page the incorrect information can be passed into the display formatter. An entity can only have a single set of fields attached so we need to record the view mode that the entity is currently prepared for.

marcingy’s picture

JimmyAx’s picture

Patch is working for 7.x too, though this seems wrong:

$entity->_field_view_prepared = array(
  $view_mode => TRUE,
);

Shouldn't it be something like this?

if (empty($entity->_field_view_prepared)) {
  $entity->_field_view_prepared = array();
}
$entity->_field_view_prepared[$view_mode] = TRUE;

Otherwise you would be overwriting the array.

Damien Tournoud’s picture

Component: taxonomy.module » node.module
Status: Needs review » Needs work

Hm. That's likely not going to fly. We don't enforce the preparation step to be re-entrant right now: we don't know if things are not going to break if you prepare the same entity twice for two different view modes.

The actual issue is this code in node_preview():

    $nodes = array($node->nid => $node);
    field_attach_prepare_view('node', $nodes, 'full');

field_attach_prepare_view() is already called in node_view() so this is completely unnecessary and breaks the node_view(clone $node, 'teaser'); call in theme_node_preview().

Also, given the nasty side effects of viewing an entity, I think node_view() should do its own cloning of the entity object, but that's a bigger change (and not backportable).

marcingy’s picture

@JimmyAx overwriting is in intentional as a node can only be in one view mode at a time so if it was rendered in full and then teaser view mode the node is actually prepared for teaser and has incorrect information related to the full mode.

@Damien Tournoud I agree that would significant change and I was unaware the system was not designed to be re-entarant.

marcingy’s picture

Status: Needs work » Needs review
FileSize
3.44 KB

Re-roll based on #23 tests of course remain valid.

xjm’s picture

guydaniels’s picture

We were getting the same errors as #17 which were preventing our pages from working. None of the patches here fixed them. I finally did the simple thing of populating the 'taxonomy_term' (the last line was #1488, where the first error was occurring):

          if (!isset($item['taxonomy_term'])) {
            $item['taxonomy_term'] = taxonomy_term_load($item['tid']);
          }
          $term = $item['taxonomy_term'];

It seems to have stopped the messages and our sites are working again.

Did I cause some other problem? Was this a bad idea?

Thanks...

BrockBoland’s picture

Needs issue summary

guydaniels’s picture

Updating to drupal core 7.15 fixed this issue for me.

rooby’s picture

I can confirm that this is still a problem in drupal 7.17.

To reproduce:

  1. Add a vocab with a field on a node type.
  2. Set the field to display on teaser, but not the full view mode.
  3. Preview a node

Here is a drupal 7 version of the patch in #25.

I can confirm that it fixes the problem, and the fix looks good to me too.
I have not tested drupal 8 & have not confirmed the tests are aok so I won't RTBC this though.

marcingy’s picture

Assigned: marcingy » Unassigned
JimmyAx’s picture

This one has in some way been fixed for Drupal 8. I can not reproduce it. It's still valid for 7 though. The patch is fixing it, so throwing it at testbot to verify. The code is still the same as done by marcingy in #25 so all credit goes to him.

Status: Needs review » Needs work

The last submitted patch, drupal-node_preview_errors-1289336-test-and-fix.patch, failed testing.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev

Looks like #1852966: Rework entity display settings around EntityDisplay config entity may have fixed this as a side effect, but I think we should get the tests here into Drupal 8 first to be sure (and to prevent future regressions).

JimmyAx’s picture

Status: Needs work » Needs review
FileSize
5.3 KB
JimmyAx’s picture

Changing version to test patches for 7.x.

JimmyAx’s picture

Version: 7.x-dev » 8.x-dev

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Needs backport to D7

The last submitted patch, 1289336-preview-node-field-attach-test-and-fix-drupal7.patch, failed testing.

JimmyAx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1289336-preview-node-field-attach-test-and-fix-drupal7.patch, failed testing.

JimmyAx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1289336-preview-node-field-attach-test-and-fix-drupal7.patch, failed testing.

JimmyAx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs backport to D7

The last submitted patch, 1289336-preview-node-field-attach-test-and-fix-drupal7.patch, failed testing.

JimmyAx’s picture

Version: 8.x-dev » 7.x-dev

Argh it was trying to apply the 7.x patch to 8.x

JimmyAx’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs backport to D7

Status: Needs review » Needs work

The last submitted patch, 1289336-preview-node-field-attach-test-and-fix-drupal7.patch, failed testing.

JimmyAx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1289336-preview-node-field-attach-test-and-fix-drupal7.patch, failed testing.

JimmyAx’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Needs backport to D7
JimmyAx’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Reviewed & tested by the community

There we go. Going to be bold and mark this as RTBC. Feel free to change it if something is wrong.

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Needs review

You're not supposed to RTBC your own patch.

greg.1.anderson’s picture

The workaround in #16 worked for me: I had a taxonomy term field that was shown in the teaser mode, but not in the other modes. Hiding it in all view modes made this bug go away, which was sufficient for me, but would not be sufficient if you really did desire to have a taxonomy term shown in one mode but not the other.

The patch in #36 works great on Drupal 7.21. I'd RTBC this, but I didn't have time to test Drupal 8.

no_commit_credit’s picture

Re-uploading to make it clear which is the D8 patch, plus a few minor comment style tweaks.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

The test looks correct to me, and #36 demonstrates that it provides coverage for the bug in D7. As a reminder, the bug is fixed in D8 and we are just forward-porting the test coverage for it.

RTBC assuming #54 passes (which it should).

greg.1.anderson’s picture

Version: 8.x-dev » 7.x-dev

As near as I can tell, today's D8 works fine without this patch. I followed the steps on #19 / #30, and found that preview still works, even with a taxonomy term that is visible in the teaser but not in the default view mode. Adjusting the version and status so that this can be considered for inclusion in D7. Please adjust the issue status if there is still need for this in D8--I don't believe there is.

Edit: Would have left this off if I had seen #55 first. Thanks, @xjm!

xjm’s picture

Version: 7.x-dev » 8.x-dev

Needs to go in 8.x first, even if it's just the test. :)

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs issue summary update, -Needs backport to D7

The last submitted patch, node-1289336-tests-d8.patch, failed testing.

JimmyAx’s picture

Status: Needs work » Needs review

#54: node-1289336-tests-d8.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, node-1289336-tests-d8.patch, failed testing.

JimmyAx’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Needs backport to D7

#54: node-1289336-tests-d8.patch queued for re-testing.

JimmyAx’s picture

Status: Needs review » Reviewed & tested by the community

There we go. There were random test failures in other tests causing the fails.

xjm’s picture

#54: node-1289336-tests-d8.patch queued for re-testing.

xjm’s picture

Issue tags: +Quick fix
xjm’s picture

Title: Calling node_view for the same node with muliple view modes on the same page does not correctly attach fields » D8 test coverage: Calling node_view for the same node with muliple view modes on the same page does not correctly attach fields

As per #55.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x, thanks!

JimmyAx’s picture

Title: D8 test coverage: Calling node_view for the same node with muliple view modes on the same page does not correctly attach fields » Calling node_view for the same node with muliple view modes on the same page does not correctly attach fields
Status: Patch (to be ported) » Reviewed & tested by the community
Issue tags: -Needs backport to D7

If I'm not mistaken there's nothing to do here except for committing the patch in #36.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Someone else still needs to review it. :)

klokie’s picture

Status: Needs review » Reviewed & tested by the community

works for me, thanks.

David_Rothstein’s picture

David_Rothstein’s picture

I don't think the patch applies - it will need a reroll due to removal of t() from test assertion messages.

It also looks like there a number of code style issues?... none particularly major, but they were fixed in the D8 patch so I think the backport should fix them too if possible. See the interdiff in #54, plus things like this:

+    $vocabulary = (object)array(

should have a space after "(object)".

And this:

-    // Check that the preview is displaying the title and body.
+    //  Check that the preview is displaying the title, body and term.

is adding indentation for no reason.

(Those are the ones I noticed on a quick read-through.)

David_Rothstein’s picture

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

(Testbot didn't bump it back, but the tests failed for #36.)

jthorson’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.41 KB

Reroll of #36. No logic changes, other than removing t() from the testing asserts ... so setting back to RTBC (assuming tests pass).

jthorson’s picture

Incidently, I tested and validated this solves the reported PHP notice (well, not quite exact, as it also had an 'undefined index' report and different entity_extract_ids() line number, but very similar) on a Drupal.org D7 development site, as reported at #2077233: On project edit preview: Notice: Undefined index: taxonomy_term in taxonomy_field_formatter_view() during Drupal.org D7 qa testing.

Status: Reviewed & tested by the community » Needs work
jthorson’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.56 KB

Updated reroll, including the code style changes from the #54 interdiff and #71.

jthorson’s picture

Status: Reviewed & tested by the community » Needs work

And test failed.

jthorson’s picture

Issue tags: +drupal.org D7

Tagging, as this impacts the Drupal.org D7 upgrade.

jrao’s picture

#20/21 is more like it, #77 is incorrect because this is not limited to previews, it'll also happen if you change the view mode using hook_entity_view_mode_alter

jthorson’s picture

Status: Needs work » Needs review
FileSize
5.55 KB

This has to have been the longest 'quick fix' ever. :)

fietserwin’s picture

A while ago, I ran into issue: #1339708: 'Undefined index: taxonomy_term' with taxonomy reference field token

Looks like that issue is caused by this issue. Perhaps comment #2 and #4 can be used for a workaround for Drupal.org D7. A possible way of a real fix is hinted at in #4 of that issue.

David_Rothstein’s picture

Re #80, looks like this issue has been about node preview for most of its lifetime (although bounced around a bit). I think it would be fine to just fix that and create a new issue for anything else (if it doesn't already exist; there are some issues about hook_entity_view_mode_alter() already, I believe).

Otherwise, this looks pretty good - is it ready to go?

The missing space after "object" (from #71) is still there in one place, but I could fix that on commit :)

yched’s picture

I don't think I agree with patch #81 - running f_a_prepare_view() in preview is needed (and IIRC, not doing so would cause errors / warnings on taxo_ref fields, for example)

Agreed that #20/#21 seems a more reasonable approach.

jthorson’s picture

Happy to reroll #20/21, but does the re-entrant concern expressed in #23 then still apply?

jthorson’s picture

Status: Needs review » Needs work

The last submitted patch, 1289336_multiple-view-mode-test-only.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
0 bytes

D'oh!

jthorson’s picture

fietserwin’s picture

Status: Needs review » Needs work

Empty patches will PASS all tests (if head is not broken), but won't solve the bug :)

fietserwin’s picture

Status: Needs work » Needs review

x-post

Status: Needs review » Needs work

The last submitted patch, 1289336_multiple-view-mode-test-only.patch, failed testing.

fietserwin’s picture

+++ b/modules/field/field.attach.inc
@@ -1145,7 +1149,9 @@ function field_attach_prepare_view($entity_type, $entities, $view_mode, $langcod
+      $entity->_field_view_prepared = array(
+        $view_mode => TRUE,
+      );
     }

This will overwrite current contents of _field_view_prepared, I would have expected:
$entity->_field_view_prepared[$view_mode] = TRUE;

But looking at other usages of _field_view_prepared, I see:
- an unset of that field in field_attach_view() (with as comment "in case the same entity is displayed with different settings later in the request.").
- Token module in contrib does the same, thus might also need to be warned when a patch along these lines gets in.
- field_view_field() in field.module checks for empty. It should check for the view mode as well. if the $display param is indeed a view mode. According to the documentation it may also be an array. how to accommodate for that I don't know.

I guess this property (_field_view_prepared) should either be treated as an array everywhere or nowhere and in the first case, needed to solve this issue, it should not be unset as a whole.

marcingy’s picture

There is only ever one view mode that prepared at any one point in time, which is why the array is re-initalised. The idea of the patch is simply to track what mode is currently rendered. Now it might be that you can store multiple rendered modes but that is a lot bigger of change and highly unlikely to be able to be done in D7 than simply tracking the current view mode 'state'.

jthorson’s picture

Status: Needs review » Needs work
FileSize
4.45 KB

Hmmm ... let's try and make the test fail for the *right* reasons (instead of relying on a non-existent entity_create() function) ... after which hopefully the fix will work.
I have no idea where the DBLog fail (table variable already exists) came from.

jthorson’s picture

Status: Needs work » Needs review

Triggering bot

The last submitted patch, 1289336_multiple-view-mode-with-test_v3.patch, failed testing.

jthorson’s picture

Getting rid of one last D8-ism.

jthorson’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1289336_multiple-view-mode-with-test_v4.patch, failed testing.

David_Rothstein’s picture

I don't think I agree with patch #81 - running f_a_prepare_view() in preview is needed (and IIRC, not doing so would cause errors / warnings on taxo_ref fields, for example)

Per Damien's comment in #23, isn't the extra field_attach_prepare_view() call unnecessary since it already gets called (with the correct view mode) later on?

tstoeckler’s picture

+++ b/modules/node/node.test
@@ -457,10 +457,72 @@ class PagePreviewTestCase extends DrupalWebTestCase {
+    $term = entity_create('taxonomy_term', array(

There's still one entity_create() left.

jthorson’s picture

Here's another attempt at the #21 reroll ... I'll let those more in the know discuss which is the proper approach, but in the meantime will try to get both approaches green so that either is ready to apply once a decision is made.

jthorson’s picture

Status: Needs work » Needs review
yched’s picture

Oh gee - I shouldn't chime on patches only on return from vacation and without taking the time to look at actual code and read up on issues a bit...

Yes, as @Damien pointed in #23, the right fix is to remove the f_a_prepare_view() call in node_preview(), it has nothing to do here, and that was how the issue got fixed in D8 (as a side affect of a larger refactor).
In short, #81 was right, and my #84 was full of crap.

Really sorry for adding to the confusion, and pushing a series of rerolls on @jthorson :-(
/me slaps himself in the face

basic’s picture

Status: Needs review » Reviewed & tested by the community

It sounds like #81 is ready and is the correct approach?

David_Rothstein’s picture

Cool, sounds like everyone is on the same page then! I'll leave this for a bit just in case, but I think we can commit it.

I'm not sure the other patch is wrong by the way (maybe it makes sense to allow multiple node_view() calls on the same $node to actually work?), although that sounds like more of a feature request than anything else. For the person who mentioned that errors can also happen when you change the view mode using hook_entity_view_mode_alter(), I think that's probably #1968348: (Change notice update) hook_field_formatter_prepare_view does not make use of hook_entity_view_mode_alter causing major errors?

DamienMcKenna’s picture

Title: Calling node_view for the same node with muliple view modes on the same page does not correctly attach fields » Calling node_view for the same node with multiple view modes on the same page does not correctly attach fields

Typo in the issue title ;-)

drumm’s picture

Issue tags: -Quick fix, -drupal.org D7 +Drupal.org 7.1, +affects drupal.org

Committed #81 to Drupal.org's BZR repo.

I'm also taking the liberty of removing the "quick fix" tag.

drumm’s picture

Issue summary: View changes

first pass to try and summarize the state of the issue. still confused about the approach to take for d7.

David_Rothstein’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed #81 to 7.x (with the minor code style fix mentioned in #83) - thanks! http://drupalcode.org/project/drupal.git/commit/850c6e3

David_Rothstein’s picture

Issue tags: +7.25 release notes
David_Rothstein’s picture

Status: Fixed » Reviewed & tested by the community
Issue tags: -7.25 release notes
FileSize
5.81 KB

I had to roll this back since it seems to have caused a test failure in HEAD...

Let's reupload the originally committed patch and see what happens.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 112: 1289336-112.patch, failed testing.

joachim’s picture

Component: node.module » ajax system

The way in this affected d.org is explained here: https://drupal.org/comment/8465305#comment-8465305 -- and might be of use when updating the issue summary.

tvn’s picture

Issue tags: -Drupal.org 7.1
Related issues: +#2191619: New on-page comment form is deleting all hidden files :(
drumm’s picture

Attached is the version of this patch we currently have deployed on Drupal.org. We are also running with #2001308: Node preview removes file values from node edit form for non-displayed items, which causes a merge conflict with this patch.

David_Rothstein’s picture

cristiroma’s picture

În #112, I suspect the test assertion is wrong:

$this->assertText($edit[$term_key], 'Term displayed.');
instead of 
$this->assertText($this->term->term, 'Term displayed.');
 
marcoka’s picture

i have the same problem as described by janipuni. reenabling the displaying of the fields again also resolves the error.
will test the patch now.

jerry’s picture

I just encountered the same thing with 7.35, and displaying the taxonomy term field had no effect. The patch provided by drumm in #116 worked for me, though it needs a re-roll now.

marcoka’s picture

reporting back: the patch worked for me too. errors gone.

jerry’s picture

Issue tags: +Needs reroll
matglas86’s picture

Issue tags: -Needs reroll
FileSize
5.57 KB

Reroll done against 7.37

dcam’s picture

Status: Needs work » Needs review

Activating Testbot

drumm’s picture

Status: Needs review » Reviewed & tested by the community

This has been working well for Drupal.org.

jerry’s picture

This still applies properly to, and works on, D7.39. Could we please get this committed for the next release?

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Running this locally I consistently get the same failure that the testbot showed previously on the patch we had to roll back (see #112; the failure is on one, or sometimes more than one, of the "Term displayed" assertions).

The patch hasn't changed since then either, so there is some kind of problem with the tests here still that we need to figure out (maybe an intermittent problem, which is why we've seen the testbot pass this sometimes?).

I think @cristiroma might be on to something in #118, or more specifically:

+    $edit[$term_key] = $this->term->tid;
....
+    $this->assertText($edit[$term_key], 'Term displayed.');

Shouldn't we be asserting the taxonomy term name is on the page, not the term ID (the term ID is just an integer, right, which wouldn't appear by design but maybe does appear randomly for a wholly unrelated reason)?

Also:

+    // Add a term to the vocabulary.
+    $term = (object)array(

Minor issue, but not sure why this code style issue slipped back in when it was previously fixed above. There should be a space after "(object)".

David_Rothstein’s picture

Title: Calling node_view for the same node with multiple view modes on the same page does not correctly attach fields » Calling node_view for the same node with multiple view modes on the same page during node preview does not correctly attach fields
Component: ajax system » node system
Priority: Normal » Major

Changing title since in the end we went back to only fixing node preview issues here.

Also raising priority since the issue summary indicates this breaks node preview entirely when it happens.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
5.24 KB
5.68 KB
2.03 KB

Completely untested patch based on the review above which might fix the problem.

However, I'm not sure if it makes sense yet or if there's more that needs to be changed.

The last submitted patch, 129: 1289336-129-TESTS-ONLY.patch, failed testing.

drumm’s picture

Status: Needs review » Reviewed & tested by the community

Looks like this is good now, except for the tests, this is the same as #123, which is what we're running on Drupal.org. I'll update to #129.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.42 release notes

Committed to 7.x - thanks!

Status: Fixed » Closed (fixed)

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