Problem/Motivation
- field_ui_default_value_widget() does not pass the entity type to field_default_value_form().
field_default_form()
is supposed to pass the entity type to the following hooks and functions:- Any custom default value callback for the field instance (registered in
$instance['default_value_function']
) - hook_field_access()
- hook_field_widget_properties_alter()
- hook_field_widget_properties_ENTITY_TYPE_alter()
- Any custom default value callback for the field instance (registered in
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.
Comments
Comment #1
RobLoachWith 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.
Comment #3
RobLoachThe Drupal 8 patch looks good. Attached is David's Drupal 8 patch, and the Drupal 7 patch.
Comment #4
RobLoachSorry about this, I was wrong... The exceptions will hit in Drupal 8 too. Need to port that over to entity.module.
Comment #5
RobLoachHere it is with the bundle check for Drupal 8.
Comment #6
thekevinday CreditAttribution: thekevinday commentedWhile 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.
Comment #7
xjmHmm, isn't this sort of reverting #1067750: Let Field API fail in a tale-telling way on invalid $entity?
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedYes, 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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedRerolled 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 :)
Comment #10
xjmThis 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.
Comment #11
xjmHmm, we should add test coverage though.
Comment #12
chx CreditAttribution: chx commentedThis 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.
Comment #13
xjmFixing crosspost.
Comment #14
xjmI 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.
Comment #15
xjmLooking into a test for this.
Comment #16
xjmSo 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
).Comment #16.0
xjm(xjm) Added summary per chx's request.
Comment #17
xjmHere's a test for this that as a bonus should add some test coverage we can expand for various widget alters.
Comment #18
xjmYeah, needs a docblock there. Fixing that.
Comment #19
xjmAdding that docblock and cleaning out some cruft.
Comment #20
xjmComment #20.0
xjmUpdated issue summary.
Comment #21
xjmNote 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.Comment #22
xjmMaking args for the helper method a bit less WTF-y.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedNice, those tests look very good. Couple things:
Typo.
This should be declared as
protected $fields
at the top of the test class, I think.(minor) The extra space isn't necessary there.
Comment #24
xjmRe: #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.
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #26
xjmAh, 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.
Comment #27
xjmComment #28
yched CreditAttribution: yched commentedWorks for me. Many thanks, @xjm !
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedThe final version looks great to me also.
Comment #30
catchLooks good here too. Committed/pushed to 8.x, will need a re-roll for 7.x.
Comment #31
xjmStraight reroll for D7.
Comment #33
xjm#1346772: StatisticsLoggingTestCase->testLogging() fails with clean URLs in some environments again.
Comment #34
xjmComment #35
webchickLooks like a nice improvement. Thanks for the tests!
Committed and pushed to 7.x.
Comment #36.0
(not verified) CreditAttribution: commentedUpdated issue summary.