Move CCK HEAD in core

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.

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

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.

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?

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...

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

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.

+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?

subscribe

<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>

@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. :)

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.

/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.

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

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

$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, ...)

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.

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

@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.

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?

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.

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.

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.

subscribe

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.

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.

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.

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.

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.

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.

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.

Re-rolled.

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

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

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

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

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.

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:

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:

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:

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:

...when editing my profile page and:

...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:

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":

---

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!

@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

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:

"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: 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.. :)

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

/**
 * 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);

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.

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.

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.

Weird, must have attached a partial patch.

- 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.

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.

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

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

Does this hold the same usecase scenario as new fields?

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

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).

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

After

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

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.

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.

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

@catch agreed

@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).

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.

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.

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):

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.

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.

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.

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.

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.

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

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.

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.

+1 for substitution of CCK acronym

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.

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.

Subscribe

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.

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

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?

As requested by Dries, a list of existing issues.

Existing issues:

• #445700: confusing behavior between vertical tabs and 'manage fields' tab
• Formatter settings, we have an API for it but the UI still doesn't support it: #226154: Formatter settings on display fields tab and in Views
• #552084: Remove the references to "CCK" in the core
• #537750: Field UI for comments

New issues:

• #552604: Adding new fields leads to a confusing "Field settings" form
• #552620: [meta-issue] Rewrite descriptions of content types UI
• #552630: Usage of fields display tabs on terms
• #552634: Manage fields overview
• #552654: Manage Fields and Display Fields links from the Content Types operation table
• #553284: Intermittent WSOD when creating/editing nodes with fields attached
• #553292: Formatter settings lost when saving a field
• #553298: Redesign the 'Manage Display' screen
• #553300: [meta-issue] Discuss/Improve user-facing Field terminology
• #553304: Update Field UI docs
• #553306: Make nodes have no body field by default. Remove deprecated APIs for body field
• #553308: Clean up UI for 'Text with summary' field settings
• #553312: Minor formatting problems with Field UI drag and drop
• #553314: Allow fields/widgets to alter Field UI settings forms
• #553324: Create an actual interface for "allowed values"
• #553326: Provide consistency to the way field UI is presented
• #553328: Streamline field UI workflow

Powered by quicksketch and webchick

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"? :)

Next, the "flow" of things is pretty jarring. Upon submitting a new field I'm taken to an intermediate page for 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:

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.

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.

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.

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.

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"):

"Tab-itis" on certain fieldable items

Compare:

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

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

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

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.

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.

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.

Sorry! Didn't mean to reset the tag!

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

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

Field UI component added.

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.

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

how am I not subscribed to this yet?

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?

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

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.

Let's verify an hypothesis.

Another hypothesis.

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

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

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.

Hey there, bot. Can I entice you?

8 minutes, testbot not complaining ;)

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

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.

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

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

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

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

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

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

Subscribing.

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

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

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.

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.

@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

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

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.

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.

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

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.

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