Posted by c960657 on October 25, 2009 at 2:18pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field_ui.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
If you go to admin/structure/taxonomy/1/fields and add a field by entering a label and a field name, you are taken to admin/structure/taxonomy/1/fields/field_foo/field-settings, and that just returns a 404.
The problem is in taxonomy_entity_info(). The bundle argument entry indicates that %taxonomy_vocabulary argument — in this example "1" (the vid of the vocabulary) — is passed to field_ui_menu_load($field_name, $obj_type, $bundle_name), but that function expects the vocabulary machine name, not its vid.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| taxonomy-field-2.patch | 4.02 KB | Idle | Passed: 14525 passes, 0 fails, 0 exceptions | View details |
Comments
#1
Replicated the same problem:
Went to admin/structure/taxonomy/1/fields
Added field with a label, type: text
Goes to admin/structure/taxonomy/1/fields/field_testfield/field-settings - which is a Page not found (404)
I applied the patch reinstalled against the latest HEAD and it works. I can add and edit fields now.
#2
The problem is that the menu loader for the
admin/structure/taxonomy/%taxonomy_vocabulary/fields/%field_ui_menu/editpath doesn't get the expected parameters.The 1st loader, taxonomy_vocabulary_load(), gets the right params, but the 2nd loader, field_ui_menu_load() doesn't.
I'm pulling my hair on this, but at any rate, I really don't see how the proposed patch could work. Have you tried with two separate vocabularies, adding and editing fields on each of them ?
#3
So, the problem is that our %field_ui_menu loader,
field_ui_menu_load($field_name, $obj_type, $bundle), needs $bundle to be the %taxonomy_vocabulary argument *as loaded by it's own taxonomy_vocabulary_load() loader*.But _menu_load_objects()'s 'load arguments' only provide the 'unreplaced' path part (here, '1'), which field_ui_menu_load() is unable to parse into a vocab id and then into a bundle name - because that's precisely the job of taxonomy_vocabulary_load().
I'd naively expect _menu_load_objects() to 'learn' as it progresses on the replacements of the menu parts. The attached patch, modifying _menu_load_objects(), seems to do the trick, but It might very well be a completely bad idea.
I'll ping chx and pwolanin about it.
#4
The last submitted patch failed testing.
#5
FWIW, I think the test failures can easily be fixed by updating image.module's image_effect_definition_load() and image_effect_load() to account for the new 'load arguments' convention, if the change is deemed OK.
#6
Yched asked me to hop in here since I did the wildcard loader for the Field UI. I'll review when I next get a chance.
#7
ugh - I quite dislike when have to do this
return strtr($bundle, array('-' => '_'));...#8
#617492: Cannot manage taxonomy fields has been marked as dup
#9
re pwolanin #7:
Nobody likes it ;-), but that's another issue: #617562: Remove underscore-to-dash conversion in path arguments for content types (D8)
#10
Is this still an issue?
#11
sounds like it is
#12
Still an issue.
So we cannot use menu loaders for the field-name (see comment #3) .
Attached patch removes the menu loader for the %field_ui_menu placeholder and goes with unnamed placeholders. The actual field and instance are loaded in the page callbacks.
Basically, replaces "[entity_bundle_path]/fields/%field_ui_menu/..." by "[entity_bundle_path]/fields/%/...".
Now, for a reason that is above me, this generates notices on the 'Manage fields' pages (e.g admin/structure/taxonomy/1/fields or admin/structure/types/manage/article/fields) :
Notice: Undefined offset: [N] in _menu_translate() (line 715 of menu.inc)- where N is the position of the previous %field_ui_menu placeholderOne error per path using this 'once-was-%field_ui_menu' placeholder.
FWIW, that part in _menu_translate() is :
for ($i = 0; $i < $router_item['number_parts']; $i++) {if ($link_map[$i] == '%') {
$link_map[$i] = $path_map[$i]; // <---- That line.
}
}
$path_map has less elements than $router_item['number_parts'], and notably no element for our %
The errors disappear if I set all the paths below /fields/% as MENU_NORMAL_ITEMS instead of MENU_LOCAL_TAB currently.
But then all those pages ('edit field settings' form, 'change widget type' form) lose their context (parenting tabs and breadcrumb trail), and we get :
instead of currently :
#13
Uploading less intrusive screenshots in my previous comment.
#14
The last submitted patch, field_ui_no_menu_loader-614030-12.patch, failed testing.
#15
Well, we need a fix for the taxo field subform pages anyway.
Attached patch:
- removes the %field_ui_menu and associated menu loader, as described in #12.
- switches all menu items below ".../field/%" to be regular MENU_NORMAL_ITEMS, to get rid of the warnings.
As a result, we lose context (breadcrumb + primary tabs) on the subform pages - see screenshots in #12.
If we can find a way to gain them back later, fine, if not, well so be it I guess...
#16
The last submitted patch, field_ui_no_menu_loader-614030-15.patch, failed testing.
#17
Paths in tests needed to be updated accordingly.
#18
Let's not change everything at this point. Here is the minimal fix for this issue.
#19
re #18: I've been there, doesn't work :-). Try to create a second vocabulary and see what happens with the 'Manage fields | Display fields' tabs : they all get attached to admin/structure/taxonomy (list of vocabs), instead of admin/structure/taxonomy/[vocab_id]
The menu path declared in taxonomy_menu() is admin/structure/taxonomy/%taxonomy_vocabulary.
So attaching Field UI pages to
'admin/structure/taxonomy/' . $vocabulary->vidwon't do the trick, the menu system doesn't recognize we're talking about the same path and doesn't build the tabs as expected.Sticking to #17.
#20
Well, ok. Then why not changing taxonomy paths, instead of field UI ones?
#21
You mean replacing
admin/structure/taxonomy/%taxonomy_vocabularyadmin/structure/taxonomy/%taxonomy_vocabulary/list
admin/structure/taxonomy/%taxonomy_vocabulary/edit
admin/structure/taxonomy/%taxonomy_vocabulary/add
by
foreach (taxonomy_get_vocabularies() as $vid => $vocab) {admin/structure/taxonomy/$vid
admin/structure/taxonomy/$vid/list
admin/structure/taxonomy/$vid/edit
admin/structure/taxonomy/$vid/add
}
Sounds like a bigger change, at this point, and also an unfortunate restriction on paths acceptable for Field UI.
#22
@yched: no, I meant replacing vid by machine_name in those paths.
#23
@Damz: Ah, sure. Would work.
But actually, I think my remark in #537750-23: Field UI for comments should let us fix this without changing legacy menu paths. We just need to use a different menu loader callback than %taxonomy_vocabulary in the Field UI paths, to read the vocab machine name (and thus Field API bundle) when given the vocab id.
#24
OK, so the fix is actually fairly simple.
As explained in #3 above, the menu loader for %field_ui_menu, field_ui_menu_load(), currently tries to load an $instance. That's not possible because this requires a bundle name, meaning it would need the result of menu loader for the first placeholder in the path (%node_type, %taxonomy_vocabulary...). Menu loaders don't learn from each other, they all receive the raw URL parts.
The current code only works for entity types whose bundle names are found exactly (modulo '_' / '-' replacement) in the URL. Happens to work for nodes and users, not for taxo terms or comments
The only thing our field_ui_menu_load() menu loader can do is load a $field (just needs a field name from the URL), and that's well enough. Field UI page callbacks receive a $field and fetch the $instance themselves (instead of currently receiving an $instance and fetching the $field themselves). Also means we cannot rely on a 'title callback' to provide the page title, because the page title is $instance['label'], and we don't have an $instance at this point.
Unlike my previous attempts, this patch doesn't lose primary tabs context when we are on a 'field edit' subform; UI is unchanged - except, it works.
Needs a menu rebuild after applying the patch, obviously.
#25
Forgot to add: this patch is a subpart of the patch in #537750-24: Field UI for comments - because both need the same fix.
#26
This is entirely field_ui's fault, BTW, the fix doesn't touch taxonomy.module
#27
The fix got in with #537750: Field UI for comments
#28
Automatically closed -- issue fixed for 2 weeks with no activity.
#29
I agree. Coming from #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc. (via admin_menu), where I'm trying to actually use those Field UI paths, the usage of $vid instead of $machine_name for Taxonomy bundles made no sense for me at all.
I therefore created #744258: admin/structure/taxonomy paths have to use machine_name, not vid
My patch over in #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc. basically reverts the removal of local task titles from this patch here, and it's not clear to me why those have been removed, as they're static.
Not sure whether we should re-open this issue.
#30
See #12.
They are static, but include the field label. Field labels are by instance. In order to load an instance you need a bundle name. Menu loaders cannot get a bundle name from the current path, for the reasons explained in #3 above, and that I just restated in #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc..
#744258: admin/structure/taxonomy paths have to use machine_name, not vid can be discussed, but I don't think we want to *require* fieldable entities to feature the bundle name in their admin paths (and : replicating the '_ to -' conversion that exists for node types ?). At least, I've tried to avoid this limitation so far.
[edit : besides, we know comments won't comply to that requirement, since the admin UI sits below the same base paths than node types admin - admin/structure/types/manage/[node-type]/... ]
#31
+++ modules/field_ui/field_ui.module 20 Jan 2010 20:40:39 -0000@@ -67,43 +67,32 @@ function field_ui_menu() {
$items["$path/fields/%field_ui_menu/field-settings"] = array(
- 'title' => 'Edit field settings',
With that, I meant the non-dynamic titles like this one. They are independent of any path arguments.
But perhaps it's better to continue over in #742318: Fields are editable regardless of whether an bundle instance exists, missing menu titles, etc....
Powered by Dreditor.
#32
I think those static titles were already superceded by a drupal_set_title($instance['label']) in the page callback itself. The patch just unified that fact (it seems the "widget type' subform did not comply to that pattern before the patch), and removed the unused 'title' properties (which, agreed, probably screws admin menu - that's an oversight)
#33
But, agreed, it's best to continue in that other issue.