Problem/Motivation
Book-related queries currently appear in book.module . We should move all storage-dependent code to one class to make storage swap-ability easier.
Proposed resolution
Move all queries from book.module
to \Drupal\book\BookOutlineStorage
and make the latter storage controller for all book-related operations.
Once this is done, split operations related to the books as collection (list all books...) from the ones related to nodes within a book (book_node_load, ...) and introduce a separate storage controller for the latter (single responsibility). But this can be done after the initial hook to manager refactoring.
Remaining tasks
- move all storage-dependent node hooks to the storage controller
- validate with an alternate storage controller (see #2224725: Add a mongodb.book.manager service
- if this seems worthwile, refactor the "manager" to two classes, one for the book as collection of nodes, and one for the nodes themselves.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#45 | interdiff.txt | 8.47 KB | slashrsm |
#45 | 2225283_45.patch | 24.21 KB | slashrsm |
#39 | interdiff.txt | 4.5 KB | slashrsm |
#39 | 2225283_39.patch | 24.02 KB | slashrsm |
#36 | make_book_storage-2225283-36.patch | 23.88 KB | martin107 |
Comments
Comment #1
dawehner#2209025: Phase 3 - Make the BookManager interface more coherent, move more code out of book.module is probably fixing most of what you need. We will improve that class and especially over time, like moving out form logic and other stuff.
This issue sounds more like a meta issue.
Comment #2
dclavain CreditAttribution: dclavain commentedI'm going to work on this
Comment #3
plachComment #4
dclavain CreditAttribution: dclavain commentedComment #5
Primsi CreditAttribution: Primsi commentedI will work on that.
Comment #6
Primsi CreditAttribution: Primsi commentedInitial patch. slashsm already did an inital review at drupalcon and gave suggestions.
Comment #8
Primsi CreditAttribution: Primsi commentedLet's see now.
Comment #9
Primsi CreditAttribution: Primsi commentedComment #10
slashrsm CreditAttribution: slashrsm commentedLooks good. Few minor comments:
Provide entire namespace in @var: \Drupal\book\BookStorage
Defines storage class.
Book storage classes.
Gets books. + what is the difference with the next one? Maybe "Gets all books."
Loads books.
Gets....
Deletes....
(And so on for all comments in this interface)
Hm... what about something like this:
"Updates book reference for links that were moved between books."
@var with namespace.
Comment #11
Primsi CreditAttribution: Primsi commentedThx for the review.
This one only loads "books", that would be the top most book links.
Comment #12
slashrsm CreditAttribution: slashrsm commented@var without :.
Same as above.
Comment #13
Primsi CreditAttribution: Primsi commentedRemoved punctuations.
Comment #14
slashrsm CreditAttribution: slashrsm commentedTwo other minor comments-related things:
Return value should not be DB dependent.
Please explain a bit more than just "see".
Comment #15
Primsi CreditAttribution: Primsi commentedHere we are returning the return from the delete query. So I just copied what the comment for \Drupal\Core\Database\Query\Delete::execute says.
Copied stuff from bookTreeBuild. Or did you mean to just elaborate more on why "see" ?
Comment #16
slashrsm CreditAttribution: slashrsm commentedI'd go with something like: "An associative array of build parameters. For info about individual parameters see BookManager::bookTreeBuild()."
I'd go with something like: "Number of deleted book entires."
Comment #17
Primsi CreditAttribution: Primsi commentedThx for the suggestions.
Comment #18
slashrsm CreditAttribution: slashrsm commentedComment #19
alexpottSome minor nits. I know that the book module doesn;t have the test coverage of some of the other modules - can we check that we've got enough coverage or what we're changing here.
Mis-spelt
Book storage
Mis-spelt - being
ing? I'm guessing integer
Comment #20
Primsi CreditAttribution: Primsi commentedFixed review stuff.
As for the test coverage goes, I've went through the tests and as far as I can understand most of it should be there. Except the test already mentioned in #2282959: Fatal error when accessing book hierarchy at /book. But as this is my first core issue it would be perhaps good to give this a second lool.
Comment #22
Primsi CreditAttribution: Primsi commentedComment #23
slashrsm CreditAttribution: slashrsm commentedComment #24
daffie CreditAttribution: daffie commentedThe patch contains an update for the file sites/sites.php. Maybe I am wrong but I do not think that it belongs to this patch. Below the update to the file sites/sites.php.
Comment #25
daffie CreditAttribution: daffie commentedComment #26
Primsi CreditAttribution: Primsi commentedUps, sorry for that. Here is the same patch without the accidentally added sites.php.
Comment #27
slashrsm CreditAttribution: slashrsm commentedFound one tiny strageness in comments. Can confirm sites.php are gone though :)
Isn't this always int?
Same as above?
as above?
again
Same here.
and here
Comment #28
martin107 CreditAttribution: martin107 commentedI agree with #27
Comment #29
slashrsm CreditAttribution: slashrsm commentedLooks good. Thanks!
Comment #30
catchLooks like the @database argument can be removed no?
Comment #31
martin107 CreditAttribution: martin107 commentedRegarding #30 Yep, it is obvious when you see it... snip snip ... stray dependency removed.
BookManagerTest passes locally.
Comment #32
martin107 CreditAttribution: martin107 commentedJust thinking about the last change.BookManaager::__construct TranslationInterface $translation can also be removed as a dependency.But it is dangerous to cross the streams... so I am opening up another issue for that. I will list it as a child of this one.It uses StringTranslationTrait, so it is a wanted dependancy!
Comment #33
dawehnerFeedback got adressed
Comment #34
alexpottI think
BookStorage
is incorrectly named - we're not storing books here we are storing book outline information. Also I think we should have a bit more documentation on what an outline is or at least point to some documentation on what it is.Here are the fixes I'd make if I was going to commit this:
Comment #35
martin107 CreditAttribution: martin107 commentedSo before fixup along the lines of #34. I am re-rolling.
Naming things is hard, just proposing a simple name and trying on a couple of existing methods :-
BookOutlineService::countOriginalLinkChildren()
BookOutlineService::getBookMenuTree()
BookOutlineService::getBookSubtree()
BookOutlineService::loadBookChildren()
For me it does not fit comfortably with all methods, I am not tied to this name, just a first iteration.
Comment #36
martin107 CreditAttribution: martin107 commentedBookOutlineService -- it is not the drupal way to append a service with "Service"
Perhaps BookOutllineManager?
Comment #37
alexpottBookOutliner
?Comment #38
martin107 CreditAttribution: martin107 commentedIf I may, I think
Drupal\book\BookStorage -> \Drupal\book\BookOutliner
will cause confusion with an existing class \Drupal\book\BookOutline. which is as follows
So my counter is :-
Drupal\book\BookStorage -> \Drupal\book\BookOutliner
Drupal\book\Outline-> \Drupal\book\BookOutlineHandler
Am I using Handler in an acceptable way, that is the question? Or would BookOutlineRender stick in anyone craw.
Comment #39
slashrsm CreditAttribution: slashrsm commentedWhat about Drupal\book\BookStorage -> Drupal\book\BookOutlineStorage and Drupal\book\BookStorageInterface -> Drupal\book\BookOutlineStorageInterface?
Comment #40
marcingy CreditAttribution: marcingy commentedLooks good!
Comment #41
fgmThe changes look good, but there are still a number of @todo remaining, even in the sections which are changed by the patch, for example:
BookManager::loadBooks(): sort nodes after loading them from storage : I think this is not actually needed, because it is extra work over loading, which is not necessarily in all cases, and can be done in another method if several book use cases need sorting. Suggestion: remove the todo.
Many others are unrelated with storage/loading, so probably don't need to be touched in this patch.
One which is a bit more worrying is in doBookTreeCheckAccess, which invokes bookLinkTranslate in a loop, and recursively, where each call performs two single loads from (node) storage. This could probably be greatly accelerated using some transformation to replace recursion and single loads by one or two loadMultiple().
Also, bookTreeCheckAccess() was modified by the patch to replace the SQL query with an EntityQuery, but did not address the todo regarding filtering, although the patch changes the querying logic.
Comment #42
slashrsm CreditAttribution: slashrsm commentedThis issue is about making book storage independent and not about fixing existing @todos. We are not changing any logic in this patch. We only re-implement existing logic in a more friendly way when it comes to storage swapability.
I totally support rising follow-ups (which should have been done when @todos were added to the codebase) and addressing things that are mentioned in #41 there. Fixing that stuff is out of this issue's scope and would make patch unnecessarly complex.
Comment #43
fgmFine with me : let's add follow-ups once this is committed.
Comment #44
alexpottWe're still calling the service book.storage and the using the variable names book_storage and bookStorage - let's add the make these book.outline_storage, book_outline_storage and bookOutlineStorage respectively.
Comment #45
slashrsm CreditAttribution: slashrsm commentedAh... my bad. Good catch!
Comment #46
marcingy CreditAttribution: marcingy commentedComment #47
alexpottCan someone update the issue summary to be aligned to the solution in the latest patch - also I think https://www.drupal.org/node/2208415 could be fleshed out and linked to this issue.
Comment #48
slashrsm CreditAttribution: slashrsm commentedComment #49
slashrsm CreditAttribution: slashrsm commentedDone.
Comment #50
alexpottCommitted ae319dd and pushed to 8.0.x. Thanks!