Follow up for #1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service
Problem/Motivation
#1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service will add a primitive form controller for forum module. This issue is to flesh it out and remove forum_form_forum() and forum_form_container() to instead use a first-class entity form controller, moving the menu callback to the new routing system in the process.
Proposed resolution
Flesh out ForumFormController to replace forum_form_forum() and forum_form_container()
Remaining tasks
Write patch
User interface changes
None
API changes
forum_form_forum(), forum_form_submit() and forum_form_container() removed.
Comment | File | Size | Author |
---|---|---|---|
#33 | 1951278-forum-patch.patch | 25.72 KB | tim-e |
#33 | 1951278-forum-patch.interdiff.txt | 3.04 KB | tim-e |
#28 | forum-form-controller.1951278.28.interdiff.txt | 427 bytes | larowlan |
#28 | forum-form-controller.1951278.28.patch | 25.61 KB | larowlan |
#25 | forum-form-controller.1951278.25.patch | 25.6 KB | larowlan |
Comments
Comment #1
larowlanNew title "Create taxonomy term form controller for forum terms (ie not default) and use that for admin/structure/forum/edit/{forum|container}/%"
Comment #2
larowlanBlocked on #1946414: Convert confirm_form() in forum.admin.inc to the new form interface
Comment #3
larowlanback on this
Comment #4
tim.plunkettBlocks #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()
Comment #6
larowlanOk patch is 75% done, pretty sure this blocks moving term description to field too, so will make this my to priority. Let me know if you want me to push my wip to sandbox.
Comment #7
tim.plunkettOh there are like 20 other issues blocking that. Seriously it will be a month til they're all done.
Comment #8
larowlanComment #9
larowlan#1909418: Allow Entity Controllers to have their dependencies injected is in, can fix some todos
Comment #10
larowlanInjects config factory now #1909418: Allow Entity Controllers to have their dependencies injected is in.
Removes refs to _values in @todo's as that looks unlikely to get in.
Comment #11
kim.pepperLooks good!
Just a few minor nickpicks.
Should be _content?
_content?
Can this be called isNew? I think that would make it clearer.
Comment #12
larowlanFixes nitpicks and missing use.
Comment #13
dawehnerJust wondering whether we still need 'parent' here.
Not sure whether we have discussed before, but as the storage controller is not injected into the term object it might be better to call save on the storage controller instead.
This can also just use Cache::invalidateCache
Is it possible to use Request $request instead?
There should be a follow up to replace this by a injected service using EQ
Comment #14
larowlanRe-roll with feedback from #13
Comment #15
dawehnerDo you know whether this is actually required?
I guess we want to use hook_entity_info instead, as we add new entries, not replace some.
Let's break the new static in multiple lines to make it easier to add new entries later, if needed (we did that in many other patches already).
entityManger->getForm() exists now.
Let's have an empty line after "}"
Comment #17
larowlanIt doesn't seem to change functionality, but it does provide extra context for alter implementations, so I say lets leave it, its consistent with other modules.
Comment #18
dawehnerThe controllers should be placed in the "Controller" folder.
The form controller should be placed in the form folder.
Comment #19
larowlanMoves namespaces as per #18
Comment #20
dawehnerThank you very much!
Comment #22
larowlanStuffed the namespaces in #19
Comment #23
dawehnerI hope this is green this time. (latest thoughts)
Comment #24
alexpottNeeds a reroll
Comment #25
larowlanstraight re-roll for removal of rdf stuff.
Comment #26
dawehnerdo we typehint for arrays?
Comment #27
larowlanStraight copy from existing function, just moved files - out of scope?
Comment #28
larowlanFixes #26
Comment #29
jibranIt is ready to go.
Comment #30
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #31
larowlanComment #32
larowlanComment #33
tim-e CreditAttribution: tim-e commentedReroll
Comment #34
xjmSo this looks like a reference to #1976298: Move taxonomy_get_tree() and associated functions to Taxonomy storage, deprecate procedural wrappers., correct? Let's make sure we have converting these lines as part of the scope of that issue. (Ideally I'd prefer to see the issue linked in the @todo, but it's not a big deal.)
Comment #35
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #36
xjmDries didn't reload. ;)
Comment #37
kim.pepperCopy/paste error here :-)
Comment #39
xjmd.o ate my comment.
@kim.pepper, can you file a followup novice issue? Thanks!
Comment #40
kim.pepperFollow up issue #2050777: Incorrect class reference in ForumFormController docblock
Comment #41
vijaycs85Removing follow up tag.