#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...
Comment | File | Size | Author |
---|---|---|---|
#174 | field_ui_menu_rebuild.patch | 608 bytes | yched |
#169 | 516138-performance-issue_1.patch | 642 bytes | webchick |
#164 | 516138-performance-issue.patch | 642 bytes | Damien Tournoud |
#162 | 516138-performance-issue.patch | 3.41 KB | Damien Tournoud |
#160 | 516138-performance-issue.patch | 513 bytes | Damien Tournoud |
Comments
Comment #1
quicksketchYay, 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.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedI prefer a separate, optional module. I would even keep 'CCK' as the namespace just for nostalgia.
Comment #3
yched CreditAttribution: yched commentedbangpound: ;-) 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.
Comment #4
sunSince we don't know what will happen and what can be achieved: field_ui.module.
Comment #5
webchickField 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.
Comment #6
shunting CreditAttribution: shunting commentedSeparating 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?
Comment #7
yched CreditAttribution: yched commented"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 :-)
Comment #8
shunting CreditAttribution: shunting commentedYes, 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...
Comment #9
yched CreditAttribution: yched commented"If the hooks are structured and applied in a consistent manner?"
Yes, hopefully so, but making predictions on yet unwritten modules is, er, difficult ;-)
Comment #10
shunting CreditAttribution: shunting commentedThe best way to predict the future is to invent it ;-)
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedYched:
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?
Comment #12
Bojhan CreditAttribution: Bojhan commentedSo 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.
Comment #13
kika CreditAttribution: kika commented+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?
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedyched: i'm happy to help with this patch. please let me know if i can do anything.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe
Comment #16
yched CreditAttribution: yched commentedHere'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
Comment #18
bjaspan CreditAttribution: bjaspan commented<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:
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>
Comment #19
webchick@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. :)
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedCall 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.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedAha! hook_field_widget_settings_form is declared twice in field.api.php.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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?
Comment #25
yched CreditAttribution: yched commentedre 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.
Comment #26
yched CreditAttribution: yched commentedre 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.
Comment #27
catchI'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.
Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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.
Comment #29
yched CreditAttribution: yched commentedbangpound: "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.
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commentedyched: 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.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedI made a new patch that accounts for the changes in default install profiles.
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedargh. i never remember to fix the status.
Comment #34
mlncn CreditAttribution: mlncn commented/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.
Comment #35
yched CreditAttribution: yched commentedfieldui / 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.
Comment #36
bjaspan CreditAttribution: bjaspan commentedHmmm... "feeldwhee" does not sound like a great module name to me. Let's break with tradition and use an underscore. :-)
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedWhat'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:
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.
Comment #38
yched CreditAttribution: yched commentedFixed 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.
Comment #39
BerdirBecause everyone hat... erm, loves my DBTNG reviews, here's another one ;)
- 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
There is also a similiar delete statement.
That should use db_delete()...
and this db_update()
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, ...)
Comment #40
yched CreditAttribution: yched commentedThanks 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 ;-)
Comment #41
yched CreditAttribution: yched commentedThis should take care of Berdir's remarks in #39.
Comment #42
tic2000 CreditAttribution: tic2000 commentedCan 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:
If I go back, the field is added and works.
Comment #43
yched CreditAttribution: yched commentedSorry, 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.
Comment #44
tic2000 CreditAttribution: tic2000 commentedWhen you create the patch on the second screen select "Project" as your "Patch root". That will do the trick of -p.
Comment #45
yched CreditAttribution: yched commentedUpdated 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'.
Comment #46
tic2000 CreditAttribution: tic2000 commented@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.
Comment #47
int CreditAttribution: int commentedIn 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?
Comment #48
yched CreditAttribution: yched commentedRemoved .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 ;-).
Comment #49
yched CreditAttribution: yched commentedand the patch.
Comment #51
moshe weitzman CreditAttribution: moshe weitzman commentedInstalled from scratch and tried this out:
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.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedHere'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.
Comment #54
yched CreditAttribution: yched commentedDoh, 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.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedWhat does this patch have to do with book? The 8 fails are at Book functionality.
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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);
Comment #59
webchickTagging.
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.
Comment #60
yched CreditAttribution: yched commentedProblem 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.
Comment #61
yched CreditAttribution: yched commentedOK, 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.
Comment #62
quicksketchSo 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 infield_ui_extra_field_values()
,field_ui_build_modes_tabs()
,field_ui_field_type_options()
,field_ui_widget_type_options()
, andfield_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.
Comment #63
yched CreditAttribution: yched commented- 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.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedyched:
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?
Comment #65
plachsubscribe
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedyched:
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?
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch incorporates all of quicksketch's comments in #62.
Comment #68
quicksketchI'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.
Comment #69
yched CreditAttribution: yched commentedMany 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 :-/
Comment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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.
Comment #71
yched CreditAttribution: yched commentedOK so far.
Sure, this cleanup is done in #367753: Bulk deletion.
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 ?
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commentedMove this to another issue if you think it's appropriate.
To reproduce in Field UI:
Comment #73
yched CreditAttribution: yched commentedBug 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.
Comment #74
quicksketchHere'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.
Comment #75
yched CreditAttribution: yched commentedW00t !
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:
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 :-)
field_default_insert() should be updated to this new code as well (possibly abstracted out to a helper function, then ?)
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.
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' ?
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?
Comment #76
quicksketchExcellent, 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.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.
Comment #77
moshe weitzman CreditAttribution: moshe weitzman commentedDuring 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.
Comment #78
quicksketchThanks for the look Moshe. It seems your sentence is missing a verb:
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.
Comment #79
webchickJust 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.
Comment #80
bjaspan CreditAttribution: bjaspan commentedDeleting 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.
Comment #81
yched CreditAttribution: yched commentedfield_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.
Comment #82
yched CreditAttribution: yched commentedCalling 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.
Comment #84
bjaspan CreditAttribution: bjaspan commentedRe-rolled.
Comment #86
Gábor HojtsyFailed to install HEAD sounds like an issue with the testbot?
Comment #87
Gábor HojtsyHum, no. The latest patch in fact does not contain the field_ui module. No wonder it was just 35k.
Comment #88
bjaspan CreditAttribution: bjaspan commentedI guess my re-roll wasn't so helpful. :-)
Comment #89
yched CreditAttribution: yched commentedNew attempt :-)
Comment #90
yched CreditAttribution: yched commented+ 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).
Comment #91
yched CreditAttribution: yched commentedMh, #90 left out quite a few places...
Comment #92
webchickCould someone provide a summary of how close this is to me tearing into it for a final pass? :)
Comment #93
yched CreditAttribution: yched commentedbangpound 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.
Comment #94
yched CreditAttribution: yched commentedUpdated 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.
Comment #95
webchickI 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:
* 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:
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!
Comment #96
Gábor Hojtsy@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
Comment #97
Gábor HojtsyDid 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:
Comment #98
yched CreditAttribution: yched commentedre webckick #95:
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.
Comment #99
yched CreditAttribution: yched commentedUpdated 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.
Comment #100
webchick"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.
Comment #101
yched CreditAttribution: yched commentedre 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 ;-).
Comment #102
yched CreditAttribution: yched commentedSide 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 ;-)
Comment #103
webchickyched: 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.. :)
Comment #104
bjaspan CreditAttribution: bjaspan commentedAt first glance, user.module should be validating form values:
Comment #105
quicksketchFor 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.
Comment #106
yched CreditAttribution: yched commented- 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 :-)
Comment #107
yched CreditAttribution: yched commentedSide 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...
Comment #108
quicksketchGreat, 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.
Comment #109
quicksketchJust 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.
Comment #110
quicksketchWeird, must have attached a partial patch.
Comment #111
quicksketch- Removed a few more TODOs by moving help text at
admin/structure/node-type/x/fields
andadmin/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.
Comment #112
Bojhan CreditAttribution: Bojhan commentedHere 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?
Comment #113
Bojhan CreditAttribution: Bojhan commentedAlthough I like the abilities of this functionality, it should not be prominently placed under Structure.
Comment #115
quicksketchThis 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).
Comment #117
Bojhan CreditAttribution: Bojhan commentedI 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
Comment #118
Bojhan CreditAttribution: Bojhan commentedCan we not show this tab if we don't have fields?
Comment #119
Frando CreditAttribution: Frando commentedI 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.
Comment #120
catchJust 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.
Comment #121
Bojhan CreditAttribution: Bojhan commented@Frando Well this is for terms? I think its quite unlikely, people encounter this before the normal content types admin.
@catch agreed
Comment #122
yched CreditAttribution: yched commented- 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.
Comment #123
Bojhan CreditAttribution: Bojhan commented@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).
Comment #124
yched CreditAttribution: yched commentedBojhan: 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 :-)
Comment #125
webchickI 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.
Comment #126
Bojhan CreditAttribution: Bojhan commentedThe "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.
Comment #127
yched CreditAttribution: yched commentedfield_ui_field_overview_form():
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.
Previously we had
We still want the 'add new' rows to be draggable, IMO. Besides, other than this change, we still have all the supporting code...
Comment #128
quicksketchI'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):
Comment #130
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #131
quicksketchHrmm, 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.
Comment #132
quicksketchYay, 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.
Comment #133
sunWhat 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.
Comment #134
quicksketchHrm, 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.
Comment #135
quicksketchOops, I'll leave at RTBC at least until webchick takes a crack at this.
Comment #136
webchickThe "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.
Comment #137
kika CreditAttribution: kika commentedLikely 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?
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.
Comment #138
figaro CreditAttribution: figaro commented+1 for substitution of CCK acronym
Comment #139
Gábor HojtsyI 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.
Comment #140
sunYeah, 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.
Comment #141
mcrittenden CreditAttribution: mcrittenden commentedSubscribe
Comment #142
webchickFWIW 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.
Comment #143
Gábor HojtsyYeah I totally agree with @webchick and @sun. Such a happy family :)
Comment #144
Dries CreditAttribution: Dries commentedI'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?
Comment #145
Bojhan CreditAttribution: Bojhan commentedAs requested by Dries, a list of existing issues.
Existing issues:
New issues:
Powered by quicksketch and webchick
Comment #146
webchickI 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:
[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.
Comment #147
Everett Zufelt CreditAttribution: Everett Zufelt commentedAlthough 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.
Comment #148
webchickOk. 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.
Comment #149
webchickSorry! Didn't mean to reset the tag!
Comment #150
kika CreditAttribution: kika commentedJust 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
Comment #151
yched CreditAttribution: yched commentedA 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.
Comment #152
Dries CreditAttribution: Dries commentedThanks for the additional review. OK to get this committed, but we'll want to follow-up with more UX patches.
Comment #153
Gábor HojtsyField UI component added.
Comment #154
webchickOk!!
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.
Comment #155
webchickOops. Cross-posted with Gábor. :) I went with field_ui.module, for consistency with our other modules' components.
Comment #156
seutje CreditAttribution: seutje commentedhow am I not subscribed to this yet?
Comment #157
webchickHeh. 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?
Comment #158
moshe weitzman CreditAttribution: moshe weitzman commentedI 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
Comment #159
Damien Tournoud CreditAttribution: Damien Tournoud commentedWell, 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.
Comment #160
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's verify an hypothesis.
Comment #161
sunComment #162
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnother hypothesis.
Comment #163
Damien Tournoud CreditAttribution: Damien Tournoud commentedSummary 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
Comment #164
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's try to remove the menu_rebuild() in field_attach_create_bundle().
Comment #165
webchickI 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.
Comment #166
yched CreditAttribution: yched commentedAlso, what are the facts pointing to the field_ui patch, rather than other patches that went in recently ? The install profiles refactors, for instance ?
Comment #167
yched CreditAttribution: yched commentedDidn't mean to change status
Comment #168
yched CreditAttribution: yched commentedSo, 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.
Comment #169
webchickHey there, bot. Can I entice you?
Comment #170
alexanderpas CreditAttribution: alexanderpas commented8 minutes, testbot not complaining ;)
However, note that the difference between #89 and #90 is two minutes (from 5 mins to 7 mins)
Comment #171
webchickI 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.
Comment #172
yched CreditAttribution: yched commented@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...]
Comment #173
moshe weitzman CreditAttribution: moshe weitzman commentedPerhaps we should call variable_set('menu_rebuild_needed', TRUE);. Thats gonna avoid rebuilding menu many times in a page.
Comment #174
yched CreditAttribution: yched commentedGosh, 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'.
Comment #175
yched CreditAttribution: yched commentedtests with #174 ran in 3 min 20. Sounds like we have a winner.
Comment #176
moshe weitzman CreditAttribution: moshe weitzman commentedyched - yes, we need to use that variable elsewhere, including in field crud.
Comment #177
webchickAwesome. Great sleuthing, guys! Committed to HEAD. :)
Back to needs work for whatever this was needs work for before ;P
Comment #178
webchickAwesome. Great sleuthing, guys! Committed to HEAD. :)
Back to needs work for whatever this was needs work for before ;P
Comment #179
yched CreditAttribution: yched commentedre "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...
Comment #180
Taxoman CreditAttribution: Taxoman commentedSubscribing.
Comment #181
Cliff CreditAttribution: Cliff commentedSubscribe. Although I might never have time to read all this before code freeze, I'll help in any way I can. :-)
Comment #182
Noyz CreditAttribution: Noyz commentedRe: 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
Comment #183
moshe weitzman CreditAttribution: moshe weitzman commentedVery 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.
Comment #184
Noyz CreditAttribution: Noyz commentedThanks. 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.
Comment #185
seutje CreditAttribution: seutje commented@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
Comment #186
Linea CreditAttribution: Linea commentedI like this design since it seems to streamline the process of using CCK.
Comment #187
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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:
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.)
Comment #188
Noyz CreditAttribution: Noyz commentedYeah 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.
Comment #189
David_Rothstein CreditAttribution: David_Rothstein commentedRe #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:
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.
Comment #190
Anonymous (not verified) CreditAttribution: Anonymous commented@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.
Comment #191
matt2000 CreditAttribution: matt2000 commentedIs D7 going to get the equivalent of D6's CCK content_permissions module? Per-field access control rocks....
Comment #192
quicksketchContent permissions will likely become it's own module in Drupal 7 contrib.
Comment #193
webchickMarking this as fixed, since CCK HEAD did indeed move into core, and the field_ui.module component is now tracking follow-up bugs.