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.
Convert the book_export page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686
Comment | File | Size | Author |
---|---|---|---|
#87 | drupal8.book-module.1938314-87.patch | 19.68 KB | jibran |
#85 | drupal8.book-module.1938314-85.patch | 19.69 KB | Letharion |
#83 | drupal8.book-module.1938314-83.patch | 19.7 KB | jibran |
#81 | book-1938314-81.patch | 19.7 KB | tim.plunkett |
#75 | drupal8.new_book_export_controller.1938314-74.patch | 20.62 KB | disasm |
Comments
Comment #1
DamienMcKennaComment #2
Kars-T CreditAttribution: Kars-T commentedPulling to me
Comment #3
DamienMcKennaComment #4
Kars-T CreditAttribution: Kars-T commentedFinally a first version :)
Worked in my manual tests but I am unsure about the comments.
Comment #5
Kars-T CreditAttribution: Kars-T commentedPatch before was faulty. Sorry. New version attached.
Comment #6
Kars-T CreditAttribution: Kars-T commentedThird version with hopefully no mistakes...
Comment #7
Crell CreditAttribution: Crell commentedWe discussed in IRC that we need to change the callback mechanism for the type-specific importers, but that can happen separately. #6 looks good to me visually. I'll wait for testbot before RTBCing it.
Comment #8
Crell CreditAttribution: Crell commentedThe bots are overloaded today, and webchick said to go ahead, so...
Comment #10
disasm CreditAttribution: disasm commentedAttached is a reroll
Comment #11
disasm CreditAttribution: disasm commentedignore the interdiff, I didn't change anything, so it's identical to the patch.
Comment #13
disasm CreditAttribution: disasm commentedlost a use flag in the reroll. Adding back in.
Comment #14
disasm CreditAttribution: disasm commentedThis isn't addressed by the _permission: 'access printer-friendly version' added to the routing YAML, hence why node_access tests are failing by testbot. This will need an access checker class added to handle the node access stuff.
Comment #16
kim.pepperdisasm, are you still working on this?
Comment #17
kgoel CreditAttribution: kgoel commentedGoing to work on this
Comment #18
kgoel CreditAttribution: kgoel commentedComment #20
kgoel CreditAttribution: kgoel commentedComment #22
dawehnerMenu callbacks now can be totally removed from hook_menu
Let's use NodeInterface in documentation/function signature.
Should be Unicode::strtolower($text)
In order to call the export functions you have to be include book.pages.inc. (maybe you could convert them into a helper object). Additional: I think instead of printing there should be a response object returned.
The proper way would be to return a render array.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll the #20 patch as per the suggestions in the #22
Can anybody elaborate on these so that I can also work on these suggestions.
Need Help in these.Your help will be appreciated.
Comment #24
Crell CreditAttribution: Crell commentedThis entire menu item can be removed completely.
Instead of EntityInterface, the $node variable should be type hinted with NodeInterface. The "use" statements above will then also need to be updated to include Drupal\node\NodeInterface.
Same as above.
$type = drupal_strtolower($type); can be replaced with $type = Unicode::strtolower($type); Again, you'll need a "use" statement for it.
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedApplied all the suggestions given in #24.Please Needs Review.
Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commented#26: book-export-1938314-26.patch queued for re-testing.
Comment #29
kgoel CreditAttribution: kgoel commentedComment #31
Crell CreditAttribution: Crell commentedThe route needs a custom access checker to match the logic in the old menu item. That's why the access test is failing. See https://drupal.org/node/1851490 for instructions on how to do that.
I don't know what the title errors are all about...
Comment #32
kgoel CreditAttribution: kgoel commentedComment #34
dawehnerIt seems to be that it would be helpful to return a full reponse object?
Comment #35
Crell CreditAttribution: Crell commentedNot just helpful; required. :-) Crell-- for not catching that before.
Nothing here is connecting the new access checker. What you need to do instead of specifying a permission is specify a requirement of _book_export_access: 'TRUE', which you can then check for in the applies() method (note the _ prefix).
As a method this isn't going to work. the bookExport() method still expects it to be a function. Sucks, but we can't fix that here.
Comment #36
disasm CreditAttribution: disasm commentedlets make this method name exportHTML
these should either be methods in this class or added to the book service.
Comment #37
dawehnerWhile working on fixing stuff I realized something totally else. Book export should be using serializer instead of its own custom, hard to extend logic, but this would require certainly more effort.
This is just a patch which passes the tests.
Comment #39
stella CreditAttribution: stella commented#37: book-1938314-37.patch queued for re-testing.
Comment #40
disasm CreditAttribution: disasm commentedThis code looks very nice and clean! If we're not going to serializer as dawehner mentioned above, RTBC++. Adding Stalking Crell to get his thoughts on committing as is vs. switching to serializer.
Comment #41
dawehnerI don't we should block this on converting to serializer now, as it might be complicated.
Comment #42
Crell CreditAttribution: Crell commentedActually... dawhner, couldn't this be replaced with an ANDed _permission and _entity_access on the route, no new checker needed? We spent all that time talking about it with chx in Portland, we may as well use that functionality. :-)
Isn't this a regression? If we insist that the format be a method on this class, contrib can no longer add new formatters here. Mind you, I don't know that contrib ever did so, and I don't know why it would ever want to, but...
I definitely agree on punting on serializer. I don't know what would be involved there, but probably more than we want to deal with. Plus, I'd figured this would end up as plugins. That said... since this is such an obscure corner of the code it would not surprise me if we could change it later (serializer or plugins or both) without it qualifying as an API change. And if not, meh, it's not the end of the world.
Comment #43
dawehnerYeah, once #1947880: Replace node_access() by $entity->access() is in, we can certainly do that.
So as the REST team is already full with work, getting a conversion going on is really unlikely, so we might just live with what we have at the moment, which means that it should be set to needs work again?
Comment #44
dawehnerThis is regarding using && and _entity_access
Comment #45
Crell CreditAttribution: Crell commentedBah. I see. OK, we should probably have a node access-specific checker, but we can punt that I guess. What about the formatter regression?
Comment #46
ygerasimov CreditAttribution: ygerasimov commentedI propose to move serializing responsibility out of controller and register that class in container. In this way if someone would like to extend that functionality they can swap class in container.
Comment #47
Crell CreditAttribution: Crell commentedmeh. I can live with that.
Comment #48
Crell CreditAttribution: Crell commentedWait, no, I can't live with that. This should be properly injected.
Comment #49
ygerasimov CreditAttribution: ygerasimov commentedPatch corrected according to #48.
Comment #50
Crell CreditAttribution: Crell commentedNow I can live with that. :-)
Comment #51
alexpottThis should be using the user on the request and the hasPermission() method on the account interface.
As above... also do we really need to do this here considering the BookAccessCheck already does this?
Let's refactor book_node_export() into the BookExport class as well as it is only called from methods already refactored to this class.
Can we not return a render array here?
This should be
@param callable $visit_func
as we can pass an array instead of a string to call a method on the class.Comment #52
tim.plunkettThis should cover @alexpott's review.
Comment #53
jibranCan we move this function to
bookManger
?This doc block needs update.
Comment #54
tim.plunkettbook_menu_subtree_data() is used in a bunch of places, I think that could be its own conversion.
Or maybe after I get this green.
node_access() has logic that $node->access() doesn't, that's messed up.
Don't see any need to bother Crell with this issue.
Comment #55
mparker17Will re-test.
Comment #56
mparker17Patch still applies.
Code looks good.
When I manually tested the patch, though, it doesn't act the same as it did before the patch.
My manual test steps are:
Before applying the patch, clicking the "Printer-friendly version" link in the bottom-right of the page would display the book with no theme or Drupal page wrapper at all.
After applying the patch, clicking the "Printer-friendly version" link in the bottom-right of the page displays the book in the content area of a normal Drupal page (i.e.: themed and with the Drupal page wrapper). While that functionality may be desirable to some people, I think that decision and the work around it is out-of-scope for this issue.
Unfortunately, that means this issue gets bumped back to "needs work".
Comment #57
mparker17Leaving this assigned to myself so I can fix it. :)
I talked briefly with @Crell about how to fix this.
He was thinking that we should change the route defaults from
_content
to_controller
, and adjust the controller object to return a response object containing the HTML.Comment #58
mparker17Try this...
Comment #60
mparker17*sad trombone* will fix.
Comment #61
mparker17Try this...
Comment #62
mparker17Or rather, this...
Comment #64
mparker17So, it turns out that interdiffs are not good substitutes for patches. Thanks @disasm! Let's try this again...
Interdiff is between this and #61.
Comment #65
mparker17Comment #66
Crell CreditAttribution: Crell commentedAk! How'd this get through? \Drupal is not allowed inside classes. It should be injected.
Ibid.
Comment #67
tim.plunkettWe can't inject in entity classes yet.
Comment #68
dawehnerI am wondering whether a notfoundhttpexception should be thrown in such a enviroment and not in the actual controller ...
Comment #69
Crell CreditAttribution: Crell commentedtim: Ugh. Still? Fine.
dawehner: You are correct, an *HttpException shouldn't be thrown below the controller level.
Comment #70
wamilton CreditAttribution: wamilton commentedThis patch needed a reroll and per talking to dawehner in IRC, I changed the exception class to just plain Exception.
Comment #72
disasm CreditAttribution: disasm commented#70: book-new_book_export_controller-1938314-70.patch queued for re-testing.
Comment #74
dawehner@wamilton
Just in general it would be nice to provide an interdiff all the time.
Comment #75
disasm CreditAttribution: disasm commentedreroll of #64. Interdiff is changes from #70 (changing NotFoundException to Exception).
Comment #76
dawehnerMh, I actually thought that we still throw NotFoundException on the controller level.
Comment #77
Letharion CreditAttribution: Letharion commentedI could definitely be wrong here, but after reading the patch in #75, I get the impression it's throwing the correct exceptions. The controller uses HttpNotFound while the export code that shouldn't know anything about httpuses a plain exception.
That looks right to me. :)
Comment #78
dawehnerOh, well, maybe it was just too late/early.
Comment #79
Crell CreditAttribution: Crell commentedThis is going to conflict with #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface, but at least for posterity...
Comment #80
webchickNo longer applies.
Comment #81
tim.plunkettStraight reroll. Nothing actually broke, just patch context
Comment #82
alexpottNeeds reroll
Comment #83
jibranJust use statement conflict nothing big.
PS: I am going to reroll some patches while @tim.plunkett is sleeping :D
Comment #84
alexpottPatch no longer applies.
Comment #85
Letharion CreditAttribution: Letharion commentedStraight re-roll.
Comment #86
jibranthis should be @entity.manager now.
Comment #87
jibranComment #88
disasm CreditAttribution: disasm commentedMoving back to RTBC after reroll. There are a number of improvements to clean this up that could be done, but we're trying to get these committed as quickly as possible so we can get rid of the old menu router. I would like to see a follow-up issue to extend ControllerBase, use $this->t(), and use proper injection instead of calling \Drupal::moduleHander() in classes, but after we get all of the book module using the new router, we could do a single issue for cleanup of that module.
Comment #89
Crell CreditAttribution: Crell commenteddisasm: I think #2077513: Refactor ControllerBase to be more consistent with FormBase is what you're looking for. (Not a blocker here.)
Comment #90
webchickAgreed; we need to get these things converted sooner than later, so let's leave polishing niceties for follow-up issues.
Committed and pushed to 8.x. Thanks!