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.
Problem/Motivation
#2286971-24: Remove dependency of current_user on request and authentication manager:
Since we have entity caching in core now, book module should absolutely not be doing anything access-dependent in book_node_load()
.
This is critical because it blocks a critical #2286971: Remove dependency of current_user on request and authentication manager.
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#34 | 2380615-34.patch | 7.42 KB | dawehner |
#32 | interdiff.txt | 1.09 KB | dawehner |
#32 | 2380615-32.patch | 263.78 KB | dawehner |
#29 | interdiff.txt | 1.96 KB | dawehner |
#29 | 2380615-29.patch | 6.98 KB | dawehner |
Comments
Comment #1
BerdirWhile it is problematic, the description here is not quite correct.
persistent entity caching does include hook_node_load(), only hook_node_storage_load(), hook_node_load() is still called once per request. But of course, if the user switches, then it is broken as well.
Comment #2
dawehnerStarted to work on this issue: Added some multiloading for nodes in the book, and ported over some of the menu link improvements.
Well, in general I think switching users, at least manually is an edge case.
In general I think its a sign that we really can't just authenticate after routing, i'm still convinced that lazy-authentication ensures that you never deal with the wrong user.
Comment #4
dawehnerSome work.
Comment #6
dawehnerNow, some propery work.
Comment #8
dawehnerNew, much more simpler approach.
Comment #9
dawehnerThis time with a test as well.
Comment #10
BerdirWould make sense to add the metadata inside the if as well, as that belongs together?
Comment #11
dawehnerValid point, thank you!
Comment #15
dawehnerDoh, I was sure I fixed that already.
Comment #16
swentel CreditAttribution: swentel commentedtypo: hook_node_lode()
Comment #17
dawehnerHa, a lode though seems to be a real existing thing: http://en.wikipedia.org/wiki/Lode
Comment #20
larowlan- * @return array - * Array of loaded book items. + * @return array Array of loaded book items. + * Array of loaded book items. */
Other Than this, rtbc
Comment #21
swentel CreditAttribution: swentel commentedComment #22
larowlanLooks good, tests fine
Comment #23
larowlanComment #24
alexpottThis looks super odd - why are we passing boolean about translation into a variable about access?
Comment #25
alexpottBack to needs review for #24 - if this is correct perhaps a comment in the code would be nice.
Comment #26
dawehnerComing from the good old days of from
menu.inc
we have this odd thing called translating.Translating is pretty much access checking though these days (
See the following code:
Having a better name would require API changes, let's avoid this here. So back to RTBC
Comment #27
webchickBack to Alex.
Comment #28
alexpottThe documentation for loadBookLinks looks like it could do with improving considering how this issue changes things. Since this is now about respecting access and therefore has security implications.
Comment #29
dawehnerThere we go.
Comment #30
znerol CreditAttribution: znerol commentedApplying that patch on top of #2286971: Remove dependency of current_user on request and authentication manager fixes the failing
BookTest
over there.Comment #31
larowlanThis looks orphaned - should it be a @see?
Comment #32
dawehnerYeah sure, let's add a @see
Comment #33
larowlanBack we go
Comment #34
dawehnerLet's do a better rebase
Comment #35
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 2874862 and pushed to 8.0.x. Thanks!