Comments are now fieldable, but currently does not provide informations to let a Field UI (be it CCK HEAD or the core patch in #516138: Move CCK HEAD in core) tie its admin pages.

From #504666-15: Make comments fieldable :

UI-wise, where would we put the 'manage fields' screen for comments on node type 'article' ?

First naïve idea would be to move the 'Comment settings' fieldgroup at the bottom of the node type edit form into it's own primary tab.
admin/build/node-type/article would then have primary tabs : Edit | Comments | Manage Fields | Display Fields
And 'Comments' would have three secondary tabs : Settings | Manage Fields | Display Fields

The current Field UI pages kind of assume they live as primary tabs and thus have 'secondary tab' level available for direct access to field configure pages, but I'm not sure we'll want to keep those anyway.
This would mean paths like admin/build/node-type/page/comments/fields/body/remove (8 parts)

Note: this is an issue for core, whether Field_UI itself gets in core or not. The current core admin UI for comments and node types doesn't provide an obvious place to attach the UI pages.

CommentFileSizeAuthor
#24 comment_field_ui-537750-24.patch10.9 KByched
#15 drupal.comment-field-ui.15.patch6.11 KBsun
#13 drupal.comment-field-ui.12.patch4.47 KBsun
#5 comment_field_ui-537750-5.patch4.4 KByched
#5 comment_field_ui.png11.36 KByched

Comments

SeanBannister’s picture

SeanBannister commented

Would it even be necessary to have a new tab. How about utilizing the existing "Manage Fields" and "Display Fields" pages. At the top of these pages would be Node fields as they currently are, but below this with a heading could be "Comment Fields".

WorldFallz’s picture

WorldFallz commented
Issue tags: +Usability, +D7UX

Took me a while to track down why I couldn't figure out how to add fields to comments with a fresh checkout of head, lol. Also took me a while to find this issue. Adding tags.

joshmiller’s picture

joshmiller
he/him
Primary language English
Location Indianapolis, Indiana, USA
commented
Status: Active » Closed (duplicate)

Marking duplicate

This is a small part of a bigger issue over #608118: Where can we tie a generic Field UI?

Lets focus the fire over there.

nadavoid’s picture

nadavoid commented

.

yched’s picture

yched commented
Title: Where can we tie a UI for fields on comments ? » Field UI for comments
Priority: Normal » Critical
Status: Closed (duplicate) » Needs review
StatusFileSize
new comment_field_ui.png11.36 KB
new comment_field_ui-537750-5.patch4.4 KB

#608118: Where can we tie a generic Field UI? won't be fixed this release, let's reopen this.
Comments are fieldable yet there is still no UI to use it.

Attached patch adds the 'Fields' and 'Display' tabs for comments next to the ones for the node type itself - see screenshot.

It also adds 'author' and 'subject' as 'extra fields', so they can be reordered as regular field components.

This required some hardcoded comment-specific help in 2 places in field / field_ui (will boil down to only 1 after the changes needed for #614030: Cannot edit fields for taxonomy terms get in). Not that I'm too happy with it, but that's the best I could find given how primary tabs work.

yched’s picture

yched commented
Issue tags: +Needs usability review

tag

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/field/field.module	11 Jan 2010 22:36:52 -0000
@@ -736,7 +736,13 @@ function field_extract_bundle($obj_type,
+    $bundle_name = $bundle->{$info['bundle keys']['bundle']};
+    // Handle the specific case of comments, for which the bundle name received
+    // is the name of the node type.
+    if ($obj_type == 'comment' && strpos($bundle_name, 'comment_node_') !== 0) {
+      $bundle_name = 'comment_node_' . $bundle_name;
+    }
+    return $bundle_name;

Hm... I could imagine that there may be other modules that want to do something like Comment module does. ;)

Any generic idea? Perhaps a simple alter hook?

Powered by Dreditor.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

That screenshot looks great to me. That article command center is really coming together.

I think this is fine as is. If yched wants to keep working on it to remove comment speciifc code in field api, thats fine but not required. Lets get the functionality and usability in now.

yched’s picture

yched commented

re sun #7: "Any generic idea? Perhaps a simple alter hook?"
Well, the generic idea would be to turn the $entity_info['bundle_keys']['bundle'] property into a callback based approach.

Like, "Hey, your damn menu_loader for the %foo_bar placeholder that is in the path where you asked me to tie the Field UI tabs just gave me this [$node_type|$vocabulary|$whatever] object I can't make any sense of, you'd be kind to turn it into a bundle string name that I can understand". Tricky to explain in the PHPdoc for hook_entity_info :-).

We can go that way, if there's a feeling it's a better road. Personally, I think I can live with the idea that sticking sets of Field UI tabs for different entities in the same set of primary tabs is a core privilege.

Bojhan’s picture

Bojhan commented

I am not sure, obviously this doesn't look good - the optimal number of tabs is around 3 to 4. Anything above that makes it look cluttered. The reason why we didn't show these tabs on the Taxonomy term page, we are now to introduce it at a different place with much higher impact.

Obviously we didn't solve "Where can we tie a generic Field UI?" because its a very though issue. This seems to be a way around that, and just putting "any" solution in. I don't have any real solutions, but we should at least do....

yoroy’s picture

yoroy commented

yoroy:Bojhan: ping
01:23am
Bojhan:yoroy: pong
01:24am
yoroy:Bojhan: saw http://drupal.org/files/issues/comment_field_ui.png?
01:24am Bojhan:writing comment now
01:27am
yoroy:Bojhan: those added comment tabs did actually make something clear about the other ones: http://skitch.com/yoroy/nxynt/comment-field-ui.png-png-image-1047x472-pi...
Bojhan:yoroy: lol, did you write that in your comment or? Because thats in the comment I am writing just now :P?
01:28am
yoroy:really?
yoroy:no, this is live skitch
yoroy:dude
Bojhan:yoroy: yhea, lol
yoroy :) magic

Bojhan:yoroy: Wait let me scrap it from my comment, you post your skitch - much better explanation.

Only local images are allowed.

yched’s picture

yched commented

re yoyoy #11:
"[node type name] fields | [node type name] display" is not possible, content type names can be arbitrarily long.
Besides, that's already the title of the page, do we need to be that explicit ?

re Bojhan #10:

The reason why we didn't show these tabs on the Taxonomy term page...

I don't think I see what you refer to.

Alternate proposal: remove 'Manage' from regular Field UI tabs (Manage fields, Manage display) - on all pages where they currently appear (vocabularies, nodes, users), and go here with
Edit | Fields | Display | Comment Fields | Comment display
Reduces overall length, but not the # of tabs, which I don't have a solution for.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new drupal.comment-field-ui.12.patch4.47 KB

We won't find a totally awesome solution, but I simply meant this.

yched’s picture

yched commented

@sun: yup, works too. Right now I'm not sure what the phpdoc of that hook would look like :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal.comment-field-ui.15.patch6.11 KB

I wondered that, too! ;)

aspilicious’s picture

aspilicious commented

There is something very wrong....

I had some warnings (2 to be correct), on the reports/fields page. Like I mentioned in #679978: "notice: undefined index: admin ... " at 'reports' -> 'field list'.
I think Yched told me those would be fixed when this issue was fixed.
I applied this patch and the warnings DOUBLED.

Two options:
1) Something wrong with this patch (I don't think so)
2) Something else wrong with Field_ui module...

In my eyes every error message/notice have to be eliminated before friday.
But who am I.

yched’s picture

yched commented

@aspilicious: you need to clear your site's caches after applying.
In fact, it sems menu cache gets cleared and rebuilt before entity_info cache is cleared, so you actually need to clear your caches twice in a row in order to see the new 'comment' tabs on admin/structure/types/manage/article ;-)
But one cache clear should be enough to get rid of the warnings in admin/reports/fields.

aspilicious’s picture

aspilicious commented

Thnx yched!

Ok this works :)

AND I can mark that other one as fixed!

WHIIEEEE me happy!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

So are we RTBC again?

dries’s picture

dries commented

No more screenshots?

yched’s picture

yched commented

@Dries: Screenshot hasn't changed since #5: http://drupal.org/files/issues/comment_field_ui.png

As I said in #12, there's an alternate proposal for tab titles:
Edit | Fields | Display | Comment fields | Comment display
instead, of with current patch:
Edit | Manage Fields | Manage Display | Comment fields | Comment display
That would imply removing the 'Manage' verb from Field UI tabs on other fieldable entities as well.

yched’s picture

yched commented

Additionally, the warnings on admin/reports/fields got reported again in #686938: Undefined index: admin in _field_ui_bundle_admin_path() (caused by fields on entity types that don't provide a path for their Field UI).

I hope we can give a UI to 'Fields on comments' here, but we need to fix the bug anyway, there might be other cases in contrib. Posted a patch over there.

yched’s picture

yched commented
Assigned: Unassigned » yched
Status: Reviewed & tested by the community » Needs work

Just had an illumination today: the menu system builds the tabs hierarchy looking at *anonymized* path structure, disregarding placeholder names.
Meaning, while node-type pages live under
admin/structure/types/manage/%node_type
we can stick Field UI tabs under
admin/structure/types/manage/%comment_node_type/comment
and use a different menu loader callback, while still being a primary tab of the parent path.

No time to roll a patch tonight, but we should be able to get rid of the ugliness discussed in #5-#7-#13-#14 (to read bundle name 'comment_node_article' when receiving menu argument 'article').

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new comment_field_ui-537750-24.patch10.9 KB

So, patch is two parts:

- comment.module : Exposes the paths for Field UI on comments. They use a different placeholder for the 'node type' part of the path, so that a dedicated menu loader callback can retrieve a *comment* bundle when the URL contains a node type. Nice and simple.
Also exposes the comment 'extra fields' (author, subject) to play nicely with d-n-d reorder.

- field_ui.module: Fixes the %field_ui_menu loader. That part is actually the fix for #614030: Cannot edit fields for taxonomy terms (see over there for explanations), but it's also needed there. In fact, it's needed for any entity type whose bundle names are not found exactly in the URL. The current works for nodes and users, but not for taxo terms and comments.

Screenshot is the same than in #5 above.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

yoroy’s picture

yoroy commented

I think this works.

Tabs aren't ideal, but this approach does kind of match how content and comments are presented in admin/content, too:

Only local images are allowed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

This looks great. The only WTF part for me was this:

+++ modules/comment/comment.module	20 Jan 2010 21:03:31 -0000
@@ -120,6 +117,19 @@ function comment_entity_info() {
+        'path' => 'admin/structure/types/manage/%comment_node_type/comment',
+        'bundle argument' => 4,
+        'real path' => 'admin/structure/types/manage/' . str_replace('_', '-', $type) . '/comment',
+        'access arguments' => array('administer content types'),

@@ -2499,6 +2546,13 @@ function comment_ranking() {
+
+  // Adjust the Field UI tabs on admin/structure/types/manage/[node-type].
+  // @see comment_entity_info()
+  $items['admin/structure/types/manage/%comment_node_type/comment/fields']['title'] = 'Comment fields';
+  $items['admin/structure/types/manage/%comment_node_type/comment/fields']['weight'] = 3;
+  $items['admin/structure/types/manage/%comment_node_type/comment/display']['title'] = 'Comment display';
+  $items['admin/structure/types/manage/%comment_node_type/comment/display']['weight'] = 4;

Hm? Why does comment module have to alter menu paths that it set up itself? Does this point to a deficiency in the API that we need to fix here while we're at it?

Powered by Dreditor.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Well. In addition to a general sigh at re-implementing much of hook_menu() in hook_entity_info() except slightly differently, but this was already established. :P

yched’s picture

yched commented

re #27-#28:

Why does comment module have to alter menu paths that it set up itself?

Comment doesn't actually set the menu paths, it tells field_ui where they should live, and field_ui sets them. Avoids lots of code duplication / hassle / errors if this was up to each entity module.
When writing this part (field_ui_menu() + admin paths info in hook_entity_info()), hardcoding the weight and titles of the primary tabs sounded good enough. Turns out that the tabs for comments need a little more customization.

We could add additional properties in hook_entity_info() to specify the weights and titles to use for the tabs, but
- that would fall into "reimplementing much of hook_menu() in hook_entity_info()" - which is not really the case currently: only 'access arguments' is duplicated, because it's a sensitive info that field_ui cannot guess nor provide a one-fits-all default for. 'bundle argument' is not a duplicate of a hook_menu() property, its' additional data about the 'path' that field_ui cannot figure out on its own either (what's the part of your path that contains the 'bundle' placeholder, so that my page callbacks receive the bundle name)
- we cannot really tell in advance what additional tweaks contrib entity types will need in the future - weight and title now, what else two months from now ?

So having field_ui build its menu items with sensible defaults, that you can then menu_alter() to your heart's content, sounds like the best option.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Fair enough, then. Committed to HEAD. Thanks!

yched’s picture

yched commented

Revealed a hiccup that stayed hidden so far : #705146: Comment bundle 'label' == Node type name

Status: Fixed » Closed (fixed)
Issue tags: -Needs usability review, -Usability, -D7UX

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

Issue tags: +Needs usability review, +Usability, +D7UX

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.