Problem/Motivation

As a result, the entity type is not set properly in the above functions for the default value widget, and hook_field_widget_properties_ENTITY_TYPE_alter() is not called for it at all.

Proposed resolution

  • Pass the instance entity type instead of NULL. Patch in #9 implements this change.

Remaining tasks

  • Needs tests.

User interface changes

  • None.

API changes

  • None.

Original report by David_Rothstein

The result is that various hooks/etc. invoked by field_default_form() do not get the entity type passed to them, even though they are documented as doing so.

The patch to fix it seems simple, and it applies to both Drupal 7 and Drupal 8.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

With the patch applied, I still get the exceptions. Doing some more checks on the bundle property fixed it. If the bundle is an empty string, it should assume the bundle is the entity type.

..... The attached is a Drupal 7 patch.

Status: Needs review » Needs work

The last submitted patch, entitybundle.patch, failed testing.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs backport to D7
FileSize
1.92 KB
695 bytes

The Drupal 8 patch looks good. Attached is David's Drupal 8 patch, and the Drupal 7 patch.

RobLoach’s picture

Status: Reviewed & tested by the community » Needs work

Sorry about this, I was wrong... The exceptions will hit in Drupal 8 too. Need to port that over to entity.module.

RobLoach’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
1.95 KB

Here it is with the bundle check for Drupal 8.

thekevinday’s picture

While trying to load the rules module admin page /admin/config/workflow/rules, the page crashed on a php exception:
EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7389 of /var/www/html/includes/common.inc).

I managed to track the problem down to a completely different module, field_permissions (which is odd that its being called on the rules module administration page) to field_permissions/field_permissions.module:178.

The problematic line is:
list($id, $vid, $bundle) = @entity_extract_ids($obj_type, $object);

There is a comment about why the @ was added and referenced this issue.
There was an exception warning worked around there, but I am having an exception error and not a warning.
The page does not load and the only thing on the screen is the exception (plus the bas.

I am using drupal 7 and after I applied your drupal 7 patch from #3, the problem went away and I was able to see the rules administration page.

xjm’s picture

David_Rothstein’s picture

Status: Needs review » Needs work

Yes, and we shouldn't do that :)

I don't actually understand what the newer patches have to do with the issue I originally filed... If modules are passing incomplete entities around, that's a bug with those modules, not core. In the case of Field Permissions (which may still have a bug here too) we can deal with that at #1326626: "EntityMalformedException: Missing bundle property on entity" error when editing a node.

For this issue, how about we go back to my original patch? That's "needs work" also because it won't apply, but probably just needs a simple reroll.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
715 bytes

Rerolled my original patch from the top of this issue, using your very nice instructions at http://xjm.drupalgardens.com/blog/rerolling-drupal-8-core-patches :)

xjm’s picture

Status: Needs review » Reviewed & tested by the community

This seems like a pretty straightforward fix. #6 also indicates that it solves outstanding issues for contrib.

Also cross-referencing #1042822: Developers need an $entity->entity_type property, which documents similar issues where the type is not available in a certain context.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

Hmm, we should add test coverage though.

chx’s picture

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

This issue needs tests and a summary. The summary should actually list and preferably link the hooks fired from field_default_form (or at least 1-2 of them) because unless you are some sort of field maven , you will have no idea we are talking of http://api.drupal.org/api/drupal/modules--field--field.api.php/function/... and their ilk.

I agree this is a bugfix 'cos the widget_form field hook I linked to indeed lists entity_type simply as being present.

xjm’s picture

Issue tags: +Needs tests

Fixing crosspost.

xjm’s picture

I think this should also allow us to finally add the entity type explicitly to $context for hook_field_widget_form_alter(). Maybe that's a followup issue.

Edit: Hmm, actually, field_multiple_value_form() would still not have them.

xjm’s picture

Assigned: Unassigned » xjm

Looking into a test for this.

xjm’s picture

So I guess the way to test this would be to create some field instance on a page node, then implement one of the various hooks in a test module and confirm that the alter has the entity type when visiting admin/structure/types/manage/page/fields/fieldname. There doesn't seem to be an existing test module for Field UI, but maybe we could use one from something else.

Edit: field_test.module should probably do the trick. It appears we don't have any test coverage whatsoever for any of the various widget alter hooks and callbacks, so might be worth opening that as a followup.

Edit 2: Okay, wow. field.test is packed with // TODO comments (not even using the doxygen-friendly @todo).

xjm’s picture

Issue summary: View changes

(xjm) Added summary per chx's request.

xjm’s picture

Here's a test for this that as a bonus should add some test coverage we can expand for various widget alters.

xjm’s picture

+++ b/core/modules/field_ui/field_ui.testundefined
@@ -643,3 +643,73 @@ class FieldUIManageDisplayTestCase extends FieldUITestCase {
+  function createFieldInstance($entity_type, $bundle, $widget_type, $field_name, $field_type = 'text') {

Yeah, needs a docblock there. Fixing that.

xjm’s picture

xjm’s picture

Assigned: xjm » Unassigned
Issue tags: -Needs tests
xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Note that hook_field_widget_form() itself gets the entity type from the instance rather than from the NULL argument fixed by this patch. An additional WTF is that field_multiple_value_form() does not get the entity type explicitly, nor get the entity itself at all.

xjm’s picture

Making args for the helper method a bit less WTF-y.

David_Rothstein’s picture

Nice, those tests look very good. Couple things:

  1. +      'description' => 'Test custom field widget hooks and callbacks on field aministration pages.',
    

    Typo.

  2. +    // Initialize field list.
    +    $this->fields = array();
    

    This should be declared as protected $fields at the top of the test class, I think.

  3. +  function testDefaultWidgetPropertiesAlter() {
    +
    +    // Verify that the size of the alter_test_text field is altered to 42.
    

    (minor) The extra space isn't necessary there.

  4. Overall, I do wonder if the createFieldInstance() method is actually adding much benefit here? It's providing a bunch of (optional) settings that don't look to me like they're ever used. The code might be a bit simpler and easier to follow if field_create_field() and field_create_instance() were used directly in the body of the test (and only called with the required + ['widget']['type'] parameters).
xjm’s picture

Status: Needs review » Needs work

Re: #4 -- There's only one optional parameter, actually. I was thinking of adding some tests to this same class and so didn't want to re-type repeatedly. I can put it inline if you think that's better, though.

I'll reroll.

David_Rothstein’s picture

I think I phrased #4 wrong. I meant that 'label', 'description', 'weight', etc. are optional parameters to field_create_instance(), and the tests don't seem to be using them for anything (just passing in random values). So it seems like those could be taken away.... And if they were taken away, the helper method wouldn't be doing much extra that you don't get from a direct call to field_create_instance()?

It's fine to leave it there if you think it's useful, though.

xjm’s picture

Ah, I see what you mean. That makes sense, since we're not really doing anything with those properties here and they are covered in tests elsewhere.

xjm’s picture

yched’s picture

Status: Needs review » Reviewed & tested by the community

Works for me. Many thanks, @xjm !

David_Rothstein’s picture

The final version looks great to me also.

catch’s picture

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

Looks good here too. Committed/pushed to 8.x, will need a re-roll for 7.x.

xjm’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
5.57 KB

Straight reroll for D7.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field-default-form-no-entity-type-1301522-d7-31.patch, failed testing.

xjm’s picture

xjm’s picture

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

Status: Reviewed & tested by the community » Fixed

Looks like a nice improvement. Thanks for the tests!

Committed and pushed to 7.x.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.