#414328: Converting core modules to provide fields leaves them with no UI showed a consensus on moving the current Field UI in CCK HEAD into core, to make it easier for the community to grasp the current status of the UI and embrace the task of enhancing it.

As I said earlier on, I won't have the bandwidth for the additional tasks of debugging / adding missing features / polishing all by myself.
It's probably easier if I do the initial sorting and roll the initial patch, though...

So a couple questions before I get started:
- should this be a separate module (optional ? required ?), or be merged into field.module ?
- if separate module : what name and namespace ? Field UI / field_ui ?

FWIW, rolling this as a separate module will probably be easier and allow for a faster commit, and we can always revise afterwards...

Files: 
CommentFileSizeAuthor
#174 field_ui_menu_rebuild.patch608 bytesyched
Passed: 12436 passes, 0 fails, 0 exceptions
[ View ]
#169 516138-performance-issue_1.patch642 byteswebchick
Passed: 12191 passes, 0 fails, 0 exceptions
[ View ]
#164 516138-performance-issue.patch642 bytesDamien Tournoud
Failed: Failed to apply patch.
[ View ]
#162 516138-performance-issue.patch3.41 KBDamien Tournoud
Passed: 12411 passes, 0 fails, 0 exceptions
[ View ]
#160 516138-performance-issue.patch513 bytesDamien Tournoud
Passed: 12411 passes, 0 fails, 0 exceptions
[ View ]
#139 NewField.png34.07 KBGábor Hojtsy
#139 OldField.png73.19 KBGábor Hojtsy
#134 field_ui.patch116.7 KBquicksketch
Failed: Failed to apply patch.
[ View ]
#133 drupal-field-ui.patch116.72 KBsun
Failed: 11411 passes, 3 fails, 0 exceptions
[ View ]
#131 field_ui.patch115.37 KBquicksketch
Failed: 11411 passes, 3 fails, 0 exceptions
[ View ]
#128 field_ui.patch115.83 KBquicksketch
Failed: 12192 passes, 0 fails, 16 exceptions
[ View ]
#128 field-config.png37.04 KBquicksketch
#118 displayfields.png6.79 KBBojhan
#117 alignmentfieldsoverview.png36.06 KBBojhan
#117 alignmentfieldsoverviewbefore.png20.94 KBBojhan
#115 field_ui.patch115.83 KBquicksketch
Failed: 11966 passes, 0 fails, 16 exceptions
[ View ]
#113 fieldsinstructure.png34.9 KBBojhan
#112 fields_overview.png53.78 KBBojhan
#112 fields_tabs.png12.99 KBBojhan
#112 fields.png34.9 KBBojhan
#112 existingfieldui.png41.38 KBBojhan
#111 field_ui.patch115.9 KBquicksketch
Failed: 12152 passes, 1 fail, 16 exceptions
[ View ]
#110 field_ui.patch123.49 KBquicksketch
Failed: Failed to apply patch.
[ View ]
#109 field_ui.patch16.6 KBquicksketch
Failed: Failed to install HEAD.
[ View ]
#106 field_ui-516138-106.patch127.34 KByched
Failed: 11359 passes, 3 fails, 0 exceptions
[ View ]
#105 field_ui_instance_prerender.txt888 bytesquicksketch
#99 field_ui-516138-99.patch126.54 KByched
Failed: Failed to apply patch.
[ View ]
#97 ArticleField.png97.28 KBGábor Hojtsy
#97 BodySummary.png42.18 KBGábor Hojtsy
#97 CupsRemoved.png42.2 KBGábor Hojtsy
#97 CupsBorked-1.png112.04 KBGábor Hojtsy
#94 field_ui-516138-94.patch126.76 KByched
Failed: Failed to apply patch.
[ View ]
#91 field_ui-516138-91.patch126.38 KByched
Failed: Failed to apply patch.
[ View ]
#90 field_ui-516138-90.patch126.33 KByched
Failed: Failed to apply patch.
[ View ]
#89 field_ui-516138-89.patch126.44 KByched
Failed: Failed to apply patch.
[ View ]
#84 field-ui-516138-84.patch35 KBbjaspan
Failed: Failed to install HEAD.
[ View ]
#76 field_ui.patch126.5 KBquicksketch
Failed: Failed to apply patch.
[ View ]
#74 field_ui.patch122.9 KBquicksketch
Failed: 11899 passes, 2 fails, 0 exceptions
[ View ]
#73 field_ui_69_to_73.txt3.07 KByched
#73 field_ui-516138-73.patch134.85 KByched
Failed: Failed to apply patch.
[ View ]
#69 field_ui-516138-69.patch135.09 KByched
Failed: Failed to apply patch.
[ View ]
#69 field_ui_67_to_69.txt1.89 KByched
#67 field_ui-516138-66.patch135.02 KBbangpound
Failed: Failed to apply patch.
[ View ]
#61 field_ui-516138-61.patch134.49 KByched
Failed: 11032 passes, 3 fails, 0 exceptions
[ View ]
#52 field_ui-516138-52.patch134.25 KBbangpound
Failed: 11037 passes, 8 fails, 16 exceptions
[ View ]
#49 field_ui-516138-48.patch132.16 KByched
Failed: 11037 passes, 8 fails, 16 exceptions
[ View ]
#45 field_ui-516138-45.patch135.75 KByched
Failed: 11858 passes, 8 fails, 16 exceptions
[ View ]
#41 field_ui-516138-41.patch134.9 KByched
Failed: 11673 passes, 8 fails, 16 exceptions
[ View ]
#38 field_ui-516138-38.patch139.03 KByched
Failed: Failed to apply patch.
[ View ]
#35 field_ui-516138-35.patch130.69 KByched
Failed: 11858 passes, 8 fails, 16 exceptions
[ View ]
#32 field_ui-516138-32.patch131.08 KBbangpound
Failed: 11037 passes, 8 fails, 16 exceptions
[ View ]
#25 field_ui-516138-24.patch131.82 KByched
Failed: Failed to apply patch.
[ View ]
#23 field_ui-516138-23.patch131.82 KBbangpound
Failed: Failed to apply patch.
[ View ]
#20 field_ui-516138-20.patch131.81 KBbangpound
Failed: Invalid PHP syntax in modules/field_ui/field_ui.api.php.
[ View ]
#16 field_ui-516138-16.patch131.58 KByched
Failed: Invalid PHP syntax in modules/field_ui/field_ui.api.php.
[ View ]

Comments

quicksketch’s picture

Yay, this opens up exciting possibilities for core, such as removing/replacing profile.module and upload.module. Under normal circumstances I'd say that we should merge the UI into Field module, since we don't have any precedent of split administrative UIs anywhere else in Drupal core. However, Fields is a particularly complex and extensive UI, plus there is active work going on to rewrite it almost entirely. Speaking from experience it's almost impossible to completely remove one UI and replace it with another entirely through hook_menu_alter(), plus it's wasteful to attempt to do so anyway. So for these reasons I think keeping it separate would be acceptable.

bangpound’s picture

I prefer a separate, optional module. I would even keep 'CCK' as the namespace just for nostalgia.

yched’s picture

bangpound: ;-) being CCK-nostalgic is cool, but I really see no reason for the cck namespace entering core. Besides, it might be handy to still have that namespace available for the CCK D6 -> Field D7 upgrade.

quicksketch's hook_menu_alter argument in #1 is pretty compelling IMO.
So, going for a field_ui module. Stay tuned.

sun’s picture

Since we don't know what will happen and what can be achieved: field_ui.module.

webchick’s picture

Field UI module also has the benefit of being disableable once all your content types are set up. Less code load FTW.

I pinged Gábor for a status update on the spanky new Field UI, and he has his hands quite full already with various other D7UX tasks. It's looking doubtful that this will be completed much before code freeze, if at all. There is far too much important work in Field API I'd like to see completed before then, so I agree with this direction.

shunting’s picture

Separating field and field ui seems like separating form and content, to me -- that's a separation of concerns thing that done pretty rigorously elsewhere in Drupal, to our great benefit.

And doesn't separating the ui make it easier for new UIs to be contributed and swapped in? That can only be good, yes?

yched’s picture

"And doesn't separating the ui make it easier for new UIs to be contributed and swapped in ?"

Well, theoretically yes. But alternate UIs will probably require slightly different integration code from field types (hooks for settings forms...) so I'm not sure it will actually be doable. We don't want to end with stuff like "foobar field works for core Field UI and contrib Field_UI_NG but not for contrib Look_at_my_cool_Fields_UI_module" on projects page :-)

shunting’s picture

Yes, I can see that is a bad thing -- but why does it need to be so? If the hooks are structured and applied in a consistent manner? Sorry to be dense...

yched’s picture

"If the hooks are structured and applied in a consistent manner?"
Yes, hopefully so, but making predictions on yet unwritten modules is, er, difficult ;-)

shunting’s picture

The best way to predict the future is to invent it ;-)

bangpound’s picture

Yched:

Given the simplicity of hook_field_settings_form() in CCK now, I would be disappointed to see that be different for other CCK UIs. I hope the settings forms can be moved to Field API in core (not just CCK UI in core). If other UI modules need something more, what can't they accomplish with hook alters or a wrapper around the core setting forms?

Bojhan’s picture

So my opinion is that any new CCK UI proposal, is not going to tackle all the UI issues in time (especially form builder). So therefor moving CCK HEAD into core is our only choise. I wish it would be as modular as possible, being able to swap in a new UI once we got one that is able to cover all the current use cases.

"Since we don't know what will happen and what can be achieved: field_ui.module."

I think so as well, lets keep it separate and do our best to allow a completely different UI to be placed on top.

kika’s picture

+1 kill CCK namespace
+1 separate module for now

Namespace: we just introduced jQuery UI to the core -- although a different thing, can we keep those "thing+UI" naming conventions in the same style?

bangpound’s picture

yched: i'm happy to help with this patch. please let me know if i can do anything.

moshe weitzman’s picture

subscribe

yched’s picture

Status:Active» Needs review
StatusFileSize
new131.58 KB
Failed: Invalid PHP syntax in modules/field_ui/field_ui.api.php.
[ View ]

Here's a patch. From my initial tests, it works pretty fine - at least as much as current CCK HEAD ;-)

Still to be done (in followup patches IMO):

- Figure out the need for the {field_ui_settings} table, which currently stores "field settings non handled by field API : default values, allowed values for list fields". I must confess I'm not sure of the initial rationale, we'd need Karen to chime in.

- Add 'manage fields' links to the node type and vocabulary overview tables. CCK HEAD menu_altered its own page callback for node types overview - inherited from CCK D6, obviously not future proof. I kept that out, it will need to be done cleanly in patches against node.module and taxonomy.module.

- Remove fieldgroup specific code. The forms, preprocessors, templates for the 'Manage fields' and 'Display fields' screens still have hardcoded stuff about fieldgroup.module, that, back in D6, seemed like a hassle to artificially abstract out to fieldgroups.module. We need to do this in D7 of course, but untying the code and making those forms cleanly 'extensible' will probably require a bit of care and engineering. So IMO it's safer to keep this code as is for now (doesn't really hurt anyone), and let the UI settle a bit.

- Convert a few statics to drupal_static (no need to rush this before the UI settles IMO)

- Move reordering of 'non-fields' into field.module. We want the order kept even when field_ui.module is disabled)

- Update the code that validates default values for D7's new field-validation workflow

Status:Needs review» Needs work

The last submitted patch failed testing.

bjaspan’s picture

<rant>
I believe that the Drupal community has forgotten the reason we have a distinction between core and contrib. The "Move Into Core" train is going to make Drupal more difficult to maintain and release.

I do not believe moving the current Field UI into core has any benefits and many drawbacks. The Body as Field patch shows how to use Field API in a limited way without a generic Field UI. profile.module can switch from custom data storage to Field data storage without a UI, and it is far from clear that profile.module will be able to use any general-purpose Field UI anyway due to its highly user-registration-specific integration. Sure, we can't get the full power of fieldable taxonomy terms without a UI... but if you can get a Field UI from contrib, who cares? That's what contrib is for! That's why distributions/install profiles exist!

In my opinion, a feature should not be "in core" just because it is cool or lots of people want to use; core should be limited to that which is necessary to build a consistent framework with which contrib can build all the features. However, I've discussed this with Dries and I know he directly disagrees with me.

Basically, I disagree with the very first sentence on this issue:

Moving the current Field UI in CCK HEAD into core [will] make it easier for the community to grasp the current status of the UI and embrace the task of enhancing it.

If the community can't handle installing a contrib module to test out new capabilities of core, we have a big problem.

You can now all get back to committing Field UI in core. I'll still be here, being an old curmudgeon. :-)
</rant>

webchick’s picture

@bjaspan: I agree with you on that point on most things. Although I think the whole #smallcore movement is doomed until install profiles start getting some muscle behind them, I do think in general we need less code in core, not more. Each new line of code added to core is a line that needs to be tested and maintained for eternity, and is frozen in time for 3+ years.

In this particular case, however, the lack of a UI in core for fields is actively holding back progress on a number of different fronts. Porting a D6 CCK module to D7's fields requires doing an awkward separation between API code and UI code, which is tons of extra work. Additionally, something like FileField has to be split into a core patch for the API parts, a contributed module for the UI parts, and downloading CCK to test the whole thing. This makes it nigh impossible to replace Upload module with FileField, unless of course you go down the road of re-writing a one-off UI for files (and re-writing a one-off UI for taxonomy and a one-off UI for body fields, and a one-off UI for...). This sucks, especially given that everyone and their mother is going to download CCK which will override these UIs anyway. Now you have UI code that's unused, untested, and thus prone to rot. If there's something I hate worse than adding more code to core, it's adding convoluted code to core that no one uses and therefore no one maintains.

So, in this particular case, I do see the logic of moving this particular contributed module (the CCK UI only: not crap like Fieldgroup, etc.) to core. But in general, +1. :)

bangpound’s picture

StatusFileSize
new131.81 KB
Failed: Invalid PHP syntax in modules/field_ui/field_ui.api.php.
[ View ]

Call me eager beaver. I am trying to fix yched's patch so it can stay "needs review."

A few insubstantial things are being patched: .info files. I don't know if this was yched's intention.

yched: I'm going to look myself at field_ui_settings. Though I'm not an expert, I do have some familiarity of how this affects list fields.

bangpound’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

bangpound’s picture

Status:Needs work» Needs review
StatusFileSize
new131.82 KB
Failed: Failed to apply patch.
[ View ]

Aha! hook_field_widget_settings_form is declared twice in field.api.php.

bangpound’s picture

I should keep my big mouth shut, but...

I am worried that field_ui_get_setting, field_ui_set_setting is being brought into core as part of an optional module. Anyone who creates fields that use the allowed_values or default_value features supported with field_ui_get_setting may get a kind jab in the eye if they want to disable field_ui module and use some future field UI module. Will their default values and custom list values disappear? Am I misreading the code in the module?

I understand why this happens... list.module + options.module need a FIELDMODULE_allowed_values function, and the field UI module is all about end users not writing functions.

There are also some hooks introduced by the Field UI module:

hook_field_default_value (which is undocumented and missing from field_ui.api.php)
hook_field_settings_form
hook_field_instance_settings_form
hook_field_widget_settings_form
hook_field_ui_extra_fields (see line 338 of field_ui module, it's confusing).

Contrib needs to depend on the settings_form hooks. If contrib authors need to write different settings forms for different UIs, it's a big mess. Are they simple enough for alternative UIs to use as-is?

yched’s picture

StatusFileSize
new131.82 KB
Failed: Failed to apply patch.
[ View ]

re bangpound:

- patching .info files was intentional. It removes the dummy 'Core - fields' package currently used by field.module and field type modules (except they're all required and so the package doesn't show on the admin/build/modules page).

- the second hook_field_widget_settings_form in field_ui.api.php is a typo, it should've been hook_field_formatter_settings_form. Attached patch fixes this.

- field_ui_get_setting, field_ui_set_setting: yes, that's the first item in my comment #16. The current implementation is wrong even in a 'field UI in contrib' scenario, since disabling the UI module break functionality. I don't think we actually need this 'separate settings storage' anymore, but just like the 'reordering of non-field elements' (same issue, disabling the UI module break functionality), I'd really advocate fixing this in followups.

yched’s picture

re bjaspan:
Well, as you know my long-standing position was 'Field UI remains in contrib'. A few things made me revise that.

- The last 6 months made it pretty obvious that *no-one* but 2 or 3 people worry about the UI code while it sits in contrib. Possibly sad but true. Field UI needs love and care from more people, and notably more FAPI, AHAH oriented people. Me + Karen is not good enough, and I'm not interested in keeping that burden.

- While Field UI has a couple 'internal' challenges (formatter settings, per-build-mode reorder, etc), I now think the biggest challenge is how Field UI plays with the rest of drupal core. Beyond what webchick rightly says in #19 about 'alternate UIs' which will be de-facto 'duplicate UIs' for more and more features (body, taxonomy, file uploads...), I think the Field UI is the 'right' central hub for things that currently clutter the 'edit content type' form : comments settings, fivestar setings... There's no way to raise consciousness about this if the UI doesn't sit in core and only 3 people care about its code.

- Core field-types (number, list) have been poorly tested, I'm quite confident there are a load of bugs in there, that no-one notices because no-one uses those fields currently.

It does mean more Field API and Field UI bugs and tasks get created, more pression on Field-aware contributors - which naturally means more Field-aware contributors are needed, which is slowly happening. I already stated that I won't be the guy to take the "Field UI in core" extra work.
This also very possibly means D7 takes longer to ship. But I now think it's the best way forward.

catch’s picture

I'm also pro putting this in core. #smallcore only works when you can take stuff like upload, poll, forum, tracker and friends out of core - which is currently more or less won't fix - see #61285: Remove poll module from core. It also only works when core isn't required to re-implement functionality which is much better done in contrib (i.e. 60% of taxonomy.module code vs. Views).

The potential downsides of having field_ui.module in core, that I see, are that it can't have even incremental interface improvements once Drupal 7 has a stable release due to string freeze etc. Also formbuilder may or may not be able to fully replace field_ui.module from contrib (but I can't see it handling display settings by itself, or not without being more than a form builder).

However both of these seem far less bad to me than shipping Drupal 7 with a upload.module, then requiring you to download both CCK and filefield.module just to have a decent file upload solution - to me, this is would be a far, far worse situation than having interface improvements to the field_ui itself having to be done in contrib.

bangpound’s picture

I'll be able to review this patch more carefully in a few hours.

My instincts (however immature) are that the UI hooks for field API settings forms need to be core hooks. I don't see how contrib field, widget and formatter modules can thrive without this as a common foundation for this CCK UI and any future field UIs. These hooks return just a form array. It's a pattern used throughout Drupal.

yched’s picture

bangpound: "the UI hooks for field API settings forms need to be core hooks"
Sorry, I'm not sure what you mean.
If this patch gets in, then they will be core hooks, right ?
If it doesn't and field_ui.module remains in contrib, then contrib field types, widgets and formatters will need implement core 'field API' hooks, and 'contrib field_ui' hooks - just like they currently do for CCK D6, BTW. A hook is a 'core hook' only if core invokes it, there's no way (and no reason) to 'freeze' a core hook interface that's only invoked by contrib.

bangpound’s picture

yched: Thanks for the clarification. This may fundamentally be an issue for Fields API documentation. I shudder at the thought that two or more Field UI's are viable (even concurrently on the same site) but they require different hooks for the settings forms. I wasn't really thinking about what it means to be core and optional nor what it means to be an unimplemented hook. Cheers.

Status:Needs review» Needs work

The last submitted patch failed testing.

bangpound’s picture

StatusFileSize
new131.08 KB
Failed: 11037 passes, 8 fails, 16 exceptions
[ View ]

I made a new patch that accounts for the changes in default install profiles.

bangpound’s picture

Status:Needs work» Needs review

argh. i never remember to fix the status.

mlncn’s picture

/submitting self as candidate to receive most useless nitpick award.

As not a single module in Drupal core has an underscore in the system name, should we make this 'fieldui'?

Overall, excellent. Modules should live where the most movement can be made on them and the things that rely on them, and right now for a field user interface that's core.

yched’s picture

StatusFileSize
new130.69 KB
Failed: 11858 passes, 8 fails, 16 exceptions
[ View ]

fieldui / field_ui : hm, I have doubts that fieldui will be accepted. And I personally find it butt ugly ;-)

Updated patch, includes CCK's #520774: Default value field shown even if module specifies to use NONE/CUSTOM default value.

bjaspan’s picture

Hmmm... "feeldwhee" does not sound like a great module name to me. Let's break with tradition and use an underscore. :-)

bangpound’s picture

What's the RTBC threshold for this patch? The problems are out there in the open. There are no tests, but nobody has said "we need tests." I've used the patch for over a week. It works... so far, so good.

I've set up API module to index HEAD with this patch. These are the problems I've found:

files missing @file phpinfo
field_ui.api.php
field_ui.install
undocumented hooks
hook_field_formatter_settings_form
undocumented functions
field_ui_extra_field_values
_field_ui_node_extra_fields
_field_ui_user_extra_fields
field_ui_field_overview_form_submit
field_ui_field_overview_form_validate
field_ui_next_destination

If this patch can be RTBC without this documentation (and otherwise as is), I want to be the person who pushes someone else over that threshold. In my n00b naïve opinion, this patch is RTBC aside from the documentation issues.

If we need to document these pieces before the patch can be RTBC, let me know and I will try my best on Tuesday or Wednesday if yched cannot.

yched’s picture

StatusFileSize
new139.03 KB
Failed: Failed to apply patch.
[ View ]

Fixed the doc problems mentioned in #37.
+ moved the hook_field_ui_extra_fields() implementations in node and user modules.
As noted in #16, 'extra fields' will need a followup task to be moved into field.module and reworked a bit to better differentiate between 'form' elements and 'view' elements.

Berdir’s picture

Status:Needs review» Needs work

Because everyone hat... erm, loves my DBTNG reviews, here's another one ;)

+  if ($setting_type == 'field' || empty($instance)) {
+    $value = db_select('field_ui_field_settings', 'fs')->fields('fs', array('setting_option'))
+      ->condition('fs.setting', $setting)
+      ->condition('fs.setting_type', $setting_type)
+      ->condition('fs.field_name', $field['field_name'])
+      ->execute()->fetchField();
+  }
+  else {
+    $value = db_select('field_ui_field_settings', 'fs')->fields('fs', array('setting_option'))
+      ->condition('fs.setting', $setting)
+      ->condition('fs.setting_type', $setting_type)
+      ->condition('fs.field_name', $field['field_name'])
+      ->condition('fs.bundle', $instance['bundle'])
+      ->execute()->fetchField();
+  }

- We have a dynamic query builder, let's use it. Instead of duplicating the whole definition, just put the additional condition into an if statement. Also, I think we agreed on putting every method call on a new line, even fetch*() (I know that core is not consistent...)

something like

<?php
$query
= db_select()
  ->
fields()
 
//...;
if ($setting_type != 'field' || !empty($instance)) {
 
$query->condition('fs.bundle', $instance['bundle']);
}
$value = $query
 
->execute()
  ->
fetchField();
?>

There is also a similiar delete statement.

+function field_ui_uninstall() {
+  drupal_uninstall_schema('field_ui');
+  db_query("DELETE FROM {variable} WHERE name LIKE 'field_extra_weights_%'");
+}

That should use db_delete()...

+      db_query("UPDATE {field_config_instance} SET weight = %d WHERE bundle = '%s' AND field_name = '%s'",
+        $values['weight'], $bundle, $key);

and this db_update()

+      $query = db_select($table, 'f', array('fetch' => PDO::FETCH_ASSOC));
+      $query->fields('f', array('entity_id'));
+      $query->condition('f.deleted', 0);
+      $results = $query->execute()->fetchAssoc();

That looks rather static to me, you don't need to use db_select() over db_query("SELECT ...") unless there is a reason (dynamic stuff, query extender, use case for hook_query_alter, ...)

yched’s picture

Thanks Berdir.

I might not be able to tackle those fixes until a couple days.
@bangpound, if you want to beat me to it, you're much welcome ;-)

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new134.9 KB
Failed: 11673 passes, 8 fails, 16 exceptions
[ View ]

This should take care of Berdir's remarks in #39.

tic2000’s picture

Can you provide a -up patch please? I managed to apply the patch by renaming the project name to match mine :).

When I try to add a field to a content type I see:

    * Warning: array_keys(): The first argument should be an array in field_sql_storage_field_storage_query() (line 364 of M:\wamp\www\d7\modules\field\modules\field_sql_storage\field_sql_storage.module).
    * PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd7.field_data__' doesn't exist: SELECT t.bundle AS bundle, t.entity_id AS entity_id, t.revision_id AS revision_id, e.type AS type FROM {field_data__} t INNER JOIN {field_config_entity_type} e ON t.etid = e.etid WHERE (deleted = :db_condition_placeholder_31) ORDER BY t.etid ASC, t.entity_id ASC LIMIT 0, 1; Array ( [:db_condition_placeholder_31] => 0 ) in field_sql_storage_field_storage_query() (line 417 of M:\wamp\www\d7\modules\field\modules\field_sql_storage\field_sql_storage.module).

If I go back, the field is added and works.

yched’s picture

Sorry, actually no I can't, Windows + Eclipse here. But the patch is created from drupal root, I don't see why you'd need to manually change project name. The only drawback is that my patches don't have function context hints, but that's only a helper for visual inspection.

Will try to investigate the errors shortly.

tic2000’s picture

When you create the patch on the second screen select "Project" as your "Patch root". That will do the trick of -p.

yched’s picture

StatusFileSize
new135.75 KB
Failed: 11858 passes, 8 fails, 16 exceptions
[ View ]

Updated patch,
- fixes the errors mentioned in #43 (when cleaning up queries in #41, I used the wrong argument for field_attach_query(), as if #367753: Bulk deletion was already in)
- moves the "all fields" overview page from admin/build to admin/structure

re tic2000: well, I usually leave default options. I have to say I never really investigated, in 4 years rolling drupal patches, you're my 1st complain :-). I guess you're right that "workspace" would be best (no cruft Eclipse header), but a google search shows you'll find quite a few "project" Eclipse patches out there. http://drupal.org/node/427082 advises root "workspace", BTW.
In case of project mismatch, the "Apply patch" Eclipse dialog lets you reassign to one of your projects : right click on the project name marked with an error, pick 'move'.

tic2000’s picture

@yched
Someone here gave me the same tip I gave you just a few months back (2 I think). I don't remember exactly who or when. I rolled patch like that after that.
LE. I found it #479340-2: Color module creates JavaScript error: container is undefined

I will test the patch later today.

int’s picture

In the #45 patch we have one file .project that should be deleted.
In field_ui.info is missed this version = VERSION.

At this point what you want to do with the Type of data: File?

yched’s picture

Removed .project, added version = VERSION.

@int #47: "At this point what you want to do with the Type of data: File?"
I don't think I understand the question. Work is under way to include Filefield and Imagefield in core and have them replace upload.module, but this is not related to this patch.

Other than that - not much heat here, I thought there was a high demand for Field UI in core ;-).

yched’s picture

StatusFileSize
new132.16 KB
Failed: 11037 passes, 8 fails, 16 exceptions
[ View ]

and the patch.

Status:Needs review» Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Installed from scratch and tried this out:

  1. I dragged Revision information to bottom but this was not reflected on the node/add/article page. Perhaps a problem with new vertical tabs.
  2. I added term reference field and got a blank page afterwards whose URL was
    http://mm/prfr/admin/structure/node-type/article/fields/field_mw/field-s...

    The next page showed the following errors. Might be due to new admin theme.

    Warning: include(/Users/mw/htd/prfr/includes/maintenance-page.tpl.php): failed to open stream: No such file or directory in theme_render_template() (line 1143 of /Users/mw/htd/prfr/includes/theme.inc).
    Warning: include(): Failed opening '/Users/mw/htd/prfr/includes/maintenance-page.tpl.php' for inclusion (include_path='.:/opt/local/lib/php') in theme_render_template() (line 1143 of /Users/mw/htd/prfr/includes/theme.inc).
  3. Got a white page when clicking on http://mm/prfr/admin/structure/node-type/article/fields/body/field-settings on field listing for Article content type
  4. It is a pity to leave behind all the advanced help we have in D6 cck. Is advanced help patch truly dead for d7? big sigh.
bangpound’s picture

Status:Needs work» Needs review
StatusFileSize
new134.25 KB
Failed: 11037 passes, 8 fails, 16 exceptions
[ View ]

Here's a patch that fixes the fundamental problem Moshe was having. The field_ui_field_has_data function was passing the field ID to the field_attach_query function. That function is expecting a field name. This fixes WSOD on clicking the 'field settings' and upon creating the field.

hook_menu was still putting something in admin/build which I moved to admin/structure.

I took the liberty of adding the taxonomy term field settings to the patch because now #491190: Provide a taxonomy term field type is committed. The settings form focuses on supporting one vocabulary with 0 as the parent while also not breaking fields whose allowed values are more complex trees. i.e. It does what's needed but it could do more.

I didn't have a chance to look at the issue Moshe had with re-ordering revision information on the node form.

Status:Needs review» Needs work

The last submitted patch failed testing.

yched’s picture

Doh, I fixed the field_attach_query() calls and admin/structure vs admin/build back in #45 alraady, but then rolled #49 upon #41, thus loosing the changes :-(. Thanks bangpound.

The admin items inside vertical tabs are probably out of reach for reordering. Messing with those is at best a job for fieldgroup. Best we can do IMO is ensure the vertical tabs group stays at the bottom.

bangpound’s picture

Status:Needs work» Needs review

What does this patch have to do with book? The 8 fails are at Book functionality.

Status:Needs review» Needs work

The last submitted patch failed testing.

bangpound’s picture

This patch needs a thorough glare from a Fields and testing expert.

The failures are occurring because the 'print' build mode doesn't exist when the tests are run. However, this build mode is in the book module, which is enabled when book tests are being run. So I don't know what gives.

bangpound’s picture

I found a workaround, but I fear it's symptomatic of a deeper issue that I am not ready to fix. In book.test near the very end of checkBookNode, add field_cache_clear(); before $this->drupalGet('book/export/html/' . $node->nid);

webchick’s picture

Issue tags:+Drupal 7 priority

Tagging.

This issue is important because it is actively holding up progress in other Field API improvements, and time is running short. It is something that will likely have to wait until Dries gets back from holiday though, so let's try and get it all spiffy and nit-picked this week.

yched’s picture

Problem is simpletest's setUp() enables book.module but explicitly chooses not to invoke hook_modules_enabled. When book is enabled through the UI, field_modules_enabled() would trigger the cached field_info rebuild.
Investigating the reason for this with boombatower on IRC.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new134.49 KB
Failed: 11032 passes, 3 fails, 0 exceptions
[ View ]

OK, sorry, a little investigation shows that's actually not related to simpletest's way to install modules.
It's rather caused by field_build_modes()'s static not properly reset.

Attached patch adds a drupal_static() to field_build_modes() and resets it in field_clear_cache(). It might be worth fixing through #503604: Move hook_field_build_modes() into hook_fieldable_info() ?, too.

quicksketch’s picture

Status:Needs review» Needs work

So a few things:

- Deleting a field does not delete its data. The "field_revision_field_foo" and the "field_data_field_foo" tables persist even after deleting a field. Is this intentional? The warning when deleting a field indicates it should be deleted: "If you have any content left in this field, it will be lost. This action cannot be undone."
- If a field widget is defined for a field type that does not exist, I get a warning on the "admin/structure/node-type/foo/fields" page. This is happening for me because I have an "Image" widget defined for the "File" field type, but File module is not enabled. You can easily emulate this by making any existing widget require a made-up field type in hook_field_widget_info().
- This patch contains a large amount of garble in the field prefix description: "Define a string that should be prefixed to the value, like '$ ' or '€ '."
- We're inconsistent about using "TODO" and "@todo" within PHPdoc. I believe we should always use @todo in PHPdoc, and // TODO: inline.
- Could we convert static $allowed_values; to use the static caching mechanism while we're moving it?
- We should also convert static variables in field_ui_extra_field_values(), field_ui_build_modes_tabs(), field_ui_field_type_options(), field_ui_widget_type_options(), and field_ui_formatter_options().
- taxonomy_field_settings_form() has an extra new line before it and it's PHPdoc.
- Throughout the patch, our PHPdoc is not wrapping properly. It's not that it doesn't wrap but that it's that it wraps too early on many lines. Even if we're working with sentences, they should go to the 80 character bar unless starting a new paragraph.
- It seems like there is a space before every "?" in the whole patch that should be removed.
- I'm not sure if we should finish field_ui_inactive_fields() or not before committing, perhaps it should be moved to a separate issue rather than including a commented out function though.

I'm curious how polished this patch should be before we commit it, since it has an awful lot of TODOs in it, but at the same time there's no way we can replace upload or profile modules until we have a UI for configuring these options.

yched’s picture

- Yes, currently deleting a field, an instance or a bundle only marks the data as deleted so that they don't come up on object load, but they stay in the db. The actual deletion and cleanup is handled in a cron process being added in #367753: Bulk deletion.
Actually, filefield, or any other 'resource reference' field, that need to do some additional cleanup or computation when a value disappears, are the primary reason for this. Contrib CCK never solved the 'massive data cleanup brings a risk of timeout' issue, and there are some cases where CCK just punts and keeps stale data.
The newly fieldable comments are an incentive to move 'single object deletion' to 'late cleanup' as well, BTW, because deleting a node or a comment can cascade down to deleting an arbitrary large number of comments. There would then be one single model for field data deletion: mark as deleted, cleanup on cron. Followup after 'bulk delete' patch gets in.

- binary garble: yes, noticed that too. That's originally a € symbol, it's quite possibly my Eclipse patch generator that corrupts it, and makes it worse on each reroll. Couldn't find a workaround so far. Should obviously be cleaned before a commit.

- TODOs, level of polish for commit:
Note that I posted in #16 a list of TODO tasks (notably involving default values, extra fields). I do not see these tasks as happening in the initial patch, reviewing such changes by comparing .patch files is awkward. Or we move the work on this to a separate repo, git, bazaar or CCK CVS. Note that this is where we've been so far (CCK HEAD for D7 is here since last december), but didn't raise much awareness from the community. The widely spread notion is that 'there's no Field UI'. No, there's a Field UI, it would be better off in core for a number of reasons, and it really needs some work.
As I said a few times here and there, with Karen out of the D7 work and Barry not familiar at all with the UI code, I just don't have the bandwidth to be the one leading the UI effort - be it core or contrib, BTW. I try to get the ball rolling with this thread and transfer knowledge / initiate discussion in http://groups.drupal.org/node/23545.

bangpound’s picture

yched:

That kind of character encoding mess usually indicates that you're coding in a Windows or ISO Latin character set and the output is UTF-8.

You could also work around with using HTML entities?

<!ENTITY euro   CDATA "&amp;#8364;"  -- euro sign, U+20AC NEW -->
plach’s picture

subscribe

bangpound’s picture

yched:

That kind of character encoding mess usually indicates that you're coding in a Windows or ISO Latin character set and the output is UTF-8.

You could also work around with using HTML entities?

bangpound’s picture

Status:Needs work» Needs review
StatusFileSize
new135.02 KB
Failed: Failed to apply patch.
[ View ]

This patch incorporates all of quicksketch's comments in #62.

quicksketch’s picture

Assigned:Unassigned» quicksketch
Status:Needs review» Needs work

I'm working on a reroll of this patch. Right now it seems that select lists will cause a WSOD on the node edit form, plus I want to fix up a few of these TODOs at least.

yched’s picture

StatusFileSize
new1.89 KB
new135.09 KB
Failed: Failed to apply patch.
[ View ]

Many thanks to both of you for stepping in.

#67 has a small bug in the drupal_static() conversion, which causes the 'add new field' / 'add existing field' rows to disappear in 'Administer fields' page.
The default value should be NULL instead of empty array().

Attached patch fixes that, and the warnings quicksketch mentioned in #62 (item 2).
txt file contains the 'manual diff' from #67 in case quicksketch moved forward on his own reroll meanwhile :-/

bangpound’s picture

I'm using this patch to develop the autocomplete widget for taxonomy term fields and list fields.

Deleting an instance of a field that isn't the only instance of the field apparently does not delete the row from field_config_instance, but the 'deleted' column is set. Running cron over and over again doesn't cause the row to be deleted. When I want to add the field back to this bundle, I'm told there are primary key constraint violations.

yched’s picture

Deleting an instance of a field that isn't the only instance of the field apparently does not delete the row from field_config_instance, but the 'deleted' column is set.

OK so far.

Running cron over and over again doesn't cause the row to be deleted.

Sure, this cleanup is done in #367753: Bulk deletion.

When I want to add the field back to this bundle, I'm told there are primary key constraint violations.

Now that sounds like a bug, not sure if that's the Field UI patch or a bug in Field API, investigation needed. Could you provide the exact error message ?

bangpound’s picture

Move this to another issue if you think it's appropriate.

There was a problem creating field instance : SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '6-article' for key 2.

To reproduce in Field UI:

  1. Create a field of any type on bundle X.
  2. Add a new instance of that field to bundle Y.
  3. Delete the field instance on bundle X.
  4. Run cron.
  5. Add the field instance back to bundle X.
yched’s picture

Status:Needs work» Needs review
StatusFileSize
new134.85 KB
Failed: Failed to apply patch.
[ View ]
new3.07 KB

Bug was in Field API : #544400: wrong unique key in {field_config_instance}

Attached patch fixes a similar case of 'add field to bundle X, remove from bundle X, create new field with same name --> form validation error' (error was in field_ui this time), + a couple glitches I found along the way.

'manual' changes in txt file for quicksketch's #68.

quicksketch’s picture

Status:Needs review» Needs work
StatusFileSize
new122.9 KB
Failed: 11899 passes, 2 fails, 0 exceptions
[ View ]

Here's an updated patch just to show where I'm at with things. This patch still has some major, major problems though. Here's the list of changes:

- Removed entirely the field_ui_settings table, which is no longer necessary at all now that we can alter the settings provided by hook_field_info(). But I didn't bother with that since we want to be able to disable the Field UI module entirely anyway, and the ability to have default values shouldn't be tied to the UI module.
- Removed the "use PHP for field settings" permission and all ability to use PHP for default values, though a contrib module can add this functionality back by using $instance['default_value_function'].
- Removed the exposed functionality for "Available values function" from List fields (Radios, Checkboxes, and Select lists), since this ability was just as dangerous as arbitrary PHP execution since you could just enter "exec" as the function to use, it was presenting a security problem. Same as default values though, a contrib module can easily reintroduce this functionality.
- Ported the warning message for "Disabled fields" when a module is disabled that was providing an in-use widget.
- Removed several other TODOs and made the Field UI module completely independent of Field module so you can actually turn off the module and have the site still work.

Things left to do:
- Some of the weight handling of "extra" fields is still in Field UI and needs to be moved to Field.
- Select / Radio / Checkboxes are still completely broken. I think there's a problem in the Field module handling itself since you can't even have a Radio button set on the node form without it causing a complete WSOD.

yched’s picture

W00t !

I have to admit I'm not clear on the current state of default values and allowed values for list fields. IIRC, this was mainly Karen's work post FiC sprint, and I did not fully grok that part. So, no real opinion on the turn you're taking.
Just a remark that deferring 'PHP for default values / allowed values' to a contrib module will make the D6 -> D7 migration script (or rather 'module', more likely) more complex, because we can't migrate a D6 field using those unless we can mimic the behavior on the new field. Or we migrate with a warning, 'PHP allowed|default values not reproduced for field foo, install contrib xyz').

Other remarks:

+++ modules/field/field.info.inc 11 Aug 2009 00:37:43 -0000
@@ -76,6 +76,9 @@
+          $field_info['instance_settings'] += array(
+            'default_value' => NULL,
+          );

Dunno about this. A 'setting' that's here for every field type should not be a setting but a top-level $instance property ($instance['default_value'] rather than $instance['settings']['default_value']) ? That's the original intent of separating field-type specific stuff in 'settings'.
I *know* we discussed that 'default_value' stuff in the sprint and in followups, but it's a little far off, would be cool to have Karen around :-)

+++ modules/field/field.form.inc 11 Aug 2009 00:37:43 -0000
@@ -38,11 +38,16 @@
-    $function = $instance['default_value_function'];
-    if (drupal_function_exists($function)) {
-      $items = $function($obj_type, $object, $field, $instance);
+    if (!empty($instance['default_value_function'])) {
+      $function = $instance['default_value_function'];
+      if (drupal_function_exists($function)) {
+        $items = $function($obj_type, $object, $field, $instance);
+      }
+    }
+    elseif (!empty($instance['settings']['default_value'])) {
+      $default_value = $instance['settings']['default_value'];

field_default_insert() should be updated to this new code as well (possibly abstracted out to a helper function, then ?)

+++ modules/field/field.module 11 Aug 2009 00:37:43 -0000
@@ -266,14 +266,15 @@
-  foreach ((array) $items as $delta => $item) {
-    if (!$function($item, $field)) {
-      $filtered[] = $item;
+  if (drupal_function_exists($function)) {
+    foreach ((array) $items as $delta => $item) {
+      if ($function($item, $field)) {
+        unset($items[$delta]);
+      }

drupal_function_exists() is needed, but I'd rather not fail silently if function doesn't exist. The field module is broken. + we should keep this in #517430: field_set_empty() missing drupal_function_exists() IMO.

+++ modules/field/field.attach.inc 11 Aug 2009 00:37:42 -0000
@@ -886,6 +886,11 @@
+  // Add custom weight handling.
+  list($id, $vid, $bundle) = field_attach_extract_ids($obj_type, $object);
+  $object->content['#pre_render'][] = 'field_alter_extra_weights';
+  $object->content['#extra_fields'] = field_ui_extra_field_values($bundle);
+

Those should be added to the $output return value. field_attach_view() works by returning an array, that is not assumed to end up in $object->content.
+ similar code should be added to field_attach_form() for form elements reordering - but thats probably 'in progress' ?

+++ modules/field/field.crud.inc 11 Aug 2009 00:37:42 -0000
@@ -632,8 +632,8 @@
-  $query->condition('fc.active', 1);
   if (!isset($include_additional['include_inactive']) || !$include_additional['include_inactive']) {
+    $query->condition('fc.active', 1);

Good catch. It worked that way in D6, dunno when / why it changed. Worth a separate patch IMO.

I'm on crack. Are you, too?

quicksketch’s picture

Status:Needs work» Needs review
StatusFileSize
new126.5 KB
Failed: Failed to apply patch.
[ View ]

Excellent, thank you for the fantastic review yched! Here's a reroll that fixes the remaining issues I had in #74 and the concerns yched had in #75 (with the exception of splitting out the $query->condition('fc.active', 1); change into a separate patch.

Just a remark that deferring 'PHP for default values / allowed values' to a contrib module will make the D6 -> D7 migration script (or rather 'module', more likely) more complex, because we can't migrate a D6 field using those unless we can mimic the behavior on the new field.

Yes, agreed. Though at the same time perhaps this module could provide commonly used lists of default values or allowed values, like a standardized list of states or something. I mostly excluded it for security reasons and because I think entering any PHP code into a text area is almost always a bad idea from a maintenance perspective, so we should push it to contrib to fight this tendency.

moshe weitzman’s picture

During field in core sprint, we discussed a contrib module lets admins creates lists of allowed values (e.g. provinces, countries, etc.). You can even this comment above list_allowed_values() - *  TODO Rework this to create a method of selecting plugable allowed values lists.. This is how CiviCRM handles list fields (so I am told).

I agree with moving PHP out of this UI.

quicksketch’s picture

Thanks for the look Moshe. It seems your sentence is missing a verb:

You can even this comment above list_allowed_values()

Can we remove the comment, or correct it? I kind of thought that it was already accounted for, so I think removal would be okay.

webchick’s picture

Just a note that the bulk deletion patch got in, so deleting fields should now work, at least from an API perspective.

Keep this great work coming folks! It looks like we're getting close.

bjaspan’s picture

Deleting fields should always have worked, though I do see that yched fixed a bug with an incorrect unique key causing constraint violations.

With the bulk delete patch in, running cron should now (in batches) remove deleted field data, instances, and field records from the database.

yched’s picture

field_attach_extra_weights(), field_attach_extra_weight(), field_attach_extra_fields():
I'd rather keep the 'field_attach_FOO()" namespace for "functions that fieldable entities need to call at the right time in their own workflow in order to call themselves fieldable:
f_a_load(), f_a_presave(), f_a_insert()...". This lets developpers have a cleaner view of " what do I need to do to be fieldable ? is there any f_a_*() function I have forgotten ?".
See http://api.drupal.org/api/group/field_attach/7 : "Fieldable types call Field Attach API functions during their own API calls; for example, node_load() calls field_attach_load(). A fieldable type is not required to use all of the Field Attach API functions."

We already have a couple existing functions that don't belong in there and should be moved somewhere else: field_attach_extract_ids(), field_attach_extract_bundle(), field_attach_create_stub_object(). Let's not add new ones here.

yched’s picture

+++ modules/field/field.form.inc 11 Aug 2009 06:40:50 -0000
@@ -38,12 +38,9 @@
+    drupal_function_exists('field_default_insert');
+    field_default_insert($obj_type, $object, $field, $instance, $items);

Calling field_default_insert() in field_default_form() sounds like a WTF. It just happens that f_d_insert() does the 'default value' stuff, but it could do some other additional stuff in the future. It's best to separate the common code to a helper function called in both places IMO.

19 days to code freeze. Better review yourself.

Status:Needs review» Needs work

The last submitted patch failed testing.

bjaspan’s picture

Status:Needs work» Needs review
StatusFileSize
new35 KB
Failed: Failed to install HEAD.
[ View ]

Re-rolled.

Status:Needs review» Needs work

The last submitted patch failed testing.

Gábor Hojtsy’s picture

Status:Needs work» Needs review

Failed to install HEAD sounds like an issue with the testbot?

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Hum, no. The latest patch in fact does not contain the field_ui module. No wonder it was just 35k.

Required modules Required modules not found.
The following modules are required but were not found. Please move them into the appropriate modules subdirectory, such as sites/all/modules. Missing modules: Field_ui

bjaspan’s picture

I guess my re-roll wasn't so helpful. :-)

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new126.44 KB
Failed: Failed to apply patch.
[ View ]

New attempt :-)

yched’s picture

StatusFileSize
new126.33 KB
Failed: Failed to apply patch.
[ View ]

+ updated after #529756: Field weight per context. Field API now supports different weights for edit forms and for each build mode.
The UI currently just sets the same weight for every context (the one set by drag-n-drop on 'Manage Fields' screen).

yched’s picture

StatusFileSize
new126.38 KB
Failed: Failed to apply patch.
[ View ]

Mh, #90 left out quite a few places...

webchick’s picture

Could someone provide a summary of how close this is to me tearing into it for a final pass? :)

yched’s picture

bangpound and quicksketch crossed a few items from my #16 (which listed major TODO points I was advocating as followup tasks), here are the remaining ones :

- Add 'manage fields' links to the node types and vocabularies overview tables. CCK HEAD menu_altered its own page callback for node types overview - inherited from CCK D6, obviously not future proof. I kept that out, it will need to be done cleanly in patches against node.module and taxonomy.module.

- Remove fieldgroup specific code. The forms, preprocessors, templates for the 'Manage fields' and 'Display fields' screens still have hardcoded stuff about fieldgroup.module, that, back in D6, seemed like a hassle to artificially abstract out to fieldgroups.module. We need to do this in D7 of course, but untying the code and making those forms cleanly 'extensible' will probably require a bit of care and engineering. So IMO it's safer to keep this code as is for now (doesn't really hurt anyone), and let the UI settle a bit.

- Update the code that validates default values for D7's new field-validation workflow

+ Field API features currently not reflected in the UI (definitely followups as well IMO) :
- formatter settings
- per context order.
- configuration of field indexes on field creation.

+ kind of related : #537750: Field UI for comments

+ more generally, UX improvements if needed:
- I'd say the 'display fields' screen will need a rework. Per context order implies one screen per build mode (instead of curently 'teaser + full', 'search_index + search_result' grouped on one subtab). I suspect vertical tabs could be great here (one tab per build mode).
- find out the right decomposition of subforms: instance settings, widget type, widget settings (depend on widget type), default values (depend on widget type + settings)
IMO #492834: Hover operations for flooded state screens (if it lands...) would be great for the subforms
This is the part where I'd expect usability folks to chime in.

yched’s picture

StatusFileSize
new126.76 KB
Failed: Failed to apply patch.
[ View ]

Updated patch :

- fixes my remarks #81 and #82.
I kept field_attach_extra_weight() in the field_attach_ namespace, because that one is actually in the 'fieldable entity' API. The other two are now field_extra_fields() and _field_extra_weights_pre_render() in field.module.

- Renamed hook_field_ui_extra_fields() to hook_field_extra_fields()

- Removed the elements of node_field_extra_fields() that live in the 'Additional settings' vertical tabs, they are out of reach for us.
Added the vertical tab group itself as an 'extra field', and added a default weight of 99 so that there's enough room for our fields.
Moved taxonomy and poll 'extra fields' out of node_field_extra_fields() and into [taxonomy|poll]_field_extra_fields().

- Fixes fatal error / WSOD on field creation, caused by field_attach_query() signature change in #367753: Bulk deletion.

webchick’s picture

I played around with this some, and here are some things I found:

1. New fields I add to nodes always appear below the buttons, and there seems to be no way to reconcile this from the Display Fields tab, since the buttons aren't able to be re-weighted.

New fields below buttons

2. On the User configuration page there is no way to get back to the configuration page; only tabs are exposed for manage/display fields:

No way to configure users

3. When I chose a "List (text)" field, with "Checkboxes/Radio buttons" widget and added some entries there, after saving the field configuration I got a series of notices like this:

    * Notice: Array to string conversion in drupal_validate_utf8() (line 1137 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Warning: preg_match() expects parameter 2 to be string, array given in drupal_validate_utf8() (line 1143 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Notice: Array to string conversion in drupal_validate_utf8() (line 1137 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Warning: preg_match() expects parameter 2 to be string, array given in drupal_validate_utf8() (line 1143 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Notice: Array to string conversion in drupal_validate_utf8() (line 1137 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Warning: preg_match() expects parameter 2 to be string, array given in drupal_validate_utf8() (line 1143 of /Users/webchick/Sites/core/includes/bootstrap.inc).

Upon going back and re-saving the configuration form, no errors occurred.

When selecting from the radio buttons in my user profile, I again got a series of errors:

    * Notice: Array to string conversion in drupal_validate_utf8() (line 1137 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Warning: preg_match() expects parameter 2 to be string, array given in drupal_validate_utf8() (line 1143 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Notice: Array to string conversion in drupal_validate_utf8() (line 1137 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Warning: preg_match() expects parameter 2 to be string, array given in drupal_validate_utf8() (line 1143 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Notice: Array to string conversion in drupal_validate_utf8() (line 1137 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Warning: preg_match() expects parameter 2 to be string, array given in drupal_validate_utf8() (line 1143 of /Users/webchick/Sites/core/includes/bootstrap.inc).
    * Warning: Invalid argument supplied for foreach() in options_buttons_elements_process() (line 139 of /Users/webchick/Sites/core/modules/field/modules/options/options.module).

The selected value does display on my profile page though.

Deleting fields works now! Yay! :)

4. I chose "key" as the display format on said field, just to try something different, and got:

    * Notice: Undefined index: safe in theme_field_formatter_list_key() (line 292 of /Users/webchick/Sites/core/modules/field/modules/list/list.module).
    * Notice: Undefined index: safe in theme_field_formatter_list_key() (line 292 of /Users/webchick/Sites/core/modules/field/modules/list/list.module).

...when editing my profile page and:

Warning: Invalid argument supplied for foreach() in options_buttons_elements_process() (line 139 of /Users/webchick/Sites/core/modules/field/modules/options/options.module).

...when saving it.

5. Widget validation and default values don't seem to be working. I attached a simple numeric field to users with a min value of 1 and max value of 10, with a default of 5. The default value did not appear, and it happily took the value of 90 that I typed in. The settings form remembers the values, however. Typing in a character value is caught though, which is good.

6. There's seemingly no way to delete values I've entered in when I continuously add new value:

Delete

I'm used to Filefield, upload, etc. that have some sort of checkbox/remove button/link, whatever. This might be "by design".

7. The styling's off on the tabs in Seven in some places, and it gets "cut off":

cut off

---

What this all basically tells me is that we're sorely lacking tests for this (of course). I could probably go on and on finding issues all night, since nitpicking is my specialty and all. :P I think we need to cut ourselves off at some point though and get this sucker in so that we can work on some of the critically important field-related patches still in the queue.

So I would request that we fix:
1. Validation and default values
2. The weighting of the new fields so they do not appear below the submit buttons.
3. Add a simple default tab for user configuration so there's a way to get back.
4. Anything else that yched/quicksketch deem critical to this initial patch since realistically it has the most eyes on it and is in the best position to lay important groundwork.

Of course, all of this needs to pass by Dries's smell test too so let's not slack off too much. :)

I'll give the code a good once-over once this gets to RTBC, but it looks like the code reviews to this point have been very thorough. Great!

Gábor Hojtsy’s picture

@webchick: on (7), looks like those tabs go wrong in their height when they go "below" the title. Most probably a Seven bug, and has nothing to do with this patch. Tabs are not necessarily to be used for actions, but that is still being discussed and cleaned up, so nothing to change here. See #542658: Move action "tabs" out of local tasks

Gábor Hojtsy’s picture

StatusFileSize
new112.04 KB
new42.2 KB
new42.18 KB
new97.28 KB

Did some testing myself:

@webchick: In (1), you probably did not mean the display fields tab. Fields in the form can be reorganized in the regular content type fields page. Agreed, that it is an issue that the fields show under the buttons by default, which seem to be somehow part of the "Additional settings" item. If you move the field above it, then it will show above the vertical tabs element.

-----

I've also went to add a simple number element. Some observations on there:

a. I'm directed to general settings for the field although I'm under the article type clearly. Clicking the highlighted (current) tab gets me to a different form with article specific settings too. Puzzling. Also, this form has general settings too (unlike the first form displayed, which said there were none).

b. A default tab is missing for the overview of all fields. Not sure that this second tab level will scale to the number of fields people have though. I understand it is there so that one can navigate among different fields, even though configure links are on the manage fields table.

-----

Went to edit the body field too and found the summary field settings puzzling:

a. It says these are display settings, while I'm on editing the form not setting whether the summary itself is displayed or not.

b. A simple yes/no AKA enabled/disabled setting should not be a dropdown. Let's use a simple checkbox IMHO.

-----

Tried to delete a field. I would not say it works. When deleting it, it still showed as a tab on the content type:

When I tried to actually click that tab again, the output was obviously quite borked:

yched’s picture

re webckick #95:

5. Widget validation and default values don't seem to be working. I attached a simple numeric field to users with a min value of 1 and max value of 10, with a default of 5. The default value did not appear, and it happily took the value of 90 that I typed in. The settings form remembers the values, however. Typing in a character value is caught though, which is good.

I cannot reproduce this, actually. Default value appears on the node create form (its' not supposed to appear on "edit existing node form"), and min 1 max 10 raise a validation error for value 90...

Investigating the 'new fields appear below submit buttons' thing. We had some code to prevent this in D6. I did not see that earlier because I have the habit of dragging my new fields to their location before I hit submit to create them ;-)

re gabor #97
- I think those secondary tabs should go anyway. They don't play well with the notion of subforms, esp if we want to load them via ajax.
- Fully agreed on the 'Display summary' option tweaks, but such adjustments are probably too much for this thread IMO, and would be best in followups.

yched’s picture

StatusFileSize
new126.54 KB
Failed: Failed to apply patch.
[ View ]

Updated patch
- fixes 'new fields appear below submit buttons'. This was actually caused by making the 'additional settings' vertical tabs reorderable.
New fields are created with a weight = 'greatest weight +1' (unless the 'add new field' row is explicitely dragged to a specific location before hitting Submit).
With 'additional settings' at 99 and buttons at 100, this caused the new field to have the same weight then the buttons, thus appearing after because added to the form later. Even if we adjusted default weights, at best new fields would appear above the buttons but below the vertical tabs by default, which is stupid.
Thus, made the vertical tabs non-reorderable, just like the buttons. They stick at weight 99 and 100.

- Fixes the text for the 'Display summary' option, mentioned by Gabor in #97.
New text: "Summary input / This allows authors to input an explicit summary, to be displayed instead of the automatically trimmed text when using the 'Summary or trimmed' display format."
This option should be a widget setting instead of an instance setting anyway. Not for this patch to fix, because it would require a fresh HEAD install to test...
As I said above, I think other such tweaks should be kept as followups. There's plenty to fine tune in here.

webchick’s picture

"I cannot reproduce this, actually. Default value appears on the node create form (its' not supposed to appear on "edit existing node form"), and min 1 max 10 raise a validation error for value 90..."

Oh! Interesting. So I added this field to /users/ and default didn't trigger, I guess since I'm editing an existing user object? But it was one where this field value did not already exist, so the lack of default was confusing. Are you sure validation is working on edit? Because it didn't seem to be for me.

@#97: Yeah, I was just documenting everything I found. I tried to explicitly mention in #95 that only a few of those are worth fixing in this patch.

yched’s picture

re webchick #100:
oh, crap, forgot you reported that on a user edit form. Didn't test validation there. If it doesn't work, it's a bug in user.module.
About default values : yes, default values only come into play on new object creation. That's the common behavior since CCK D5. Not really sure how and if we want to change that for 'but ma'm, the field didn't exist when I created the object', but not for this thread ;-).

yched’s picture

Side note: as could be expected, this patch will open the gate for a flood of strictly Field API reports... Probably good that it happens now rather than later, but this will require additional community help ;-)

webchick’s picture

yched: I'm hoping that with the flood of Field API reports will come a flood of people with motivation for learning more about the field API now that they can 'see' it.. :)

bjaspan’s picture

At first glance, user.module should be validating form values:

<?php
/**
 * Validation function for the user account and profile editing form.
 */
function user_profile_form_validate($form, &$form_state) {
 
$edit = (object)$form_state['values'];
 
field_attach_form_validate('user', $edit, $form, $form_state);
?>
quicksketch’s picture

StatusFileSize
new888 bytes

For the next reroll, could you incorporate the attached change yched? The current field_ui_field_edit_instance_pre_render() function is meant to render all the widget and instance settings on the same level so that the weights can be distributed between both of them as if they were in the same fieldset. However the current function doesn't work, attached is a better version of this function that I'm using with FileField.

yched’s picture

StatusFileSize
new127.34 KB
Failed: 11359 passes, 3 fails, 0 exceptions
[ View ]

- Added a default primary tab for user settings on admin/settings/user (re webchick #95)

- Fixed the secondary tab still showing after field got removed (re Gabor #97)
Again, ideally I think they should go in favor of AHAH subforms from the 'Manage Fields' screen.

- Added quicksketch's #105 snippet.
Although I actually find the current 'common properties (label, required...) + instance settings + widget settings' are merged in a form soup with no logical structure is terrible UX.
Ideally, these would all be split in separate AHAH subforms - or at least 'common properties (label, required...) + instance settings', with 'widget settings' separate.
Less ideally, they would at least be in separate fieldsets in the form.
So allowing more mixup in the soup isn't a step in the right direction IMHO :-)

yched’s picture

Side note: I'll be mainly AFK for the next 3-4 days.
I think webchick intends to do a code review this weekend, and, being the webchick she is ;-), will most certainly come up with quite a few remarks, so help welcome for the rerolls meanwhile...

quicksketch’s picture

Great, thanks yched, I'll put another review up shortly with the next reroll.

Regarding the merging of options, I think the delimitation between "widget" and "instance" settings is very fuzzy for end-users (and even for developers). Some things like "Image size validation" seems like it should be right next to "File extension validation", yet "Image size validation" will be a widget setting versus an instance setting like "File extension validation". I think grouping them together will help with a reasonable ordering, but we'll definitely be leaving the flexibility to contrib developers to order things in undesirable ways, but that's a small risk for the reward of putting things into a reasonable order within core.

quicksketch’s picture

StatusFileSize
new16.6 KB
Failed: Failed to install HEAD.
[ View ]

Just a quick re-roll as I had something come up tonight. Changes:

- Corrected allowed_values t() replacements.
- Removed TODO from list_allowed_values(), since we already provide a mechanism for generating options through $field['settings']['allowed_values_function'], which can be set by other modules through hook_field_read_field().
- Removed changes to *.info files renaming "Core - fields" to just "Core", seems like it doesn't belong in this patch.

quicksketch’s picture

StatusFileSize
new123.49 KB
Failed: Failed to apply patch.
[ View ]

Weird, must have attached a partial patch.

quicksketch’s picture

StatusFileSize
new115.9 KB
Failed: 12152 passes, 1 fail, 16 exceptions
[ View ]

- Removed a few more TODOs by moving help text at admin/structure/node-type/x/fields and admin/structure/node-type/x/display to hook_help().
- Added in the variable documentation for field_ui-display-overview-form.tpl.php and field_ui-field-overview-form.tpl.php.
- Cleaned up API docs.
- Minor code style changes.
- Removed all fieldgroup handling, which will need to be incorporated separately.

Bojhan’s picture

StatusFileSize
new41.38 KB
new34.9 KB
new12.99 KB
new53.78 KB

Here is my initial review, some descriptions are quite bad - but I think we should not work on fixing those for this patch. This is my first CCK UI review, I avoided it - but, we need to ensure consistency in our interfaces.

fields_overview.png
This page is lacking quite some alignment, quicksketch told me its "Seven" related.

fields_tabs.png
We want to avoid using these tabs as much as possible, they do not hold the same ease of use compared to tabs.

existingfieldui.png
Does this hold the same usecase scenario as new fields?

Bojhan’s picture

StatusFileSize
new34.9 KB

fieldsinstructure.png

Although I like the abilities of this functionality, it should not be prominently placed under Structure.

Status:Needs review» Needs work

The last submitted patch failed testing.

quicksketch’s picture

Status:Needs work» Needs review
StatusFileSize
new115.83 KB
Failed: 11966 passes, 0 fails, 16 exceptions
[ View ]

This patch should accommodate for most of Bojhan's concerns:

- Alignment of text in table cell on admin/structure/x/fields: Not fixed. Center aligning just looked funnier than left-align, I couldn't see any real reason to make the change.
- Remove the "Add" row from admin/structure/x/fields: Fixed.
- Change "Configure" and "Remove" to "Edit" and "Delete": Fixed.
- Drop the field list from the sub-tabs: Fixed. Note that this broke the breadcrumbs, but since Seven doesn't have breadcrumbs you can't notice. This can be fixed by #483614: Better breadcrumbs for callbacks, dynamic paths, tabs.
- New/Existing field addition: Not fixed. There's no clear solution to this problem and I think it'd be best left to a follow-up, since this is what we've been using for all of the D6 release cycle.
- Move the "Fields" menu item: Fixed. I moved it to admin/reports/fields and named it "Field list" instead of "Fields". I agree that it didn't belong under Structure, since I was repeatedly clicking on it when I wanted to add a new field.

I also fixed the testing error, but when I run my tests locally I get a ton of PHP errors in the try {} catch {} statements, which I'm not sure show up in testing bot (this patch shouldn't cause this anyway I think).

Status:Needs review» Needs work

The last submitted patch failed testing.

Bojhan’s picture

StatusFileSize
new20.94 KB
new36.06 KB

I am not sure if you understood the alignment issue I was bringing up, I dont see how its related to center vs left alignment. I hacked the overview with nbsp.

Before
alignmentfieldsoverviewbefore.png

After
alignmentfieldsoverview.png

Bojhan’s picture

StatusFileSize
new6.79 KB

Can we not show this tab if we don't have fields?

displayfields.png

Frando’s picture

I think it's less confusing to always show the "Display fields" tab even if there are no fields. When reading the documentation, the first thing you usually do is clicking through the various interfaces without actually doing stuff, and therefore I think it will add quite some confusion if the "display fields" tab only appears after fields have been created. IMO.

catch’s picture

Just a note we should be removing body from content type settings when this is in since it's now covered by the manage fields tab, doesn't have to be done in this patch, but shouldn't be forgotten.

Bojhan’s picture

@Frando Well this is for terms? I think its quite unlikely, people encounter this before the normal content types admin.

@catch agreed

yched’s picture

- re Bojhan #17: -1 from me. Those cells use a colspan on purpose. Those 'pseudo fields' could have a longish text displayed here, while the 'Field' and 'Widget' columns will always be empty, so the colspan saves precious screen real estate.

- According to the latest screenshots, the 'add new' and 'add existing field' rows have lost their drag-n-drop handler ?

- 'Fields list' -> admin/reports/fields: not sure its the best choice. This info does belong in Structure IMO. Besides, the actual 'manage fields' screens will be scattered across different paths and menu locations for the various fieldable types and bundles, and the 'Structure > Fields' page could provide a handy shortcut to list all of them.
No big deal, and can definitely be discussed and adjusted in a followup task. That screen can receive lots of enhancements still anyway.

Bojhan’s picture

@yched I dont really get it, regarding #117. Could you provide an example? I do think we are talking about different things here. For a notion, mis alignment should not be a compromise for screen estate, since it confuses the eye - thereby, defeating the purpose of saving screen estate (making it more clear).

yched’s picture

Bojhan: I don't think we're talking about different things, the difference between your two screnshots in #117 is that the description text for non-fields spans over several columns. And I can't really provide an example of a long text here, this is free for contrib...
As a side note, that colspan has been there for about a year in CCK D6, and I can't remember of anyone reporting this as confusing.
This is followup discussion IMO :-)

webchick’s picture

I agree with Bojhan that the first screenshot in #117 looks incredibly odd, but I also agree with yched that fixing this could happen in a follow-up.

Obviously, this is causing exceptions at the moment which need to be fixed, but after that, yched/quicksketch could you give an indication on how far away you feel this is from being RTBC in your view and what, if anything, you're looking for from reviewers to get it there? I'd say this is probably the most important issue in the queue atm.

Bojhan’s picture

The "but its been like this in contrib notion" argument is never valid - since this is not an opinion thing and its core (other standards, also UI). So agreed lets keep this issue to a follow up then.

yched’s picture

field_ui_field_overview_form():

+      'parent' => array(
+        '#type' => 'hidden',
+        '#default_value' => ''
+      ),

that was part of the fieldgroup code, should be removed as well, I guess ?
Same thing for the 'hidden_name' bits, i think - and also the #leaf / #root stuff.
Additional work will be needed to allow 3rd party modules to add their own rows, and we might find we need to add some of this back in, but if we're up for removing fieldgroup-related code before we commit, it's probably best to wipe all of it and start clean.

+    $row->class .= isset($element['#add_new']) ? 'add-new' : 'draggable';

Previously we had

+    $row->class = 'draggable';
+    $row->class .= isset($element['#add_new']) ? ' add-new' : '';

We still want the 'add new' rows to be draggable, IMO. Besides, other than this change, we still have all the supporting code...

quicksketch’s picture

Status:Needs work» Needs review
StatusFileSize
new37.04 KB
new115.83 KB
Failed: 12192 passes, 0 fails, 16 exceptions
[ View ]

I'm not a big fan of the draggable addition field in this case (even though it was my suggestion to begin with), but we need to figure out that addition mechanism all over again anyway, since the 3 addition rows is pretty terrible no matter what. I've added back in the draggable row in this version.

This version should fix the PHP notices (I think), removes the lingering references to fieldgroup and parents, and makes a compromise on the #117 issue Bojhan brought up. Now we show the the field (machine) "name" in addition to the description, so the description only spans the field and widget columns, rather than name, field, and widget columns. Even though it's redundant in this screenshot, it won't be in all cases (such as if Title or Body labels were changed):

Status:Needs review» Needs work

The last submitted patch failed testing.

Bojhan’s picture

Status:Needs work» Needs review

I am going to mark this oke for the first pass of the UI, although there is loads of work to do - it shouldn't derail this issue.

quicksketch’s picture

StatusFileSize
new115.37 KB
Failed: 11411 passes, 3 fails, 0 exceptions
[ View ]

Hrmm, I can't reproduce these notices locally (though I could last patch). Here's another patch for test-bot to chew on. Also fixed "Edit" to "edit" and "Delete" to "delete" in the Operations column.

quicksketch’s picture

Yay, passing tests now. I think my last patch was mis-referenced somehow. Anyway, to answer webchick's question about "how ready is this?": I think it's ready to go for a first pass. The original scope of this patch was to "Move CCK HEAD into core" but we've already done much more than that: abstracting out the UI entirely, moving default value handling, rewriting available options for selects, plus several visual tweaks. It just goes to show that things actually *in core* get a lot more attention than things "sort of in core".

Getting this in now will open up a flood of other tasks, not only bug fixes but also finally killing Upload and Profile module. Considering our short time-line, getting this in will spike enthusiasm and eyeballs looking at this code.

sun’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new116.72 KB
Failed: 11411 passes, 3 fails, 0 exceptions
[ View ]

What a monster patch! Awesome work! :)

Instead of pasting a Dreditor review that would be longer than the entire issue, I re-rolled with a variety of smaller changes.

The code is mostly sane. Here and there a bit more inline comments would be helpful. But I would highly recommend to defer that to another issue.

So. This is to 99.9% the same patch as quicksketch's (no functional changes, only clean-ups), so I would like to kindly ask Dries or webchick to commit this as is. We need to move forward and build on top of this functionality as soon as possible.

quicksketch’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new116.7 KB
Failed: Failed to apply patch.
[ View ]

Hrm, you changed all my "Implement hook_x()" to "Implements hook_x()", which is incorrect (at least compared with current core). Same patch with the correct PHPdoc for hook implementations.

quicksketch’s picture

Status:Needs review» Reviewed & tested by the community

Oops, I'll leave at RTBC at least until webchick takes a crack at this.

webchick’s picture

The "sun touch" makes for really boring patch reviews. :)

Summary: This patch adds a unified field UI to core that's essentially D6 CCK with some minor cosmetic enhancements. This same field UI is used to add fields to nodes, users, terms, as well as (depending on how the last couple weeks of code thaw goes) blocks, comments, and files. We'll be doing away with the confusing pattern of "If you're in profile module, you add fields this way, if you're in nodes you add fields that way..." in favour of one unified user experience. This is a big usability win. It also means that we can start to get some "real world" testing for Field API by users, which will not only make other patches MUCH easier to review, but will likely uncover bugs that have laid dormant ever since field API's inception.

I spent about an hour with the code in this patch, and apart from some things like a missing example from the hook_field_formatter_settings_form documentation, and a couple places where $description is being printed directly without being escaped first (which may or may not be a bug, not sure), I really couldn't find too much to complain about. There are still @todos, but they're clearly marked, and most of them are of the "There has to be a better way to do this" variety. All of this falls under "We can clean it up once this lands."

The follow-up issues that I'm aware of:
* Remove Title/Body UI from admin/structure/node-type/article, in favour of unified field UI.
* Figure out where to stick field UI screens for comments.
* Evaluate and discuss places where it may be desirable to "special case" field UI settings -- taxonomy autocomplete comes to mind, since you almost never want to only select a maximum of one tag, yet 1 is the default for all fields.
* TESTS. :P We might want to break this up into a series of "Novice" patches to expose new users to writing these.

(feel free to add others)

I was worried that we'd basically end up cramming something sort of half-working just to get it in, but this code definitely "feels" core-worthy to me at this point. I was actually pretty shocked at how easy it was to follow along with the patch after all of these various clean-ups. And here I thought CCK was so scary. ;) Kudos to everyone who provided reviews and patches on this issue... the quality of work really shines through, IMO. My one regret is we don't have test coverage for this, but this patch is already well large enough, and writing tests post-freeze will help us catch and fix bugs before release.

Between about 4-5 major review sessions, I've spent several hours with this patch in its various forms, testing lots of edge cases (running into #549726: user_profile_form_validate() not called when submitting user_profile_form on more than one occasion -- sorry for the false alarms! ;)). I feel like this is ready, not only to act as a starting point, but also to let people tear into it and start building awesome sites.

I confirm RTBC, but want to check with Dries before committing this.

kika’s picture

Likely a followup material but if we are so close, can we hit the final nail to the CCK acronym coffin -- meaning removing a reference to it?

+       $output .= '<li>' . t("<em>Text</em>: Adds text field types. A text field may contain plain text only, or optionally, may use Drupal's input format filters to securely manage HTML output. Text input fields may be either a single line (text field), multiple lines (text area), or for greater input control, a select box, checkbox, or radio buttons. If desired, CCK can validate the input to a set of allowed values.") . '</li>';

Note that there are at least 4 mentions of "CCK" in core without this patch #552084: Remove the references to "CCK" in the core. So, more nails to be hammered.

figaro’s picture

+1 for substitution of CCK acronym

Gábor Hojtsy’s picture

StatusFileSize
new73.19 KB
new34.07 KB

I still think that the first screen upon adding a field is confusing, but otherwise adding, configuring and deleting fields seem to work all as expected to me. If I was not clear above, I'd repeat this initial screen issue:

- add a field to the article content type
- you got redirected to a page with settings for the field
- BUT the field has no global settings, and even though we are apparently under the Article content type (see page title), Article specific settings are not displayed either
- go to manage fields again and to edit the particular field and now you get both Article specific settings and actual global settings (which in the previous screen were said to be nonexistent)

Initial screen on adding the field:

Going back to the "same" screen in the navigation:

I'm not going to push for solving this before committing, but it is certainly a very core confusion point IMHO.

sun’s picture

Yeah, there are probably a lot of more simple tasks involved here. However.

Currently, this patch can be re-rolled by 2-3 people. (I had hard times to do so myself.)

When this patch has been committed, hundreds of developers are able to work in parallel on all remaining tasks.

mcrittenden’s picture

Subscribe

webchick’s picture

FWIW I totally agree with Gábor's #139, esp. given that these exact same settings appear to be alterable as well on the next settings page. Very confusing. However, I also agree with sun's #140 that solving issues like this are much easier when we have a base in core to work from.

Gábor Hojtsy’s picture

Yeah I totally agree with @webchick and @sun. Such a happy family :)

Dries’s picture

I've spent nearly 2 hours with this patch.

First, there is so much broken about the UX of Fields UI, and even the features don't work as expected yet (random example: taxonomy term formatters are not working when they are set to 'plain text'). It is difficult to even begin writing a review, so I won't at this point.

However, at the risk of introducing another "Drupal 4.7 Form API nightmare", I support us committing this patch.

This will require a ton of follow-up work. I'd actually recommend that we create the follow-up issues before committing this patch. We'll need to track those carefully, and having a list of all the big follow-up issues will give us a better feeling with the size of the effort ahead of us.

So, maybe, before we commit this, we can work together on extracting all the most important follow-up issues and linking them from this post?

Bojhan’s picture

webchick’s picture

I spoke with Dries earlier today, and he has serious concerns about this getting in without the requisite clean-up we would require of other patches to be "core worthy". I therefore spent a few hours to do a "webchick pass" on the UI tonight, to try and document everything I see that is messed up and/or makes no sense. Apologies if anything of this overlaps Bojhan and Gábor's comments... I tried to go into this relatively "fresh". Some of these might not be worth fixing, but I wanted to err on the side of documenting everything I saw.

[bug] WSOD when creating/editing nodes with fields attached
Seemingly randomly (I think when I try and enter invalid settings on a field), when I try and create a new node with a field attached to it, I get a WSOD followed by the following errors on the next page:

    * Warning: include(/Users/webchick/Sites/core/includes/maintenance-page.tpl.php): failed to open stream: No such file or directory in theme_render_template() (line 1145 of /Users/webchick/Sites/core/includes/theme.inc).
    * Warning: include(): Failed opening '/Users/webchick/Sites/core/includes/maintenance-page.tpl.php' for inclusion (include_path='.:/Applications/MAMP/bin/php5/lib/php') in theme_render_template() (line 1145 of /Users/webchick/Sites/core/includes/theme.inc).

[bug] Formatters are lost after saving a field
Nate gets credit for finding this one. But set some display options, go back and save one of your fields, and PRESTO! Formatter selections are gone. :\

General UI Comments
The "look" of Field UI is pretty non-standard to anything else in Drupal. The first screen would leave me as a new user with a lot of questions, including what the hell is a "widget"? :)

Questions

Next, the "flow" of things is pretty jarring. Upon submitting a new field I'm taken to an intermediate page for field settings:

Field Settings

Sometimes said page says "there's nothing to configure here". Well then DON'T show me a page! ;) When I click "Save field settings" I'd expect to get taken back to where I was, like all other settings pages in Drupal. Instead, I'm taken to another settings form, this one much longer, but which includes the settings I configured a second ago! Very silly, and very confusing.

Once the field is created, I find that it exposes not one, not two, but THREE settings forms, two of them with "mystery meat" links:

Settings Galore!

http://localhost/core/admin/structure/node-type/page/fields/field_new_fi... Settings for the field type itself only, related to default values, data storage size, etc.
http://localhost/core/admin/structure/node-type/page/fields/field_new_fi... A widget selection only.
http://localhost/core/admin/structure/node-type/page/fields/field_new_fi... That long form again, which has a duplicate of the settings form at field-settings.

[docs] Things that need to be communicated
The documentation and help text for this needs to convey some pretty hefty concepts to people:
- What's a field vs. a widget vs. a display.
- That the same field can be attached to multiple entities.
- In the settings form, there are certain settings that affect all places a field is added, and others where it only applies to this "instance."
- Some things you can change once you've created a field, other things you can't.

Inconsistent UI for Title and Body Field
Title and Body have their own UI for changing their field labels under the content type edit page. This should go away, and use Field UI instead.

Title and Body

Within the field UI itself, there are no edit links on Title (so you cannot do things like set a maximum length, or specify plain-text vs. filtered HTML which would be very nice), which is also inconsistent. We also expose the word "Node" here, which is against our UI guidelines.

Title inconsistency

And finally, the settings for Body field could use some love. There's an option to turn on/off the summary field but you'd never know it by its name. There's a preview for the field here (which is actually kind of nice) which is inconsistent from other fields. As a result of this being a preview instead of input boxes asking for default values, the "Body" part of this field is LONG and scrolls way down.

Body settings

Oddities with Drag and Drop
When using the drag and drop on the fields, random borders appear out of nowhere and there appears to be general some styling issues with Seven. Haven't tried in Stark.

Drag and drop

Ensure Accessibility Compliance
Speaking of things like Drag & Drop, we need to ensure that this UI meets accessibility standards. The Accessibility Group should be pinged for a review.

Booleans/Checkboxes
This whole screen/experience for a boolean field is pretty confusing. I think this points to the fact that fields need to be able to affect settings pages to perform additional validation and/or limit the values shown for things like "Number of values". This would help with other field/widget types such as taxonomy term autocomplete as well (where it should really default to "Unlimited" rather than "1"):

Allowed Values

Boolean settings

"Tab-itis" on certain fieldable items

Compare:

Content types (3 tabs: edit, manage fields, display fields)
Content types

Users (3 tabs: settings, manage fields, display fields)
Users

Taxonomy (5 tabs: edit vocabulary, list terms, add term, manage fields, display fields)
Taxonomy

We desperately need some consistency here. I think we should get rid of the "list terms" and "add term" tabs from taxonomy, change "edit vocabulary" to "edit" (I can already see from the title what I'm editing IMO), and make "list" and "add" operations you can only do from the main listing. This would be consistent with content types, which people have been using for years with CCK and no one complaining.

For users, I think we should make "User profiles" its own section and stick manage and display fields tabs there, de-coupling them from settings which really has nothing to do with anything.

No way to add fields to comments
We have fieldable comments, but no UI. That needs to be fixed.


SUMMARY
So, is there more clean-up work to do? Clearly, yes. However, I still stand behind my opinion that we need to commit this in its current state to make progress on it, and the sooner we do so, the better off Drupal 7 will be.

Everett Zufelt’s picture

Issue tags:+accessibility

Although I do not have time to review this component for accessibility. I am happy to provide any assistance that I can to assist in ensuring that this is accessible.

webchick’s picture

Issue tags:-accessibility

Ok. I've expanded out Bojhan's list at #145 with additional issues that I've found. That's definitely not everything but it's at least a good chunk of the clean-up work left to do.

I believe this satisfies Dries's requirements, so I'm going to head to bed in a couple of minutes and then commit this in ~8-10 hours unless there are further objections raised.

webchick’s picture

Issue tags:+accessibility

Sorry! Didn't mean to reset the tag!

kika’s picture

Just for a reference: some of the webchick's concerns are shared over #521468: Redesign content type editing (after CCK UI is in) http://drupal.org/files/issues/kika_contenttype_cleanup_problems_1.jpg (some of them already fixed) and there is a very rough proposal to fix some contenttype settings <-> field setup UI cleanup http://drupal.org/files/issues/kika_contenttype_cleanup_proposal_1.jpg

yched’s picture

A couple remarks:
- agreed that we need to commit and move from there
- we need a 'Field UI' component in the issue queue. Field UI adjustments flood the Field API issues ;-)
- if/when this gets committed, don't forget Karen in the credits, this patch is massively based on CCK D6 code and she did the bulk of D7 upgrade.

Dries’s picture

Thanks for the additional review. OK to get this committed, but we'll want to follow-up with more UX patches.

Gábor Hojtsy’s picture

Component:field system» Field UI

Field UI component added.

webchick’s picture

Component:Field UI» field system
Status:Reviewed & tested by the community» Needs work

Ok!!

Patch: committed!! WOO. :)
field_ui.module component: created!
marauding of field system component clean-up: in progress. ;)

Marking down to needs work so we don't lose this central list of issues in the shuffle.

webchick’s picture

Component:field system» field_ui.module

Oops. Cross-posted with Gábor. :) I went with field_ui.module, for consistency with our other modules' components.

seutje’s picture

how am I not subscribed to this yet?

webchick’s picture

Heh. Ok, so it appears that this module slows Drupal down by about 50,000% (give or take ;)).

The test suite used to take about 4 minutes for testing bot to run, and now it's more like 10. Since we don't have any test coverage for field UI yet, my money's on adding field_ui to the default profile's list of required modules.

Anyone who knows this code well enough to investigate what might be going on and if it's something relatively simple to fix?

moshe weitzman’s picture

I suspect it is slowness in the tests. The module does not slow down drupal. Perhaps thats what you meant.

I know that the field tests consume a ton of memory. Not sure about field_ui

Damien Tournoud’s picture

Category:task» bug

Well, this patch slowed down the tests by a factor of 2, which is not a good thing. At this time, we have no confirmation that this slowdown affects the tests only. Converting this to a bug.

Damien Tournoud’s picture

Status:Needs work» Needs review
StatusFileSize
new513 bytes
Passed: 12411 passes, 0 fails, 0 exceptions
[ View ]

Let's verify an hypothesis.

sun’s picture

Status:Needs review» Reviewed & tested by the community
Damien Tournoud’s picture

StatusFileSize
new3.41 KB
Passed: 12411 passes, 0 fails, 0 exceptions
[ View ]

Another hypothesis.

Damien Tournoud’s picture

Summary for those following at home:

- current HEAD: about 10 minutes
- current HEAD with field_ui disabled: about 8 minutes (a 20% gain!)
- current HEAD with the access callbacks of the field_ui module disabled: about 10 minutes

Damien Tournoud’s picture

StatusFileSize
new642 bytes
Failed: Failed to apply patch.
[ View ]

Let's try to remove the menu_rebuild() in field_attach_create_bundle().

webchick’s picture

Status:Reviewed & tested by the community» Needs work

I don't think there's anything here that's RTBC. Looks like we're just trying different test bot fixes.

And this could probably be done in an issue that doesn't have 500 subscribers. ;) but please ping back when you have the solution.

yched’s picture

Status:Needs work» Reviewed & tested by the community

Also, what are the facts pointing to the field_ui patch, rather than other patches that went in recently ? The install profiles refactors, for instance ?

yched’s picture

Status:Reviewed & tested by the community» Needs work

Didn't mean to change status

yched’s picture

Status:Needs work» Reviewed & tested by the community

So, the patch in #164 is a needed fix and is RTBC. field_ui takes care of the menu_rebuild() when it needs one, field.module has nothing to do with menus.

It seems to shave off 2-3 minutes of the additional 5 minutes on the test suite. I don't think this will add up with the 2-3 minutes gain provided by disabling field_ui (tested in patch #160), but it should be tested in a different thread once #164 is committed.

webchick’s picture

StatusFileSize
new642 bytes
Passed: 12191 passes, 0 fails, 0 exceptions
[ View ]

Hey there, bot. Can I entice you?

alexanderpas’s picture

8 minutes, testbot not complaining ;)

However, note that the difference between #89 and #90 is two minutes (from 5 mins to 7 mins)

webchick’s picture

Status:Reviewed & tested by the community» Needs work

I thought that might have the side effect of breaking links to node/add/foo after you created a content type, but I couldn't seem to trigger it manually, and testing bot is cool. Committed #164/169 to HEAD.

Leaving needs work for the next patch.

yched’s picture

@webchick : node/add/foo is not handled by field UI or API. The menu paths we care about here are field_ui's admin/structure/node-type/foo/fields and .../fields/display.

When a bundle gets created, there's high chance that we need to add new menu paths for the corresponding 'Manage Field' page. That's why field_ui's implementation of hook_field_attach_create_bundle() does a menu_rebuild(). The one we just removed gave us 2 menu_rebuild() for the price of one.

This being said, when a node type gets created, there's a high chance that node.module *already* triggers a menu_rebuild for it's own paths. I'm wondering if field_ui should just skip its menu_rebuild entirely, and leave it to calling code. Tricky when you consider non-node entities and bundles.

[edit: fixed quite a few typos and unfinished sentences - it's really after hours, here...]

moshe weitzman’s picture

Perhaps we should call variable_set('menu_rebuild_needed', TRUE);. Thats gonna avoid rebuilding menu many times in a page.

yched’s picture

Status:Needs work» Needs review
StatusFileSize
new608 bytes
Passed: 12436 passes, 0 fails, 0 exceptions
[ View ]

Gosh, looks like menu_rebuild_needed is exactly what we need here. Patch attached.

Shouldn't there be an API function to set that variable ?

Also, note that node_type_save() itself doesn't issue a menu_rebuild(), only node_types_rebuild() does. node_type_form_submit() does a node_type_save() then a node_types_rebuild(); but programmatic node type creation doesn't issue a menu rebuild (that is, other than the one triggered by field_ui if it's enabled).

More generally, it seems odd that only menu.test makes use of that menu_rebuild_needed variable. Not within the scope of this thread, obviously, just sayin'.

yched’s picture

tests with #174 ran in 3 min 20. Sounds like we have a winner.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

yched - yes, we need to use that variable elsewhere, including in field crud.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Awesome. Great sleuthing, guys! Committed to HEAD. :)

Back to needs work for whatever this was needs work for before ;P

webchick’s picture

Awesome. Great sleuthing, guys! Committed to HEAD. :)

Back to needs work for whatever this was needs work for before ;P

yched’s picture

re "Back to needs work for whatever this was needs work for before ;P":

#154: "Marking down to needs work so we don't lose this central list of issues in the shuffle."
I assume that's the list in #145 ?
Isn't the list of issues better followed with the field_ui project component ?
http://drupal.org/project/issues/search/drupal?text=&assigned=&submitted...

Taxoman’s picture

Subscribing.

Cliff’s picture

Subscribe. Although I might never have time to read all this before code freeze, I'll help in any way I can. :-)

Noyz’s picture

Re: 152, here's a screencast showing a possible solution to make CCK UI much more user friendly:

http://jeffnoyes.com/content/drupal-cck-fields-ui

moshe weitzman’s picture

Very nice.

There are devils in the details. For one, you are not somewhat ignoring the distinction between field level properties and instance level properties. Hopefully Barry or David can guide you through that if needed. Would be really really helpful if you stuck with this through the hard implementation issues.

We might need #557272: FAPI: JavaScript States system in order to simplify the UI for impossible pref combinations and so on.

Noyz’s picture

Thanks. In the mockup, you can set instance level and field level properties via the properties tab. I led with instance level because that's what you're creating. If field level properties have to be set first, we could reverse the order, or use smart defaults.

seutje’s picture

@182: I think the fields should at least look disabled while changing settings and stuff, since u can't fill them out anyway, or rather there's no point in filling them out... this might work confusing

Linea’s picture

I like this design since it seems to streamline the process of using CCK.

bangpound’s picture

I will leave the decisions to the experts and the people who will actually do this work. I know we've got the best minds on this problem.

At one of the BoFs in Paris, a bunch of us were sitting around discussing this problem. Someone laid out the problem in a clear way to me.

CCK Fields UI manages:

Fields
this is the data as stored, how it's stored, its type, etc.
Widgets
this is how the user interacts with the field's values in a form, and it can be instance specific (which also means different bundle contexts [nodes, users, terms, whatsits, doodads])
Formatters
this is how the field's values are displayed, and they can be displayed in all kinds of different build modes, in different contexts, on different bundles.

It seems that we're always conflating fields and widgets into the same UI. Widgets and formatters are managed separately. The options for widgets are going to be instance specific while field options are often specific to a field, which can be shared across many bundles.

Does anyone have any insight on whether it would be good, bad, already tested, not tested to pull FIELD management out of WIDGET management? (I have no idea what it would look like to manage fields separately from widgets.)

Noyz’s picture

Yeah maybe. Hard to say. This design is based on form builder. I can't speak for others, but I wasn't confused by the field not being disabled. Maybe your concern is an artifact of looking at a static prototype. Not sure. The right treatment can be settled user testing - which I'll be doing so long as the concept is well recieved.

At any rate, disabling the field is a very small part of this solution. I'd rather focus on identifying the bigger obstacles - if there are any.

David_Rothstein’s picture

Re #182-#184: I talked with Jeff about this a bit today, after checking with Barry for the things I wasn't 100% clear on myself -- he would have been a better choice, but was more busy than me :)

Anyway, here's a brief comment:

It looks to me like the mockups in #182 already do consider the difference between per-field and per-instance properties, but don't call them out too much. There is a "properties" tab in the mockups (when editing an existing field or creating a new one), and that is split up into per-field and per-instance sections -- similar to the existing Field UI in that respect.

The complicated issue is how to explain all this to the user in a clear way:

  1. When creating a new field, which properties won't be possible (or wise) to change later - what's the best way to communicate that?
  2. When editing an existing field for a particular instance, how to present the per-field properties in a way that explains that edits to them will affect other instances too? (And in some cases, might destroy existing data...)

And also, how not to let the above overwhelm the rest of the UI, especially since I don't think sharing fields between instances is necessarily even that common.

bangpound’s picture

@189 David_rothstein: Sharing fields — multiple instances of the same field — is going to be VERY common with taxonomy terms as fields. In D6, a vocabulary could be applied to many content types. In D7, a vocabulary is a field and the vocabulary/node type relationship is a field instance.

matt2000’s picture

Is D7 going to get the equivalent of D6's CCK content_permissions module? Per-field access control rocks....

quicksketch’s picture

Is D7 going to get the equivalent of D6's CCK content_permissions module? Per-field access control rocks....

Content permissions will likely become it's own module in Drupal 7 contrib.

webchick’s picture

Status:Needs work» Fixed

Marking this as fixed, since CCK HEAD did indeed move into core, and the field_ui.module component is now tracking follow-up bugs.

Status:Fixed» Closed (fixed)
Issue tags:-accessibility, -Drupal 7 priority

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