Updated: Comment #N
Problem/Motivation
Converting book_outline and it's associated function calls to Form and BookManager Service.
Proposed resolution
Convert functions to methods on classes.
Remaining tasks
API changes
The following public methods were added to BookManager Service:
public function getLinkDefaults($nid)
public function getParentDepthLimit(array $book_link)
public function addFormElements(array $form, array &$form_state, NodeInterface $node, AccountInterface $account)
public function checkNodeIsRemovable(NodeInterface $node)
public function updateOutline(NodeInterface $node)
public function createMenuName($id)
public function updateID(array $book_link)
public function getTableOfContents($bid, $depth_limit, array $exclude = array())
These can be accessed using the following:
Drupal::service('book.manager')->methodName($args);
The following protected methods were added to BookManager Service:
protected function addParentSelectFormElements(array $book_link)
protected function recurseTableOfContents(array $tree, $indent, array &$toc, array $exclude, $depth_limit)
These are no longer directly accessible and only usable in BookManager. They are both helper functions for public methods above.
Related Issues
Original report by @Crell
Convert this page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686
Comment | File | Size | Author |
---|---|---|---|
#108 | interdiff.txt | 438 bytes | disasm |
#108 | drupal8.book-module.1938316-108.patch | 42.77 KB | disasm |
#101 | drupal8.book-module.1938316-101.patch | 42.78 KB | jibran |
#101 | interdiff.txt | 946 bytes | jibran |
#98 | drupal8.book-module.1938316-98.patch | 42.78 KB | disasm |
Comments
Comment #1
DamienMcKennaComment #2
jaskho CreditAttribution: jaskho commentedComment #3
jaskho CreditAttribution: jaskho commentedPosting code in progress.
Have routing working, but not menu.
Remaining tasks that I'm aware of:
* Get hook_menu working
* Page title (currently displays "Error")
* Port (I think) book_remove_button_submit() to BookOutlineForm method
* Port docblocks for BookOutlineForm::buildForm and ::submitForm
* Remove unused code from book.pages.inc
Comment #4
Crell CreditAttribution: Crell commentedUh oh, patch is empty. :-( Did you forget to commit to your branch before diffing?
Comment #5
jaskho CreditAttribution: jaskho commentedTrying again with patch of code in progress, a few clean-up pieces added.
* hook_menu is working, but had to re-instate access callback key, which I suspect is not correct.
* still no title; not sure how to handle this. Original code used custom callback to drupal_set_title() then hand off to drupal_get_form().
* still not sure what to do with book_remove_button_submit() - leave in module inc file or refactor to BookOutlineForm::removeButtonSubmit() (e.g.)?
Comment #6
disasm CreditAttribution: disasm commentedattached is a reroll that fixes two space issues and removes ?> at end of file.
jaskho: to apply:
git checkout 8.x
git pull
git branch -D
git checkout -b
git apply -v
git add core
git commit -m 'reroll'
Comment #7
jaskho CreditAttribution: jaskho commentedUpdate... I've been dealing with an untimely system breakdown (vs the timely kind??). Still very much "on" this issue, fwiw.
Comment #8
jibranComment #9
Crell CreditAttribution: Crell commentedI don't understand this @todo. But at the very least it should be after the shortdesc and longdesc.
Null implementation. If the class is abstract it cannot be used. :-)
The todos above need to be resolved, and I don't think we can commit this until we sort out the title and tab-access issues. Blargh.
Comment #10
jaskho CreditAttribution: jaskho commentedPosting work in progress (review away, but it might be more efficient to wait since there's a known blocker - see below)
A commit-blocking issue remains unresolved:
hook_menu/access as noted in #5. Any pointers appreciated.
Also just as a note, re moving the "remove" submit handler (book_remove_button_submit) to the form Controller class, it appears FAPI is not ready to handle methods as callbacks (see #1454730: Allow OO methods as FAPI / render API #callbacks), so this will have to wait for another day.
Comment #11
tim.plunkettThis should be 'route_name'. I think that's the main cause of your problem
Comment #12
jaskho CreditAttribution: jaskho commentedFixes 'route' -> 'route_name'.
Still pending upstream issues with registration of menu_links (see http://drupal.org/node/1945418).
Apropos of that, the code in this patch has been successfully tested against patch in http://drupal.org/node/1945418#comment-7211562.
Comment #13
jaskho CreditAttribution: jaskho commentedI'm about to disappear into vacation land till April 1, with limited web access. If this is still pending when I return I'll be happy to pick it back up, but between now and then I won't be able to work on it much.
Comment #14
jaskho CreditAttribution: jaskho commentedThis is the patch from #12, renamed so it will test now that the menu/router integration is fixed.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedThis needs the full namespace
eg @see \Drupal\book\Form\BookOutlineForm::buildForm()
No need to do this anymore:)
use inheritdocs:
#1906474: [policy adopted] Stop documenting methods with "Overrides …"
I think we can just remove this?
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedComment #17
jaskho CreditAttribution: jaskho commentedIncludes the recommended changes EXCEPT "remove validateForm()" (per comment #9)
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedah i see, it directly implements FormInterface..it should be
public function validateForm(array &$form, array &$form_state) {}
no need for the dots comment:P
Comment #19
jaskho CreditAttribution: jaskho commentedWith null comment removed per #18
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedthis is not the same
any reason why dont we move this inside the class?
those are not needed and should be removed
Comment #21
jaskho CreditAttribution: jaskho commentedThanks rootatwc, good catches. +1 for rootatwc for code reviewer chops:)
Re. moving book_remove_button_submit() into the class, see #10. My analysis may not be correct, but that's what I've been able to come to.
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedah i see, sorry did not go through all comments..yes seems FAPI does not accept methods as callbacks:/ meh
Comment #23
disasm CreditAttribution: disasm commentedjaskho, great job on this patch. It's very clean. However; I think we can move some more functions into this class and/or the BookManager service (if the functions are used across multiple classes).
If this function is specific to this callback put it in this class. Otherwise, add it to BookManager service.
If this function is specific to this callback put it in this class. Otherwise, add it to BookManager service.
If this function is specific to this callback put it in this class. Otherwise, add it to BookManager service.
If this function is specific to this callback put it in this class. Otherwise, add it to BookManager service.
Comment #24
jaskho CreditAttribution: jaskho commentedWill do
Comment #25
kim.pepperjasko, are you still working on this?
Comment #26
jaskho CreditAttribution: jaskho commentedYes, I should have a patch today. Thanks for asking.
Comment #27
jaskho CreditAttribution: jaskho commentedStill working. I really think I'll have it very soon. I have a good block of time tomorrow, so fingers crossed. I'd like the chance to see this through to the end, but I don't want to be holding anything up. If it would be best for me to let it go, I will.
Comment #28
jaskho CreditAttribution: jaskho commentedworking... getting close...
Comment #29
jaskho CreditAttribution: jaskho commentedRefactored the following functions from book_manager.module to methods on the BookManager service (per #18):
_book_link_defaults()
>>BookManager::bookLinkDefaults()
_book_parent_depth_limit()
>>BookManager::bookParentDepthLimit()
_book_add_form_elements()
>>BookManager::bookAddFormElements()
_book_node_is_removable()
>>BookManager::bookNodeIsRemovable()
Updated code in
book_form_book_outline_alter()
to accommodate API changes.EDIT:
I'm not confident the below is the best/right way to grab the node object from the
$form_state
array:$node = reset($form_state['build_info']['args']);
.As a matter of fact, I'm not confident about setting the title in
x_form_alter()
. I was originally following another form controller's example, but I don't see any great reason not to set the title inBookOutlineForm::buildForm()
.Comment #31
jaskho CreditAttribution: jaskho commentedThe failed test was Drupal\user\Tests\UserPasswordResetTest. I have to assume this is an extraneous issue.
Comment #32
jaskho CreditAttribution: jaskho commented#29: book_outline1938316-29.patch queued for re-testing.
Comment #34
stella CreditAttribution: stella commentedPasses that test for me locally, resubmitting
Comment #35
stella CreditAttribution: stella commented#29: book_outline1938316-29.patch queued for re-testing.
Comment #36
jibranI think no need to add book in methods name we know these are in book manger so all the methods are related to book.
Comment #37
stella CreditAttribution: stella commentedPatch reroll to change those 4 function names to be without the "book" prefix.
Comment #38
disasm CreditAttribution: disasm commentedNice work. Here's a couple more comments. This is real close though. Thanks for the hard work!
Unless I'm missing something, I think this should be an access checker rather than function.
Is there a reason this can't be a method on book manager service? If there is, then lets move the meat of this to the book manager service, and just return the output of that function.
Since a book is always a node, why not just isRemovable() for the function name?
Comment #39
scor CreditAttribution: scor commented#37 no longer applied, rerolled.
Comment #40
ygerasimov CreditAttribution: ygerasimov commentedChanging status to run tests.
Comment #41
Crell CreditAttribution: Crell commentedentity_get_controller() should be replaced with an injected service.
The Translation service needs to be injected.
Otherwise I think this is pretty close.
Comment #42
tim.plunkettHere is a major overhaul.
Comment #43
disasm CreditAttribution: disasm commentedThis is awesome! Major improvement getting rid of user_access everywhere.
Not sure how this book_get_books stayed in there as long as it did. Much improved.
Is this _book_update_outline getting converted in another issue? If so, an @todo would be in place here.
Finally! I found it odd that we had submitForm as the method name on a form class.
Comment #44
tim.plunkettsubmitForm is actually correct, that's just working around #2022875: Resolve difference between submitForm(), submit(), and save() in EntityFormController
I know of no issue for _book_update_outline().
Comment #45
disasm CreditAttribution: disasm commentedNo reason not to move that method in the book manager service here then. Here's a patch. Also moves book_create_menu and book_update_bid into the service.
Comment #46
disasm CreditAttribution: disasm commentedFoobar... I looked at that interdiff 10 times before uploading, but saw it right away in dreditor. changing book->manager to book_manager.
Comment #48
tim.plunkettCan we make this $this->connection please?
Needs to be $this->database (or $this->connection, ideally)
Comment #49
disasm CreditAttribution: disasm commentedthanks for the speedy review tim.plunkett. Attached is a patch addressing comments in #48.
Comment #50
jibranI think we need two new
AccessController
.We should create
AccessController
and deprecate_book_outline_remove_access
.We should create
AccessController
and deprecate_book_outline_access
.Comment #51
disasm CreditAttribution: disasm commentedbook_admin_edit is using that currently, and it's already converted in that patch: #1945390: Convert book_admin_edit to a new-style FormInterface implementation. Switching back to needs review because that's out of the scope of this issue.
Comment #52
tim.plunkettBoth are out of scope:
_book_outline_remove_access() will be handled in #1938318: Convert book_remove_form to a new-style Form object.
_book_outline_access() needs to be handled in #1945390: Convert book_admin_edit to a new-style FormInterface implementation, after this issue is in.
Comment #53
tim.plunkettStatus crosspost.
Comment #54
disasm CreditAttribution: disasm commentedComment #55
disasm CreditAttribution: disasm commentedComment #56
jibranOk I take back #50 :). I really want to combine all three in one issue but anyways I have nothing to complain here.
$form is passed by reference in
_book_add_form_elements
why not here?_book_parent_select
should also be the part ofbookManger
because only _book_add_form_elements and nowbookManger::addFormElements
calls it.Can we add
@todo
here to remove it when #2018411: Figure out a nice DX when working with injected translation is in.Typehint missing.
NodeInterface and $entity why not $node?
We can remove
$form=
now we are passing$form
by reference toaddFormElement
see 1.Comment #57
disasm CreditAttribution: disasm commentedattached are the changes from #56. jibran also wanted some more details in the book_link @param, but I'm not familiar enough with what it does to add anything beneficial.
Comment #59
tim.plunkett1, 6: I purposefully changed addFormElements() to not be by reference, it seemed messy. It should be consistent though.
2: Sure
3: Unless we have a ManagerBase or require 5.4, that code is likely not moving. A @todo is fine.
4: Sure
5: The variable has to be $entity. The typehint in the parent is for EntityInterface, when we override this here it fixes the typehints for the file.
Comment #60
disasm CreditAttribution: disasm commentedreverting the parts #59 pointed out were bad.
Comment #61
jibran@disasm You missed #56.2. Rest is fine. Thanks for working on this like a reboot :).
@tim.plunkett Thank you for explaining everything. I didn't get #59.5 but if you think it is not important then it is fine by me.
Comment #62
disasm CreditAttribution: disasm commentedHere's an interdiff to #49 (you'll see book_parent_select has been removed in the patch that was tested). Not sure exactly what that last diff was to, but it wasn't very helpful. It might help someone catch what I broke in 60.
Comment #63
tim.plunkettThese should not be camel case.
$this->configFactory = $config_factory;
@return array
$this->t()
Comment #64
disasm CreditAttribution: disasm commentedI think I found the problem. had a ConfigFactory where I wanted configFactory.
Comment #66
disasm CreditAttribution: disasm commentedAddressing #63. Also moving book_toc and book_toc_recurse to the service. That should be the end of the rabbit hole!
Comment #67
disasm CreditAttribution: disasm commentedTry again, had a missing $this->.
Comment #69
Letharion CreditAttribution: Letharion commentedJust trying to verify if this patch really breaks an install related test.
Comment #70
Letharion CreditAttribution: Letharion commented#67: drupal8.book-module.1938316-67.patch queued for re-testing.
Comment #72
disasm CreditAttribution: disasm commentedA quick search for this exception, and it looks like it's related to PHP/APC version on the testbot. I'm still unclear why the issue showed up since #49. I moved a good bit of code in book.module to the book service, but none of the functionality was changed. Here's a link to the PHP bug I found:
https://bugs.php.net/bug.php?id=61956
Comment #73
disasm CreditAttribution: disasm commentedreroll!
Comment #75
disasm CreditAttribution: disasm commentedFor future reference, That xml error tends to be thrown in tests when a class can't be found. Would be nice if it gave a better error message than that. The issue was with the configFactory stuff added earlier. Changed it to be more like other patches I've seen where the service stores just the config it needs, in this case 'menu.settings'. I called the variable $menuConfig.
Comment #76
tim.plunkettThis interdiff is a new pattern that I've personally pushed back against. Most of core until this point has kept configFactory around...
Comment #77
disasm CreditAttribution: disasm commentedswitching back to configFactory.
Comment #78
jibranit should be $configFactory.
Comment #79
disasm CreditAttribution: disasm commentedattached patch addresses #78.
Comment #80
disasm CreditAttribution: disasm commentedreroll!
Comment #81
jibranGreat work @disasm here is some minor issues.
typehint missing.
This is a function is already converted to BookManger. I don't know we need this or not.
scope and typehint.
Comment #82
jibranI think we should add all the api changes to issue summary so that core committer can approve api changes. And please add api changes tag after updating the summary. see #1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service for reference.
Comment #83
disasm CreditAttribution: disasm commentedAddressing #1, #2 and #3 in #82. Also, renamed methods to to be more descriptive.
Comment #84
jibranGreat work @disasm. I have no more feedback. Let's wait @tim.plunkett for RTBC.
Comment #86
disasm CreditAttribution: disasm commenteddoh! I swear that looked like an array! Removing type hint on that specific function for array.
Comment #87
pwolanin CreditAttribution: pwolanin commenteddoxygen for protected function t() on the BookManager should be the shorter form like shown at: #2079095: Shorten the doxygen for method t() in ControllerBase to the standard version
Comment #88
disasm CreditAttribution: disasm commentedattached fixes doxygen for t(). I left the todo to remove in though.
Comment #89
tim.plunkettThis issue is resolved. We try to put this t() method on as many base classes as possible, but for top level classes, this is what we're going to have to do until we require PHP 5.4
So let's just remove the @todo.
Comment #90
disasm CreditAttribution: disasm commentedremoving @todo
Comment #92
tim.plunkett#90: drupal8.book-module.1938316-90.patch queued for re-testing.
Comment #93
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #94
pwolanin CreditAttribution: pwolanin commentedIt would be nice to be able to convert 'node/%node/outline' to a local task plugin, but that's block by node/%node not yet being converted to a route. Same for the patch used for the form redirect - it would be nice to use the UrlGenerator, but that needs to be converted to a route.
I find the concept of a book manager a little funny, but that's already in core, so not an issue for this conversion.
I don't see any other problems except things blocked on other route conversions, so I think this is ready.
Comment #95
alexpottPatch no longer applies.
Comment #96
disasm CreditAttribution: disasm commentedstraight reroll. if it's green back to rtbc.
Comment #98
disasm CreditAttribution: disasm commentedlost a - in merge.
Comment #99
jibranBack to RTBC.
Comment #100
alexpott#2077513: Refactor ControllerBase to be more consistent with FormBase has changed getCurrentUser() to currentUser()
Comment #101
jibranBack to RTBC as it is minor fix.
Comment #103
jibran#101: drupal8.book-module.1938316-101.patch queued for re-testing.
Comment #105
disasm CreditAttribution: disasm commented#101: drupal8.book-module.1938316-101.patch queued for re-testing.
Comment #106
disasm CreditAttribution: disasm commentedminor fix, and the failure was RandomTest, unrelated testbot quirk.
Comment #107
webchickSorry to point this out, but since this issue was originally started, we now have a means of migrating MENU_LOCAL_TASKs as well, so we should take care of that here. https://drupal.org/node/2044515 has the details.
That looks wrong. :)
I didn't get through the rest of the patch yet, but those are enough to mark it needs work for now. Keep up the great work!
Comment #108
disasm CreditAttribution: disasm commented@webchick, thanks for the comments.
I removed the ========. As for the conversion, this is impossible. According to the change notice:
In order for a local task to be converted, both it and the path it appears on must first be converted to routes.
So pretty much every dependent task needs to be converted before any local tasks can be converted. In this case, it's node_page_view that is preventing this from using local_tasks.yml.
As such, I would highly suggest waiting until the old menu router is gone before requiring conversions to local_tasks.yml.
As such, moving back to RTBC for webchick's approval.
Comment #109
webchickOk, at long last!
Committed and pushed to 8.x. Thanks so much!! :D
Can we get a (major) follow-up to figure out why that ==== didn't fail tests?
Comment #110
tim.plunkettIt was inside a comment.
Comment #111
webchickD'oh. Right. :)
Comment #113.0
(not verified) CreditAttribution: commentedAdding issue summary.