Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
field_ui_next_destination() is only used by the "Manage Fields" forms, and they are similar enough to share a base class (added here).
field_ui_get_destinations() is used three times (once for no reason), but is not a Field UI-specific function, and should just be called inline as needed.
Comment | File | Size | Author |
---|---|---|---|
#32 | field-ui-1982138-31.patch | 51.65 KB | yched |
#32 | interdiff.txt | 1.45 KB | yched |
#30 | field-ui-1982138-30.patch | 51.42 KB | tim.plunkett |
#28 | field-ui-1982138-28.patch | 51.39 KB | amateescu |
#28 | interdiff.txt | 567 bytes | amateescu |
Comments
Comment #1
tim.plunkettThis builds on #1982088: Remove hook_entity_bundle_info()'s need for 'real path'.
Comment #2
andypostsuppose better to postpone this on #1982088: Remove hook_entity_bundle_info()'s need for 'real path'
+1 to RTBC
Comment #3
swentel CreditAttribution: swentel commented#1982088: Remove hook_entity_bundle_info()'s need for 'real path' got in, here's a reroll
Comment #4
tim.plunkettIn light of http://buytaert.net/reducing-risk-in-the-drupal-8-release-schedule, let's stop tiptoeing around and just clean things up for real.
Each of the following are called only once, in either FieldOverview/DisplayOverview:
field_ui_field_type_options()
field_ui_formatter_options()
field_ui_existing_field_options()
field_ui_widget_type_options()
Any thoughts on converting to methods?
I'm going to open an issue about allowing any callable for machine_name 'exists', which kills _field_ui_field_name_exists()
field_ui_fields_list() is #1987708: Convert field_ui_fields_list() to a new style controller
_field_ui_reduce_order() is weird, whatever.
Then we are left with field_ui_table_pre_render() and theme_field_ui_table(). The latter is Twig fodder.
The reroll in #3 was only half of the original, and the rest conflicted, so no real interdiff.
Comment #6
tim.plunkettWhoops, #1981198: More accurate names and form_ids for Field UI form classes wreaked havoc on my reroll.
Comment #7
swentel CreditAttribution: swentel commentedClosed #1854868: Let formatter summaries return arrays instead of strings and #1854880: Cleanup DisplayOverview::form from our sandbox, maybe we can tackle that too at once.
The ultimate follow should be 'simply' renaming field ui to entity ui - although that might depend on #1875974: Abstract 'component type' specific code out of EntityDisplay (although it's not a requirement)
I will do those seperately
Comment #8
swentel CreditAttribution: swentel commentedAddendum, I'll work on top of this one, depending on the size, I might split it anyway :)
Comment #9
swentel CreditAttribution: swentel commentedThe ultimate follow should be 'simply' renaming field ui to entity ui - although that might depend on #1875974: Abstract 'component type' specific code out of EntityDisplay (although it's not a requirement).
Comment #10
tim.plunkettUseful and generic functions:
field_ui_formatter_options() => FormatterPluginManager::getFormatterOptions()
field_ui_widget_type_options() => WidgetPluginManager::getWidgetOptions()
Specific functions:
field_ui_field_type_options() moved inline
field_ui_existing_field_options() => FieldOverview::getExistingFieldOptions()
Total removed code from field_ui.admin.inc: 211 lines, 9 functions
Remaining code in field_ui.admin.inc: 251 lines, 5 functions
Comment #12
tim.plunkettGrr, FieldOverview uses $this->entity_type not $this->entityType.
Comment #14
tim.plunkettSerious deja vu here.
Comment #15
tim.plunkettOpened #1992052: Allow machine_name 'exists' to accept a callable
Comment #16
yched CreditAttribution: yched commentedOverall this looks simply lovely !
A couple remarks :
Seems a little weird to have the $widgetManager here - FieldEditForm, which extends this base class, doesn't need one.
This being said, FieldEditForm extends FieldInstanceFormBase is weird too :-).
TBH, aside from the getNextDestination() method, I'm not sure I see the real gain of the base FieldInstanceFormBase class and the other methods in there.
For example, I'd rather have the "
$this->instance = $form_state['instance'] = $field_instance;
" code explicitly repeated in each child buildForm() method, than mutualized / hidden away over here.Then, if it's only to have a place for getNextDestination(), maybe it would make more sense as a method on a static FieldUI helper API class like we tend to introduce in core modules ?
Copy/pasted, but two typos here:
"be added" / "user interface" (would "via the UI" be acceptable ?)
Typo in the comment here as well.
Also, this code should move a couple lines up, after the call to getWidgetOptions() (the code is "start X, do unrelated Y, finish X")
Minor: this could reuse the $widget_options variable defined above.
"in OverviewBase::form()" is not really true, OverviewBase doesn't refer to the getRowRegion() method (+ the actual method name is buildForm()).
DisplayOverview and FieldOverview just happen to both use such a method, but I'm not sure it deserves to have an abstract definition here ? You don't *need* to implement it to be allowed to extend OverviewBase ?
Hm, this function doesn't seem to be called anywhere in HEAD now, but it was called in both "field overview" and "display overview" before #1946404: Convert forms in field_ui.admin.inc to the new form interface - see http://drupalcode.org/project/drupal.git/commitdiff/6ed57d3.
I'm not sure that removing those calls was intentional ?
[edit: also, if we *do* remove that function, then field_ui_inactive_instances($entity_type, $bundle) can be deleted as well]
Other than that:
_field_ui_reduce_order() could be an inline closure within field_ui_table_pre_render(), I guess. No biggie either way.
field_ui_table_pre_render() itself could potentially be a helper method in OverviewBase, but drupal_render() currently doesn't allow methods as #pre_render callbacks.
Comment #17
yched CreditAttribution: yched commentedAlso, looks like this is going to conflict with #1875992: Add EntityFormDisplay objects for entity forms
Comment #18
tim.plunkettPostponing on #1993312: Change pre_render, post_render, and after_build callbacks to accept callables and #1875992: Add EntityFormDisplay objects for entity forms
Comment #19
tim.plunkettComment #20
tim.plunkettOkay, so the FieldEditForm / getNextDestination bit is a good point. I have not yet done anything about that.
This still includes the RTBC callable patch.
Comment #21
andypostSeems better to include #1998992: Typo in FieldOverview.php
Comment #22
jibranAs per #18
Comment #24
tim.plunkettStraight reroll.
Comment #25
tim.plunkettOkay, here it is with the static helper.
Comment #26
amateescu CreditAttribution: amateescu commentedI have only one small problem with this patch: I think the
getFormatterOptions()/getWidgetOptions()
methods should both be renamed to justgetOptions()
so they can be used generically by a (base) class that works with either formatter OR widget plugins (yes, I'm leveling the ground for the form modes patch :P).Implemented this change in the patch attached, along with some other minor cleanups.
RTBC anyone? :)
Comment #28
amateescu CreditAttribution: amateescu commentedPfff.
Comment #29
swentel CreditAttribution: swentel commentedLooks good to me, I'll let yched do the final call.
Also, disabled modules is going to be removed anyway now, so we don't need that inactive message :D
Comment #30
tim.plunkettStraight reroll.
Comment #31
yched CreditAttribution: yched commentedTwo minor fixes:
- removes an unused variable
- adds a missing \ before Exception
Other than that, well, the fact that we add object methods as FAPI #callbacks means we're now serializing DisplayOverview / FieldOverview objects in $form, and we hit the serialization chain of hell mentioned in #2004282-2: Injected dependencies and serialization hell (serializing all of those objects injected dependencies as well - here, the widget / formatter managers and their list of cached definitions), with the subtle associated bugs down the road.
Strictly speaking, those methods are just a copy/paste of former functional helpers. They are just moved to methods for better code organization, but they don't rely on the state of the object - of course, since their former functional version didn't - that's how FAPI #callbacks worked in D7, all external state info needs to be placed in $element.
So these helpers could be made static methods, and added as '#callback' => array(array(get_class($this), 'methodName')). (OptionsWidgetBase does this to avoid unneeded serialization)
Not sure what to do with this. We have already started doing this in other places, so it should probably not be held against this specific patch...
Comment #32
yched CreditAttribution: yched commentedForgot to upload the minor changes mentioned in #31
Comment #33
swentel CreditAttribution: swentel commentedGood to go.
Comment #34
alexpottNice refactor!
Committed 7f99ae9 and pushed to 8.x. Thanks!
Comment #35
swentel CreditAttribution: swentel commentedAs far as I can see, this doesn't really need a change notice. The only useful ones /might/ be FormatterPluginManager::getOptions and WidgetPluginManager::getOptions - but I don't really think people use those, unless I'm completely wrong ?
Comment #36
andypostSuppose better to update existing change notice with latest changes
Comment #37
YesCT CreditAttribution: YesCT commented@andypost OK, which change notice should we update?
I looked at the list of change records that have something about plugins: https://drupal.org/list-changes/drupal?keywords_description=Plugin&to_br...
but none really jumped out at me.
Comment #38
swentel CreditAttribution: swentel commentedChange notice added at https://drupal.org/node/2017627
Comment #39
amateescu CreditAttribution: amateescu commentedYeah, I think that's all we need for this change.
Comment #41
yched CreditAttribution: yched commentedFollowup : #2050367: FieldInstanceFormBase is useless