Problem:

Entities exposed by data_entity.module are marked fieldable, it is impossible to add fields using Fields UI.
For the module that allows changing table schemas, this may sound as overkill, but ability to add fields can prove useful (think fields like flag, fivestars and lot others)

Analysis:

Function field_ui_field_overview_form() fails to find admin path for data entity due to incomplete data in data_hook_entity_info (there is no bundle keys property)

Proposed solution

See attached patch (will be posted in the first comment)
Note that this patch only allows adding fields, entity edit form still won't display them.
I don't know if it's better to fix in a separate issue or this one

CommentFileSizeAuthor
#18 1952884.18.data_.field-ui-menu-tabs.patch14.32 KBjoachim
#16 data-field-ui-1952884-16.patch6.05 KBvalthebald
#4 data-field-ui-1952884-4.patch5.58 KBvalthebald
#3 data-field-ui-1952884-3.patch3.83 KBvalthebald
#2 data-field-ui-1952884-2.patch1.35 KBvalthebald
#1 data-field-ui-1952884.patch2.25 KBvalthebald

Comments

valthebald’s picture

valthebald
Location Sofia
commented
StatusFileSize
new data-field-ui-1952884.patch2.25 KB
valthebald’s picture

valthebald
Location Sofia
commented
StatusFileSize
new data-field-ui-1952884-2.patch1.35 KB

Removed my test code, which is not needed, actually

valthebald’s picture

valthebald
Location Sofia
commented
Status: Needs work » Needs review
StatusFileSize
new data-field-ui-1952884-3.patch3.83 KB

A little bit more tweaking of field callbacks, but as a result, Field UI is working (at least, for me)

valthebald’s picture

valthebald
Location Sofia
commented
StatusFileSize
new data-field-ui-1952884-4.patch5.58 KB

Found another problem. In data entity submit form, several important hooks were missing. As a result, non-trivial fields (like autocomplete taxonomy terms reference with autocreation) were not working.
Attached patch adds node_submit()-like processing

rooby’s picture

rooby commented

This patch fixes the masses of errors I get on the manage fields tab of my data tables:

Notice: Undefined index: in _field_ui_bundle_admin_path() (line 330 of /var/www/mysite/modules/field_ui/field_ui.module).
Notice: Undefined index: field_name in field_ui_field_overview_form() (line 338 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: field_name in field_ui_field_overview_form() (line 339 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: label in field_ui_field_overview_form() (line 345 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: label in field_ui_field_overview_form() (line 349 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: widget in field_ui_field_overview_form() (line 351 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: label in field_ui_field_overview_form() (line 358 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: field_name in field_ui_field_overview_form() (line 372 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: in field_ui_field_overview_form() (line 376 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: widget in field_ui_field_overview_form() (line 382 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: in field_ui_field_overview_form() (line 382 of /var/www/mysite/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: widget in field_info_max_weight() (line 802 of /var/www/mysite/modules/field/field.info.inc).
Notice: Undefined index: in _field_ui_bundle_admin_path() (line 330 of /var/www/mysite/modules/field_ui/field_ui.module).
Notice: Undefined index: widget in field_info_max_weight() (line 802 of /var/www/mysite/modules/field/field.info.inc).

joachim’s picture

joachim commented

I'm sure when I was using this module, I added fields to my entity tables and they worked fine when editing the data row entities too. I'll have to have a look at that project and see how it was done.

+++ b/data_entity/data_entity.module
@@ -63,17 +63,18 @@ function data_entity_entity_info() {
+      'bundle keys' => array('bundle' => 'type'),

This is assuming that our data table has a 'type' column.

EDIT: actually this is not needed at all. It's only relevant if bundles are objects. See http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...

+++ b/data_entity/data_entity.module
@@ -63,17 +63,18 @@ function data_entity_entity_info() {
+          'type'  => $entity_type,

This key is not mentioned in the docs.

+++ b/data_entity/data_entity.module
@@ -126,6 +127,48 @@ function data_entity_menu_alter(&$items) {
+    $items = $new_items;

That looks wrong -- clobbering the entire menu structure doesn't feel right at all! Can $menu be altered in place? Also, rather than iterating over the entire menu, can we not build the paths we need and grab them?

+++ b/data_entity/data_entity.pages.inc
@@ -93,24 +93,28 @@ function data_entity_entity_edit_form_validate($form, &$form_state) {
 function data_entity_entity_edit_form_submit($form, &$form_state) {

Changes to this function seem unrelated?

I'll also say now that this is a really big patch, and I'm a maintainer who no longer actively uses this module (#1796346: New maintainer(s) needed! :). So anything that can be done to simplify it / break it into different issues will be a big help!

rooby’s picture

rooby commented
Status: Needs review » Needs work

So far I just blindly patched and my errors went away.

If I can get a chance I will review the patch functionality properly.

diggingrelic’s picture

diggingrelic commented

+1 - This patch fixed all sorts of errors and issues I was having. I couldn't even access the "manage display" at all before this patch.

valthebald’s picture

valthebald
Location Sofia
commented
Status: Needs work » Active

First of, I am not sure if it's possible to split the issue into smaller ones - all parts are inter-related.

+++ b/data_entity/data_entity.module
@@ -63,17 +63,18 @@ function data_entity_entity_info() {
+      'bundle keys' => array('bundle' => 'type'),

this does not assume that data table has type column, it just informs that bundle key should be taken from the 'type' element of array returned by hook_entity_info():

+++ b/data_entity/data_entity.module
@@ -63,17 +63,18 @@ function data_entity_entity_info() {
+          'type'  => $entity_type,

despite of documentation, bundle keys can't be omitted. Actually, that was the first and obvious finding from inspecting multiple errors coming from the manage fields form.

+++ b/data_entity/data_entity.module
@@ -126,6 +127,48 @@ function data_entity_menu_alter(&$items) {
+    $items = $new_items;

I am also not 100% convinced that current implementation is ideal. I chose to bulk-replace whole $items array in hook_menu_alter() for 2 reasons:
1. Adding/removing indexes of large PHP array while traversing it sounds as even worse idea than rebuilding it.
2. I didn't want to hard-code all subpaths added by field_ui.module. Once again, I am not sure if that's the right solution.

+++ b/data_entity/data_entity.pages.inc
@@ -93,24 +93,28 @@ function data_entity_entity_edit_form_validate($form, &$form_state) {
function data_entity_entity_edit_form_submit($form, &$form_state) {

Changes to this function seem are related! If you attach text or integer fields (which is weird, by the way, having ability to add the same fields to data tables themselves), function can work unchanged, but if attached fields require preprocessing (i.e. files, auto-added taxonomy terms etc.), you need them

imclean’s picture

imclean commented

Related: #1538588: Add Entity Label field (Was: allow entity ref fields to point to data items)

Please review patch #20.

valthebald’s picture

valthebald
Location Sofia
commented

@imclean: both issues are stuck due to #1796346: New maintainer(s) needed. And new maintainers can't be introduced due to the same issue - noone can review/approve patches. Classic catch 22 :(

rooby’s picture

rooby commented

If others can test and review the patches joachim is still being very responsive.

I will try make some time to try out the patch mentioned in #10

valthebald’s picture

valthebald
Location Sofia
commented

Sorry for being unclear. I deeply appreciate joachim's work, and didn't want to offend him by my comment

rooby’s picture

rooby commented

No worries, I knew what you meant.

I just mean that it should still be possible for us to push things forward if we get some reviews. Even before a new maintainer has been found.

joachim’s picture

joachim commented

I'm not offended at all :)

As you say, I'm able to find the time to review patches by eye and commit them when other people have tried them out and reviewed them in more depth.

valthebald’s picture

valthebald
Location Sofia
commented
Status: Active » Needs review
StatusFileSize
new data-field-ui-1952884-16.patch6.05 KB

Per suggestion in #6, I rewrote hook_menu_alter() to modify only items that we need, not rebuild $items array from the scratch

cayenne’s picture

cayenne commented

UPDATE: Error gone. Disable and re-enable module seems to have done the trick (yeah, I know, I shoulda...)
Still not seeing references in related note, but will continue to whittle away at that.

Using the dev version, with both the most current patch and the earlier-posted one, I still get the failed "Manage Display" tab and the host of errors as follows"

Notice: Undefined index: in _field_ui_bundle_admin_path() (line 325 of /var/www/cca/modules/field_ui/field_ui.module).
Notice: Undefined index: field_name in field_ui_display_overview_form() (line 948 of /var/www/cca/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: display in field_ui_display_overview_form() (line 949 of /var/www/cca/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: in field_ui_display_overview_form() (line 956 of /var/www/cca/modules/field_ui/field_ui.admin.inc).
Notice: Undefined index: label in field_ui_display_overview_form() (line 959 of /var/www/cca/modules/field_ui/field_ui.admin.inc).
....

I created a simple two-field table for testing. Am running the dev version of Data here with all other modules up-to-date. I imagine it's related, but where I create an entity reference to a table entry in another node, there is no display on that node either.

Happy to help with any debug.

Michael

joachim’s picture

joachim commented
Issue summary: View changes
StatusFileSize
new 1952884.18.data_.field-ui-menu-tabs.patch14.32 KB

Thanks everyone for working on this, and sorry I've not been very involved :(

The patch in #16 looks like a lot of hard work went into it, and it contains what looks like a lot of cunning trickery and jumping through hoops to make the Field UI menu items work.

However, I've decided on a radical approach: we bite the bullet and define separate menu items for each data table. It's much simpler than any other approach we've tried so far.

It's a bit more work in page and form callbacks, because we no longer have the benefit of a menu loader, but I think on balance that is much simpler than the alternatives.

Here's a patch which I'll commit fairly soon.

joachim’s picture

joachim commented
Status: Needs review » Fixed

Committed. New alpha release shortly!

Status: Fixed » Closed (fixed)

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