We know pretty much know that CCK D6's fieldgroup module will remain in contrib.

The code generating and theming the 'Manage fields' and 'Manage display' pages in D6 natively included fieldgroups and drag-n-drop nesting.
That part was rightfully ripped out during #516138: Move CCK HEAD in core, leaving the followup task of ensuring contrib modules can extend those screens (read: insert their own rows in the tables) - or fieldgroup is dead in D7 :-).

Let's start with the 'Manage Fields' screen.

Most of the job can already be done using regular form_alter, to the exception of the actual theming of the rows.
The form is currently themed in field_ui-field-overview-form.tpl.php and its preprocessor, which hardcode a number of 'row types': regular field, non-field component (e.g node title before it was converted to Field API, or current taxonomy term name), 'add new field' row, 'add existing field' row.

Attached patch lets each row be themed separately. The above 'core' row types go through a newly added field_ui_field_overview_row.tpl.php, contrib 'fieldgroup' rows can be form altered in the form, and themed through a custom theme function with $form['group_name']['#theme'] = 'fieldgroup_whatever';

The patch also cleans a little the form helper #properties and styling classes.

Once this is in, we can apply the same pattern to the 'Manage display' form.

CommentFileSizeAuthor
#87 field_ui_extensible_616240-87.patch86.45 KByched
#86 field_ui_extensible_616240-86.patch85.99 KByched
#84 field_ui_extensible_616240-84.patch85.97 KByched
#83 field_ui_extensible_616240-83.patch79.66 KByched
#80 field_ui_extensible_616240-80.patch72.74 KBdawehner
#77 field_ui_extensible_616240-77.patch71.79 KByched
#75 field_ui_extensible_616240-75.patch71.86 KByched
#75 Manage_fields_with_fieldgroups.png24.62 KByched
#73 616240-field_ui.patch65.1 KBdawehner
#71 0001-dereine-re-roll-on-6_0_2.patch64.14 KBdawehner
#70 0001-dereine-re-roll-on-6.patch65.22 KBmarvil07
#68 0001-dereine-re-roll-on-6.patch65.45 KBmarvil07
#66 0001-task-616240-Make-Field-UI-screens-extensible-from-co.patch64.38 KBdawehner
#63 0001-task-616240-Make-Field-UI-screens-extensible-from-co.patch65.62 KBmarvil07
#58 field_ui_extensible-616240-58.patch69.08 KByched
#56 field_ui_extensible-616240-56.patch62.82 KByched
#55 field_ui_extensible-616240-55.patch21.72 KByched
#51 field_ui_extensible-616240-52.patch49.75 KByched
#50 field_ui_extensible-616240-51.patch47.15 KByched
#48 field_ui_extensible-616240-48.patch43.01 KByched
#46 field_ui_extensible-616240-46.patch38.69 KByched
#38 field_ui_table-616240-38.patch26.86 KByched
#36 field_ui_table-616240-36.patch27.01 KBmarcingy
#31 field_ui_table-616240-31.patch27.06 KByched
#29 field_ui_table-616240-29.patch27.07 KByched
#27 field_ui_table-616240-27.patch27.18 KByched
#26 field_ui_table-616240-26.patch27.29 KByched
#21 field_ui_table-616240-21.patch23.79 KByched
#18 field_ui_table-616240-18.patch23.89 KByched
#5 field_ui_extensible-616240-5.patch17.34 KByched
field_ui_extensible.patch14.61 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review

CNR.

One question for quicksketch:
In it's 'core' version, the form has 'flat' drag-n-drop behavior. The form preprocessor does

drupal_add_tabledrag('field-overview', 'order', 'sibling', 'field-weight');

A module like fieldgroup will want to add hierarchical d-n-d (nest field rows under a fieldgroup row). I'm wondering if a

drupal_add_tabledrag('field-overview', 'match', 'parent', 'class-parent', 'class-parent', 'field-name', TRUE, 1);

added in a separate, later contrib preprocessor will work ?

Note that the form already includes a hidden 'field_name' form element on each row to serve as the base of the hierarchical structure (who is a child of who), we left those during #516138: Move CCK HEAD in core. I'm wondering whether we should also add a 'parent' form element, though, or leave that to contrib to add (but there might be several contrib modules that want to add 'hierarchical rows' to the table...). Right now we're kind of in the middle.

yched’s picture

"I'm wondering whether we should also add a 'parent' form element, though, or leave that to contrib to add"
Hm, but from contrib, this 'parent' element won't be easily themed into core-themed 'field rows' at the place where tabledrag expects it.

quicksketch’s picture

"I'm wondering whether we should also add a 'parent' form element, though, or leave that to contrib to add"

Indeed, this is a tricky problem because the only way that contrib could provide this element in a themed table would be to completely override page and replace it with it's own themed table (at least this was the case in D6). Unless...

We now have things like hook_page_alter() and renderable arrays. If contrib can alter the table structure before it's rendered, then it can add the form element for "parent" in a new column and add it's own drupal_add_tabledrag() command. The discussion from #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable seems to imply this is already true.

quicksketch’s picture

yched: After reading the patch, it doesn't look like hook_page_alter() would help either before or after the patch, since columns are very hard-coded into certain positions in the .tpl.php file. Would it be possible just to build up the entire structure as an array, then just using theme_table()?

yched’s picture

Thx for jumping in, Nate.

I've been following a different track where I always add and display the 'parent' selector and simply replace it with array(#markup => '') at preprocess time if $form['#hierarchical'] hasn't been form_altered to TRUE in between. Patch attached.
It also has the advantage of always initializing the 'parent' selectors in the FAPI array, saving some if (isset(...)) dance if several contrib modules want to add hierarchical elements in there.

The theme_table() approach would indeed make the form truly extensible - not just add rows, but modify existing rows (e.g adding 'operations', which might be interesting for some 'pseudo fields'). OTOH, having the rows themed in a template really made adjusting the form a whole lot easier back in D6...
I'll try to play with the idea.

yched’s picture

recategorize.

Crosslinked in #80855: Add element #type table and merge tableselect/tabledrag into it, which would *really* help a theme_table()-like approach.

sun’s picture

Component: field system » field_ui.module

This is quite a challenge. Moving table rendering logic into a template file isn't very nice... We should think about #pre_render + #post_render + custom #types for field_ui.module here, as long as #type => table isn't as usable as we all want it to be.

sun.core’s picture

Priority: Critical » Normal

Not critical. Please read and understand Priority levels of Issues, thanks.

sun.core’s picture

#5: field_ui_extensible-616240-5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, field_ui_extensible-616240-5.patch, failed testing.

catch’s picture

Status: Needs work » Needs review

This blocks fieldgroup port from contrib, so is in fact critical, or at least 'major', so let's wait for that status to arrive before downgrading.

catch’s picture

Priority: Normal » Critical
Status: Needs review » Needs work

doh.

Bojhan’s picture

quicksketch: Could you perhaps put in some ideas, to get this issue moving again?

quicksketch’s picture

Heh, I'd actually been searching for this issue for a few days but I couldn't find it. Thanks for bubbling up in my tracker Bojhan. :-)

quicksketch’s picture

Assigned: Unassigned » quicksketch

I'm working on a generic "theme_form_table" function that will make it possible to put any form into a table in a generic fashion, hopefully without additional theming beyond what theme_table_form() provides. This should make it so that forms within table are not only extensible, but easy to recreate. Unfortunately as you might imagine, such code is a bit convoluted (take a look at theme_table() for instance), but should be entirely possible with a little work. This should eliminate at least two theme functions from field_ui module, though if we decide to apply it in other places (roles, filters, etc.) we could save quite a bit of duplicate form-into-table theming.

yched’s picture

@quicksketch : that would be plain awesome !
I was planning to take a look at http://drupal.org/project/table at some point, but it cannot be before another couple weeks :-/

moshe weitzman’s picture

any update quicksketch? this is a critical. thx.

yched’s picture

Status: Needs work » Needs review
FileSize
23.89 KB

Here's a work-in-progress patch.

- Uses a FAPI table element, initially stolen from http://drupal.org/project/table, simplified and specialized for a couple requirements specific to Field UI.
"polish and add http://drupal.org/project/table in core" is far beyond the scope of this thread. There's been several efforts for a generic FAPI table element in core, none succeeded in the couple past years, we won't repeat that here. This thread will have to do with a tailor made solution, that doesn't try to be an API addition, only internal refactoring.

- Only deals with 'Manage fields' screen for now. 'Display fields' will need a similar treatment once #796658: UI for field formatter settings settles.

- Cells with colspans mess the table. Working on it.

- Doesn't address yet the question of how easily contrib modules (fieldgroup.module or others) can form_alter hierarchical tabledrag into this otherwise flat tabledrag table.

- Actual form submission hasn't been updated yet to match the changes in the form structure. The patch disables submit handlers completely for now, in order not to harm test dbs :-)

The last submitted patch, field_ui_table-616240-18.patch, failed testing.

swentel’s picture

Subscribing, this is interesting for display suite port to D7 :)

yched’s picture

Should take care of colspans and submission workflow (testbot should tell us).

TODO : nested tabledrag added from contrib

Status: Needs review » Needs work

The last submitted patch, field_ui_table-616240-21.patch, failed testing.

yched’s picture

Assigned: quicksketch » yched

reassigning

apaderno’s picture

Status: Needs work » Needs review

#21: field_ui_table-616240-21.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, field_ui_table-616240-21.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
27.29 KB

Updated patch, should be ready for review now.

Addresses the question of hierarchical tabledrag (in a default core install, the table only offers flat reorder since there are no such things as fieldgroups et al. Contrib fieldgroup module needs to be able to form_alter its own rows, that will offer nested drag-n-drop, into the table).

We add all the elements needed to support hierarchical drag-n-drop in the core version of the form. All rows come with a "parent" dropdown select element and a hidden 'self_name' element, and we perform the additional call to drupal_add_tabledrag() to support hierarchical drag based on those.
In a default core install, this degenerates into flat reorder only, because all rows are marked as 'leaf' (instructing tabledrag.js that they can't accept children), and the "parent" select dropdown only has a 'none' option.

Fieldgroup (or similar) can
- add its own 'group' rows to the FAPI array. Using the correct structure, it will be rendered as a table row automatically
- add candidate parent rows to the available #options in the 'parent' select input on existing rows
- add its own validation / submit handlers to handle the actual parenting data submitted in the form
through a simple form_alter()

As a side bonus, it's now fairly simple to form_alter() the table (for example, add links in the 'Operations' column).

No visual or API change whatsoever.

yched’s picture

Slightly better version, uses array_reduce() to determine row order from parenting and weight data, instead of running drupal_render() on a dummy render array - see template_preprocess_field_ui_table().

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the code and this is a terrific cleanup. We have some decent test coverage here, so I'm comfortable with RTBC. Lets slay this critical and move on to Display Fields table.

yched’s picture

Fixed a comment typo, removed a now useless property ('#table_rows')

yched’s picture

note for Drieschick : the patch removes a file (modules/field_ui/field_ui-field-overview-form.tpl.php)

yched’s picture

Fixed another comment typo.

YesCT’s picture

#31: field_ui_table-616240-31.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I've been reviewing this in spurts for a couple of weeks. I think it's good to go. The only question I really had was whether it makes sense to genericize theme_field_ui_table() to something other contrib modules might call, but I see in #18 that yched's ruled this out. We're past API freeze anyway, so let's try for this goal again in D8.

Unfortunately the patch has gone stale. :( Could I get a quick re-roll?

apaderno’s picture

Status: Needs work » Needs review

#31: field_ui_table-616240-31.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, field_ui_table-616240-31.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
27.01 KB

Just a reroll.

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

RTBC

yched’s picture

Thks for taking care of the reroll, marcingy.
Patch just readds one of the '#title_display' => 'invisible' that was lost in the process.

CVS reminder for webchick : the patch removes modules/field_ui/field_ui-field-overview-form.tpl.php

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed this patch and committed it to CVS HEAD. Thanks.

yched’s picture

Title: Make field UI screens extensible from contrib » Make Field UI screens extensible from contrib - part II
Status: Fixed » Active

Thks Dries. Now on to #796658: UI for field formatter settings ? :-)

+ Reopening for the 2nd part : 'Manage Display' screen, after that other patch is in.

catch’s picture

Issue tags: +API change

Tagging this as an API change - even if it's an 'addition' it needs to go in that list for tracking.

yched’s picture

I'm not sure in what sense this is an API change - it's only internal refactoring. No biggie though.

Still waiting for #796658: UI for field formatter settings to get in...

apaderno’s picture

It's an API change all the times a new function is introduced. Strictly speaking, it's an API change also when it's introduced a new private function.

mcreature’s picture

Subscribing

thekevinday’s picture

subscribe

yched’s picture

Posting the current state of the (ongoing) work, to allow #824812: Future of fieldgroup - call for volunteers to move ahead in parallel.
Will provide more explanations when the patch is ready.

In short, the 'Manage display' screen adds a little complexity because of its 'Hidden' region. Basically, the patch adds $form['#regions'] so that the set of regions can possibly be made extensible too. The intent is to allow http://drupal.org/project/ds to plug into the existing 'Manage display' screens rather than duplicating them with its own in D6. Ongoing discussing with swentel on the best way to achieve that.

sun’s picture

Status: Active » Needs work

Looks like we need to get #812688: Organize image formatters around settings and #504564: Make summary length behave with fields in...

+++ modules/field_ui/field_ui.admin.inc
@@ -84,27 +84,34 @@ function _field_ui_reduce_order($array, $a) {
+function field_ui_table_pre_render($element) {
...
-  $list = drupal_map_assoc(element_children($elements));
+  $list = drupal_map_assoc(element_children($element));

We can keep $elements plural here to decrease the size of this patch.

yched’s picture

#46 had a couple bugs preventing real use. Here's an updated version.
Still @todo:
- breaks 'Manage fields' screen for now
- update visibility selects when dragging a whole group to the 'hidden' region

re #47 :
- 'formatters settings' patches : yep, the patch includes the dummy formatter settings hunks from #796658: UI for field formatter settings, because I need to test the feature with formatter settings on fields present in the default core node types (image, text).
- ack for $element / $elements. $elements actually makes more sense.

swentel’s picture

For those who want to test, don't bother about not applying to system.module, it only tries to change the the $Id$ tag at the top.

- edit -

I really need to read.

yched’s picture

New patch, still largely in progress but at least should not break 'Manage fields' screen anymore.
Removed the dummy system.module hunk (git merge hiccup).

The behavior of the 'Hidden' region is still buggy (dragging in and out of the 'Hidden' region or changing the 'format' select to 'hidden').

Combining tabledrag + hierarchy + region rows + parts with ajax update is a challenge. Lots of edge cases to care about...

yched’s picture

Updated patch for the new state of http://github.com/yched/field_group.
I should probably be updating this thread through a git branch of core as well...

webchick’s picture

Status: Needs work » Needs review

Think this should be "Needs review"?

yched’s picture

Status: Needs review » Needs work

Great to see that tests pass, but this is far from ready yet :-(.

webchick’s picture

Ah, sorry. :\

yched’s picture

Status: Needs work » Needs review
FileSize
21.72 KB

No worries :-).

Updated patch for current state of http://github.com/yched/field_group.
Field group integration with the core field ui pages should be 'fairly' stable now, I'll get back to the JS niceties.

This patch slightly changes the layout of $form_state['values'] on 'Manage fields', so actually CNR for the bot.

yched’s picture

Er, I totally did the wrong git flow.

Here is the right patch.

Status: Needs review » Needs work

The last submitted patch, field_ui_extensible-616240-56.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
69.08 KB

Daily update...
Should fix tests.

yched’s picture

Status: Needs review » Needs work

cool - back to CNW then...

andypost’s picture

When it's ready to test/review?

aspilicious’s picture

check the todo's

yched’s picture

In short, the PHP side is now ready, give or take minor code polish. But the JS side is tricky. Each 'row type' (field, extra_field, fieldgroup-that-core-is-not-supposed-to-know-about, ...) can have it own behavior wrt to being dragged in and out of the 'hidden' region.

Nothing impossible, but I haven't been able to find quality time to move this forward for the last two weeks :-(.

marvil07’s picture

Status: Needs work » Needs review
FileSize
65.62 KB

I am rebasing this patch on the actual HEAD, changing status only for test bot, like mentioned there is still work to do here.

on hook_field_formatter_settings_form() and hook_field_formatter_settings_summary() implementations of text_field module there is a hunk that is differing from the patch
on the patch: if ($display['type'] == 'text_trimmed' || $display['type'] == 'text_summary_or_trimmed') {
on HEAD: if (strpos($display['type'], '_trimmed') !== FALSE) {
I have left the HEAD version.

I am also removing the implementations of hook_field_formatter_settings_form() and hook_field_formatter_settings_summary(), since they seems to be added just for testing, and now we have real implementation of those functions in HEAD.

Update: now the comment make sense

yched’s picture

Thks marvil07. FWIW, I've been able to move this forward again a bit. I see where I'm heading, but this will still take a couple days to finalize and polish.

Status: Needs review » Needs work

The last submitted patch, 0001-task-616240-Make-Field-UI-screens-extensible-from-co.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
64.38 KB

Just a very small rerole. So it does still apply. The issues in #63 wasn't fixed yet.

Status: Needs review » Needs work

The last submitted patch, 0001-task-616240-Make-Field-UI-screens-extensible-from-co.patch, failed testing.

marvil07’s picture

here we(dereine & me) remove a parents index where it was not need

now, tests for field_ui pass!!

marvil07’s picture

Status: Needs work » Needs review
marvil07’s picture

minor style changes

dawehner’s picture

Make it possible to apply

Status: Needs review » Needs work

The last submitted patch, 0001-dereine-re-roll-on-6_0_2.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.1 KB

New patch which fixed the tests.

Could someone sum up what has to be done?

yched’s picture

Status: Needs review » Needs work

The JS part still needs major refactoring. In short : when dragging a row or changing the 'format' select from 'visible' to 'hidden', the behavior will be different for a regular 'field' row, or for a 'fieldgroup' row, because those have child rows on which the visibilty change needs to cascade down. So the behavior on the JS side needs to be 'pluggable'.

I''ve been moving forward lately, I'd say that I'm about 75% through. Will try to post a working / testable patch ASAP.
Meanwhile, unfortunately, I'm afraid I'm the critical path and not much help can come from the outside :-(.

FWIW, the patch should not result in any API or visual changes, only changes in $form structure - will break modules that form_alter things on the current 'Manage fields' / 'Manage display' forms, which should be pretty rare.

This possibly means that the issue can be demoted to 'major' and not hold a beta or RC release, if it makes people feel better :-).

yched’s picture

Status: Needs work » Needs review
FileSize
24.62 KB
71.86 KB

Updated patch. Still a couple todos (mainly doc), and possibly some more refactoring pending, but this can already be played with.
Will post a detailed patch summary when the patch is actually ready.

No visual or functionnal change on a default core install, but this is mainly intended to play with current state of fieldgroup : http://github.com/yched/field_group (the module is still mainly in proof-of-concept state, mainly focuses on integration with Field UI right now).

FWIW, sample screenshot with fieldgroups, but the point is mainly about getting the JS and tabledrag interaction right, which a static shot doesn't really convey.

Status: Needs review » Needs work

The last submitted patch, field_ui_extensible_616240-75.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
71.79 KB

Doh @ git --no-prefix option.

Status: Needs review » Needs work

The last submitted patch, field_ui_extensible_616240-77.patch, failed testing.

Berdir’s picture

Same fails as #71, I guess the changes from #73 need to be merged in.

dawehner’s picture

Status: Needs work » Needs review
FileSize
72.74 KB

Yes the changes to the tests of the file module has to be done.

Einkahumor’s picture

Subscribe

Crell’s picture

moo.

yched’s picture

Updated patch (including the file.test fixes, thks dereine) + updated http://github.com/yched/field_group accordingly.
Still a couple doc / comment @todos, but the code refactoring is mostly done.

Will try to finish later today and provide a technical summary.

yched’s picture

Should be ready for prime time now. Works with the current state of http://github.com/yched/field_group.

Patch summary coming up ASAP.

Status: Needs review » Needs work

The last submitted patch, field_ui_extensible_616240-84.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
85.99 KB

Hm, a warning on the first instance created on each bundle = test bot implosion.

This one should be better.

yched’s picture

Updated patch with minor tweaks (comment typos, wording), and patch summary :

The goal of this patch is to make Field UI "Manage display" page extensible by allowing contrib modules to add their own rows to the table - main use case is to allow fieldgroup module D7, which, in addition, requires hierarchical tabledrag (fields nested under a group). A 1st patch already got committed in #39 above, and dealt with the "Manage fields" page, while the "Manage display" page was still pending important changes in #796658: UI for field formatter settings.

The patch here applies the same principle : move from a loosely-structured FAPI array themed into a table through a tpl.php (not extensible, the core template cannot know about contrib rows), to a dedicated FAPI type whose structure maps the table rows and columns, and rendered using theme_table() (extensible / alterable through a regular form_alter()). However, dealing with the "Manage display" page had more challenges than "Manage fields", because of the more complex interactions there : Visible / Hidden regions, #ajax update of formatter settings, which have interesting combinations when you add grouping in the mix.

The patch :
- Formalizes the existence of different kinds of rows in the "Manage fields" and "Manage display" tables ($table['row']['#row_type']), having different behavior on server and client side.
- Formalizes the use of 'regions' ($table['#regions']): 'visible' / 'hidden' on core "Manage display" screens. Side bonus : the list of regions is extensible from contrib, which drastically simplifies the job of Display suite / Node display - no need to reinvent a separate UI)
- In overviews involving groups and nested rows, the 'format type' selects can trigger a series of changes in child rows (i.e group moved to 'Hidden' --> all child rows are hidden too and need to be updated). The #ajax behavior is therefore not attached directly to the selects anymore, but triggered by the client-side field_ui.js through a hidden #ajax 'Refresh' button. A hidden 'refresh_rows' input tracks the name of affected rows.
- As a result, renames the multistep and #ajax callbacks from to field_ui_formatter_settings_[submit|js]() to field_ui_display_overview_multistep_[submit|js](), because they're not only related to formatter settings anymore.
- The client-side field_ui.js script, handling the integration of tabledrag behavior with the 'Visible' / 'Hidden' regions (originally largely stolen from block.js handling of admin/structure/blocks), is fully rewritten and abstracted around the notion of 'row handlers' prototypes, implementing how a given row type behaves on the client side. Contrib rows simply need to provide they own handler class (two methods + a constructor) in the Drupal.fieldUIDisplayOverview namespace. See the bottom of field_ui.js, and http://github.com/yched/field_group/blob/master/field_group.js for examples.

Cleanups:
- FAPI '#type' = table from the 1st patch renamed to a more specific '#type' = field_ui_table
- Massaging of the table FAPI structure before rendering (determining row order) moved from a theme preprocessor to a #pre_render callback - non-trivial business logic, doesn't belong in the theme layer.
- Unifies the FAPI structure on both "Manage fields|display" screens. Previously form values for the table rows were in $form_state['values'] for "Manage fields", and in $form_state['values']['fields'] for "Manage display" (because of the 'Custom display settings' fieldset at the top level). Hair pulling for field_goup, that acts on both forms. Now values sit at $form_state['values']['fields'] for both forms. Hence the test fixes.
- Cleans the upper part of field_ui.js that deals with the "Manage fields" behavior (no proper once() safeguard, incorrect passing of settings)
- Bugfix in tabledrag.js: in hierarchical tabledrags, rows can be indented below non draggable rows (e.g 'region heading' rows).
Did not bite so far because the only current core tabledrag that has region rows (blocks admin page) is flat.

API additions:
- adds a 'no_striping' option on rows in theme_table() : the "Manage display" table has 'region heading' rows, that cannot be part of the 'even/odd' striping, while theme_table() currently stripes all rows.
- field_info_max_weight() / hook_field_info_max_weight() : needed to make sure that newly added fields sink at the bottom instead of at a random spot amongst existing weights. With groups in the mix, we need to collect the weights of existing groups too.

aspilicious’s picture

Well I would like to push #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework first, after that one gets in this one doesn't need a reroll or a followup anymore.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Terrific summary. All tests are happy. I see no compelling argument for postponing this as proposed in #88. I also tested the patch manually (without fieldgroup module) and Manage Display still works nicely. RTBC.

If you re-roll for some reason, it would be nice to coax the help text to appear on all view modes, not just the default view mode. not strictly related to this patch.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Spent about a half hour going through this patch. Other than the @todo & commented-out stuff that aspilicious was likely referring to, I couldn't find anything else that stood out to me. This is an absolutely horrible patch to review though, as evidenced by the summary above. :\ I'm basically trusting you that this stuff is necessary and that this patch will be the end of it in terms of further kitten killing. I know your schedule is tight, which precludes smaller, easier-to-grok patches, and I really do appreciate you taking the time to get this done.

Committed to HEAD. Here's what I found (and fixed) going through.

+++ modules/field/field.api.php
@@ -2066,6 +2066,30 @@ function hook_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
+function hook_field_info_max_weight($entity_type, $bundle, $context) {

This hook returns stuff, but the return value isn't documented. Fixed before commit.

+++ modules/field_ui/field_ui.css
@@ -1,51 +1,60 @@
+	display: none;

Tab. Fixed before commit.

Powered by Dreditor.

swentel’s picture

@yched awesome! @webchick Awesome! @drupal and @allothers who also reviewed, tested and helped with this, again, awesome!
It's probably rare to see such an enthousiastic comment on a committed patch, maybe we should create like a 'I super like this button' on d.o :) Anyway, this will speed up the port for the D7 fieldgroup port - I personally *really* hope to get some progress on that the next few days/week(s), and looking at my personal schedule, it should be possible.

I'm planning to organise a code sprint for fieldgroup (and an initial port of Display Suite/Node Displays for those interested) end of september/october taking full advantage of this patch, which might (probably/most likely) result in a few smallish follow up patches, making this effort one of the biggest sucesses in Drupal Core in years - I'm exaggerating now, sorry :) The sprint will be in Belgium, I'll post and tweet about it in a few days, we'll see where we get with this!

Anyway, glad this is in!

quicksketch’s picture

swentel++

ditto.

rfay’s picture

Congratulations, all!

I just read #87 and it looks like there's no API backward compatibility breakage. True? So I won't announce it unless there is.

yched’s picture

@rfay : right, no API break here. Code that was relying on the structure of the $form array for the "Manage fields" / "Manage display" forms will break - but that should be fairly exceptional in contrib. See the paragraph starting with "Unifies the FAPI structure on both "Manage fields|display" screens" in #87.

@swentel : yay, thks for making the push on fieldgroup ! I'm on the road for the next couple weeks and won't be able to move this forward. + i really can't promise I'd do a proper maintainer job for D7 fieldgroup, so I'd love to see other people embrace this.

Berdir’s picture

@yched: "but that should be fairly exceptional in contrib" => Privatemsg did! But only in tests: http://drupal.org/cvs?commit=420956 :)

Status: Fixed » Closed (fixed)
Issue tags: -API change

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