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.
Comment | File | Size | Author |
---|---|---|---|
#61 | field-ui-1946404-61.patch | 105.09 KB | tim.plunkett |
#61 | interdiff-from-53.txt | 2.77 KB | tim.plunkett |
#59 | field-ui-1946404-59.patch | 105.73 KB | tim.plunkett |
#59 | interdiff.txt | 594 bytes | tim.plunkett |
#56 | interdiff.txt | 4.68 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettWorking on this.
Comment #2
tim.plunkettYeah, nevermind. The menu item is in a foreach, and I don't have the bandwidth to figure out EventSubscriberInterface to write a RouteSubscriber.
Comment #3
kim.pepperWould this be a route enhancer now? Any good examples of this already?
Comment #4
tim.plunkettOkay, this actually wasn't hard because of the EventSubscriber, but because #1735118: Convert Field API to CMI isn't in yet. This will be much more straightforward with that in.
Comment #5
tim.plunkettWorking on this, it turns out that it will be very hard to do only the ONE confirm_form(), and leave the other forms, due to the foreach in hook_menu(). So this will now encompass the 6 form callbacks at once.
Comment #6
Crell CreditAttribution: Crell commentedSee the section "Dynamic Paths" in the change notice for how to port the foreach loop: http://drupal.org/node/1800686
(That is, unless it's possible to eliminate the foreach and make it a single, more flexible definition. That's always preferable.)
Comment #7
tim.plunkettJust to see what happens.
It's possible that breadcrumbs are completely broken for dynamic routes.
Comment #9
tim.plunkettHm.
Comment #10
jthorson CreditAttribution: jthorson commentedFrom the testbot apache logs:
Comment #12
swentel CreditAttribution: swentel commentedtagging
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedRaising to major because of #1963340-14: Change field UI so that adding a field is a separate task.
Comment #14
mgifford@jthorson I'm guessing that this is because of:
I had a problem apply the code to my git repo. I think a lot has changed recently.
Comment #15
tim.plunkett@mgifford that code isn't anywhere near the menu module...
I'm working on rerolling this.
Comment #16
mgiffordThanks @tim.plunkett it's a big patch, thanks for doing the re-roll. But ya, you hadn't touched menu.module so was trying to look at what seemed like the next logical place.
Comment #17
tim.plunkettSo this is just a reroll because the last patch included the entire Field API as CMI conversion.
This needs a complete overhaul, because all of the routes are wrong and broken.
It will likely incorporate something like #1783964: Allow entity types to provide menu items or #1970360: Entities should define URI templates and standard links.
Comment #19
tim.plunkettOoookay. This was interesting.
It still needs a lot of cleanup, but I want to get it green first.
Comment #21
tim.plunkettI think EnableDisableTest will still fail, but this is a start.
Comment #22
tim.plunkettUm, bot, I wanted those tags
Comment #24
tim.plunkettMissed a spot, and figured out the EnableDisableTest
Comment #25
yched CreditAttribution: yched commentedYay & double yay !
Just began skimming through the patch, and I'm fairly new to the new routing system :
Can you expand a bit on the changes made on the menu paths building logic ?
Like :
- in hook_entity_bundle_info(), the 'path', 'bundle argument', 'access callback' entries are not needed anymore ?
- admin/structure/types/manage/article/fields/field_foo --> admin/structure/types/manage/article/fields/node.article.field_foo
Ouch :-/. Is there any way we could avoid putting config ids in URLs ?
As for the form callbacks themselves: is this just moved around, or did this require more significant changes ?
Comment #26
tim.plunkettI didn't make any changes to the form callbacks, other than storing $instance as $this->instance, instead of putting it in $form_state['instance'], $form['#instance'], or just calling field_info_instance() a lot (all of which were done).
----
For Field UI paths, we currently have something like:
admin/structure/types/manage/%node_type/fields/%field_ui_instance/edit
It also has
'load arguments' => array($entity_type, $bundle_arg, $bundle_pos, '%map')
in the hook_menu.When visiting
admin/structure/types/manage/article/field_body/edit
node_type_load()
turns 'article' into an object, andfield_ui_instance_load()
takes the object and 'field_body' and the load arguments and loads a field instance.In order to avoid adding more ParamConverters, I've switched the path to
admin/structure/types/manage/article/node.article.field_body/edit
since node.article.field_body is the ID of the FieldInstance entity
But then we have the bundle (article) repeated twice, and an uglier URL.
I'm not sure that routes can be bent to suit our original implementation, or if its worth it since ParamConverters are vaguely expensive
Comment #27
Crell CreditAttribution: Crell commentedI'm A-OK with shifting the URI around to be easier. My main concern would be keeping breadcrumbs sane within the Field UI pages, which Tim informs me he was able to do. (I've not verified.) I didn't go over the patch in detail, but on a skim through it all looks fairly sane to me.
A few quick notes while I'm at it:
entity_get_info() should be replaced with whatever its injectable modern version is.
The array merging syntax here took me a while to grok. Without PHP-aware syntax highlighting I can't really follow it. Changing the whitespace structure is probably sufficient to make it grokable.
Comment #28
tim.plunkettI should go back through the classes looking for injectable versions of functions.
That Route syntax is CRAZY. I copy/pasted from somewhere else. Glad to know it's not important to someone if we fix it.
Comment #29
yched CreditAttribution: yched commentedNot sure what to infer from the last exchanges with @Crell. Do we have a hope to get back to non-ugly URLs short-term ?
If not, I'm fine with getting this committed with its current URLs and see if this can be made better in a followup.
Thrilled at how this will make it easier to finally rename field_ui.module to entity_ui.module (since that's what it is, really).
(Still not a real review, though. Will try to do that asap)
That info is only correct if {bundle} is the node type, not the actual comment bundle name (comment bundles are 'comment_node_[node_type]'). I don't think that's the case ? So, not sure that works ?
Comment #30
tim.plunkettYeah I added this to handle that, because WOW this is bizarre.
Comment #31
tim.plunkettReroll to address Crell's feedback. Still haven't fixed the comment hack.
Comment #33
tim.plunkettFixed tests and clarified the workaround for comment module.
Comment #35
swentel CreditAttribution: swentel commented#33: field-ui-1946404-33.patch queued for re-testing.
Comment #36
swentel CreditAttribution: swentel commentedSo, here's a solution for the comment hack, I'm not sure if this is the way to go or not, but it works at least.
Comment #37
tim.plunkettNice, I like that. Here it is with DI.
Comment #39
tim.plunkettReroll for #1967106: FieldInstance::getField() method
Comment #40
tim.plunkettReroll for the various entity/annotation commits.
Comment #41
yched CreditAttribution: yched commentedOnly got minor remarks :
"Constructs a RouteSubscriber object." ?
Not for this patch either but: core seems to establish a kind of pattern that all subscriber classes to RoutingEvents::DYNAMIC are all called just RouteSubscriber ?
Is that really needed ? Maybe would warrant a code comment ?
Not for this issue, I guess, but those two entries using different placeholder syntaxes is a bit unfortunate.
Will that be tackled as a side effect of other, more general "mobe routes out of hook_menu()" tasks, or should we open a dedicated followup ?
Comment #42
yched CreditAttribution: yched commentedOh, sorry - while we're in there, if we could just reintroduce in class names and route names the sanity that was lost in the previous form_ids when we removed the "field settings" fieldset from the "edit instance" form:
Proposal:
class: FieldEditForm -> FieldInstanceEditForm
route: field_ui.edit.$entity_type -> field_ui.instance_edit.$entity_type
class: FieldSettingsForm -> FieldEditForm
route: field_ui.settings.$entity_type -> field_ui.field_edit.$entity_type
If not, no biggie, can be a followup.
Comment #43
tim.plunkettI think the FieldInstanceEditForm name change makes things clearer, but as long as the URL is /field-settings, I think FieldSettingsForm is a clearer name. Follow-up to rename both the class and the URL seems fine to me.
Crell opened #1970360: Entities should define URI templates and standard links to address the various URI properties we have floating around.
I deleted the change to MenuLinkStorageController, since I think it may have just been an intermediate hack. If this fails, then we'll know what to make the comment :)
Yeah, I think everyone who needs a RouteSubscriber keeps copying the same code :)
Comment #45
tim.plunkettWelp, that would be why I had that check.
Comment #46
amateescu CreditAttribution: amateescu commentedSeems that @yched's concerns were adressed, so I just fixed the comment from the last interdiff and call this done :)
Comment #48
alexpottthe
entity_
is redundant...Shouldn't this be using the injected plugin.manager.entity
It's a shame that we're not using an injected services from the container here... but AFAIK at the moment we can't inject things in ConfirmForms
Shouldn't this implement the ContainerInterface and have a create method so that the plugin.manager.entity and the module handler can be injected
and ContainerInterface here too... to inject modulehandler, field.info
and ContainerInterface here too... to inject field.info
Also chatting to @fubhy in IRC we should have followups in place once #1906810: Require type hints for automatic entity upcasting and #1837388: Provide a ParamConverter than can upcast any entity. so that we can reduce the redundancy of urls like
admin/structure/types/manage/article/fields/node.article.body
(which used to beadmin/structure/types/manage/article/fields/body
)Comment #49
alexpottCrosspost with testbot :)
Comment #50
tim.plunkettThanks @alexpott, working on injecting stuff now.
Comment #51
tim.plunkettOkay, injected as much as I could, there is an issue with module_handler and $form_state, so just using \Drupal::moduleHandler for now.
Comment #52
tim.plunkett@amateescu pointed out a typo, thanks
Comment #53
amateescu CreditAttribution: amateescu commentedI just found another problem..
Comment #54
yched CreditAttribution: yched commentedThen, just delete() please (param is optional and defaults to TRUE).
Then should be injected as well ? ($container->get('field.info'))
See next comment.
Then should be injected as well ? ($container->get('field.info'))
SameSee next comment.
Comment #55
yched CreditAttribution: yched commentedActually, sorry:
Those two could now more simply be :
$field = $field_instance->getField();
Comment #56
tim.plunkettOh duh, forgot that field.info was a service.
Also, in 2/3 of those cases, we can just do $this->instance->getField().
Comment #57
yched CreditAttribution: yched commentedThanks ! Looks good to me.
Comment #59
tim.plunkettGrrr.
Comment #61
tim.plunkettFieldInfo has ModuleHandler injected, so we cannot serialize that either.
This is #53 + the non-breaking fixes suggested in #54/#55
Comment #62
alexpottCommitted 6ed57d3 and pushed to 8.x.
During the commit I fixed the following comment as we no longer use an access callback...
Comment #63
yched CreditAttribution: yched commentedYay ! Thanks a ton @timplunkett !
Comment #64
yched CreditAttribution: yched commentedFollowup for #42: #1981198: More accurate names and form_ids for Field UI form classes
Comment #65
tim.plunkettSee #1982058: ModuleHandler cannot be injected into forms. Working on a change notice.
Comment #66
tim.plunkettAdded http://drupal.org/node/1982084 and opened #1982088: Remove hook_entity_bundle_info()'s need for 'real path' as a follow-up.
Comment #67
swentel CreditAttribution: swentel commentedLooks good to me.
Comment #68
yched CreditAttribution: yched commentedMarking as fixed, then ?
Comment #70
amateescu CreditAttribution: amateescu commentedThis FormInterface conversion brought up a very nasty side effect on PHP 5.4: #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0. Reviews appreciated :)
Comment #71
yched CreditAttribution: yched commentedPing: #2160159: Remove useless D7 field_ui menu loader callback