Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#42 | field-ui-1987708-42.patch | 8.62 KB | tim.plunkett |
#42 | interdiff.txt | 8.67 KB | tim.plunkett |
#40 | 1987708-route-field-ui-40.patch | 8.02 KB | swentel |
#40 | interdiff.txt | 742 bytes | swentel |
#37 | 1987708-route-field-ui-37.patch | 8.01 KB | swentel |
Comments
Comment #1
vijaycs85Comment #2
swentel CreditAttribution: swentel commentedComment #3
swentel CreditAttribution: swentel commentedComment #4
yched CreditAttribution: yched commentedNot sure what is the current status of hook_menu() deprecation: for something that is just a MENU_NORMAL_ITEM (no integration with local tasks, etc), do we still need to keep an entry in hook_menu() ?
This should inject the services needed in the fieldsList() method:
the entity manager,
the FieldInfo service,
(looks like that"s all for the moment)
:-)
Comment #5
manimejia CreditAttribution: manimejia commented#3: 1987708-3.patch queued for re-testing.
Comment #6
swentel CreditAttribution: swentel commentedUnassigning for now
Comment #8
mtiftI'll take a crack at this one
Comment #9
mtiftThe attached patch incorporates comments from #4
Comment #10
mtiftWhoops. Needs review.
Comment #11
mtiftRenamed the _construct object
Comment #12
mtiftAdded a missing param thanks to @swentel
Comment #14
swentel CreditAttribution: swentel commented#12: 1987708-12.patch queued for re-testing.
Comment #16
swentel CreditAttribution: swentel commented#12: 1987708-12.patch queued for re-testing.
Comment #18
vijaycs85syntax error fix...
Comment #19
smiletrl CreditAttribution: smiletrl commentedTo address concern for #4,
There's such a local task, so perhaps that's why
'type' => MENU_NORMAL_ITEM,
will be required.
For
I guess
could be a better option.
One thing: getInstances will have some performance issue,see Clas Fieldinfo. Second thing: Results from getFieldMap() could make more sense for this scenario. For what we really want here is fields list, not instance list.
This
if
condition means there are redundant loops, because current code loops all existing instances. Obviously, there would be redundant loops when one field has multiple instances in site. If we get only fields list in the very beginning, it could save a lot of headache, imho:)Comment #20
mtiftI think we're fine with MENU_NORMAL_ITEM. I agree with @smiletrl regarding getFieldMap() and made the change.
Comment #21
swentel CreditAttribution: swentel commentedSweet
Comment #22
mparker17Hmm. Tests might pass, but I'm getting a large series of messages when I go to
admin/reports/fields
:Comment #23
swentel CreditAttribution: swentel commentedRight, sorry about that.
$this->fieldInfo->getInstances(); is fine - this also just an admin overview, we don't really care about performance here.
Comment #24
swentel CreditAttribution: swentel commentedAdditionally, we do need al the instances, as we're printing the bundles where they are used.
Comment #25
mparker17Okay, I'm going to:
and add a test so we can detect if this page gets broken at some point in the future.
Comment #26
mparker17Comment #27
mparker17Okay, let's try this.
Comment #28
swentel CreditAttribution: swentel commentedGreat!
Comment #29
smiletrl CreditAttribution: smiletrl commentedNew patch for #19. This isn't a major/performance issue, but more of "which is the best practice" thing.
Personally speaking, I think
$fields = $this->fieldInfo->getFieldMap();
makes code cleaner/clearer and pretty straightforward~
Comment #30
smiletrl CreditAttribution: smiletrl commentedOops, extral spaces...
new patch fix it.
Comment #31
smiletrl CreditAttribution: smiletrl commentedAlso, I think
should be put before
__construct
, since methodcreate
will be executed firstly before_contruct
?Comment #32
swentel CreditAttribution: swentel commentedThat's fine. There have been a couple of other conversion which went in where the construct method is at the top. __construct methods are also usually at the top of the class anyway.
Tested manually and works fine and we have a test, sweet!
Comment #33
alexpott#30: 1987708-route-field-ui-30.patch queued for re-testing.
Comment #34
alexpottNeeds a reroll
Comment #35
swentel CreditAttribution: swentel commentedSimple reroll after #1982138: Clean out field_ui.admin.inc got in.
Comment #36
alexpottNeeds a reroll
Comment #37
swentel CreditAttribution: swentel commentedreroll after #1798654: Clean faulty HTML in help description of field widgets got in
Comment #38
swentel CreditAttribution: swentel commentedRemoving tg
Comment #40
swentel CreditAttribution: swentel commentedOh ControllerInterface ...
Comment #41
alexpottI wonder if we can make this use EntityListController?
Noticed some minor nits...
This can use
$this->fieldInfo->getField()
I think this could just return an array like this..
Comment #42
tim.plunkettYes, let's use the ListController.
Comment #43
smiletrl CreditAttribution: smiletrl commentedGood work!
It's nice to replace the 0, 1, 2 keys with meaningful keys 'type', etc.
Comment #44
swentel CreditAttribution: swentel commentedSweet
Comment #45
alexpottCommitted 8b1ce34 and pushed to 8.x. Thanks!