Problem/Motivation
On the AccountInterface
The methods getSessionId()
, getSecureSessionId()
and getSessionData()
are completely unused and superflous. The introduction of these methods probably was based on the misconception that a user is always authenticated using a session. This is not the case anymore.
In addition those methods do not make any sense at all on user entities loaded from the disk (i.e. user entity !=
current user). Typehinting against the AccountInterface
in order to get at the session id when it is not guaranteed that it is actually there is pointless - and that's probably also the reason why those methods are not used at all.
Rather code which relies on the session should retrieve it directly from the $request
via getSession()
.
Proposed resolution
Remove those methods.
Remaining tasks
User interface changes
API changes
Remove the following methods:
AccountInterface::getSessionId()
AccountInterface::getSecureSessionId()
AccountInterface::getSessionData()
AccountInterface::getHostname()
Follow-up to #1858196: [meta] Leverage Symfony Session components
Beta phase evaluation
Issue category | Bug because this removes broken code |
---|---|
Issue priority | Normal |
Disruption | Disruptive for contributed and custom modules because it will require a BC break |
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff.txt | 640 bytes | andypost |
#30 | remove_session_related-2347799-30.patch | 8.89 KB | andypost |
#18 | remove_session_related-2347799-18.patch | 8.52 KB | andypost |
Comments
Comment #1
znerol CreditAttribution: znerol commentedComment #3
znerol CreditAttribution: znerol commentedClosing in favor of #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries.
Comment #4
almaudoh CreditAttribution: almaudoh commentedRerolled. I think we should continue with this patch since #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries seems to be held up on other things.
Comment #5
xjmDiscussed with @znerol -- this is actually a bug, because the methods do not work reliably.
Comment #6
xjmComment #7
znerol CreditAttribution: znerol commentedComment #8
andypost+1, RTBC
Suppose it needs CR also
Comment #9
znerol CreditAttribution: znerol commentedThe
AccountInterface
still mentions sessions in some comments. Those should be reworded.There is an existing change record mentioning the
getSession*
methods. I propose to just remove the methods from there.It seems to me that
AccountInerface::getHostname()
is broken as well. Thehostname
column is only present in thesession
table, but not in theuser
table. The property on the user entity just returns the client ip (which is obviously wrong for users loaded from the database). Also the current ip is readily available from$request->getClientIp()
. Already in D7 core the$user->hostname
property is never used (git grep -- "->hostname"
). Even thepoll
module usesip_address()
instead,comment
does the same.Comment #10
almaudoh CreditAttribution: almaudoh commentedI think we should also handle the disparity btw
User::getLastAccessedTime()
andUserSession::getLastAccessedTime()
in this patch.Comment #11
znerol CreditAttribution: znerol commentedWhat about that: In the
UserSession
class rename thetimestamp
property toaccess
(also bring comments inline with the ones inUser
/UserInterface
entity) and then just remove the workaround from thecookie
provider.Comment #12
almaudoh CreditAttribution: almaudoh commentedThis would entail renaming the column in the {session} table (or transfer the workaround to the
SessionHandler
)Comment #13
znerol CreditAttribution: znerol commentedI don't think so. The
timestamp
column is set toREQUEST_TIME
inSessionHandler::write()
, it is not accessed at all inSessionHandler::read()
and only used as a secondary index in order to purge old entries inSessionHandler::gc()
. Thesession.timestamp
column became completely meaningless outside of this class.Note that in Drupal 7 it played an important role. It was used to throttle writes on the session table if the session record was not modified during a request. This role has been delegated to the
WriteCheckSessionHandler
proxy class in Drupal 8.Comment #14
znerol CreditAttribution: znerol commentedAs per #9ff, setting this to CNW, I will watch this from the side-line and stick to the reviewer role here.
Comment #15
BerdirAgree with separating this from #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries. The methods already don't make sense anymore, as not even UserSession is implementing them. It used to be that switching to a loaded user entity changed that behavior, but that's no longer the case, so fixing as a separate bug makes sense.
Comment #16
almaudoh CreditAttribution: almaudoh commentedDone #9/#10/#11. Also removed the test coverage for the deleted code. Change records will be updated after commit.
Comment #17
znerol CreditAttribution: znerol commentedComment #18
andypostPatch cleans remains in AccountProxy and user entity.
Do we really need empty
AnonymousUserSession
?This makes this class empty with overridden constructor, so maybe we need follow-up to remove it
Comment #19
almaudoh CreditAttribution: almaudoh commentedI left it in because of the comment:
Comment #21
znerol CreditAttribution: znerol commentedThis is likely a random test fail (#2474909: Allow Simpletest to use the same APC user cache prefix so that tests can share the classmap and other cache objects). Please reupload the patch.
Comment #23
znerol CreditAttribution: znerol commentedI very much like the diffstat here. Thanks!
After commit, one existing change record needs to be updated.
Comment #24
andypost#9 - could be done in follow-up? I think user.module has some such comments
Comment #25
alexpottRe #9 and #24 I'm not sure about that I think we should do the entire clean up in one issue (as far as possible). Isn't it an issue that the namespace of
AccountInterface
is\Drupal\Core\Session\AccountInterface
? Should it be in\Drupal\Core\Session
?Comment #26
znerol CreditAttribution: znerol commentedThe
AccountInterface
represents an anonymous or authenticated user of the site. I'd probably move it into\Drupal\Core\Authentication
. However that would be a major BC break which not only affects core but also contrib in many ways.Comment #27
znerol CreditAttribution: znerol commentedFiled #2477213: Move AccountInterface out of Session namespace.
The only place where the session is still mentioned in the
AccountInterface
is in the interface description. I propose to just delete the description entirely and only keep the first line (i.e. the brief description).Comment #28
andypostI think better to fine-tune the description
Comment #29
znerol CreditAttribution: znerol commentedThe first sentence is still weird. The interface cannot really define an object - or can it. Also the comma should be an and now that the last part of the enumeration is gone.
How about currently logged in user instead of global?
Comment #30
andypostChecked a way we describe interfaces - most of them one-liners
Also there's more then 2 implementations:
Comment #31
znerol CreditAttribution: znerol commentedThanks. #30 addresses the easy part of the concerns in #25, the follow-up #2477213: Move AccountInterface out of Session namespace will address the complicated ones.
Comment #33
xjmNice diffstat.
This issue is a normal bug fix, reduces fragility by narrowing the API, and has minimal and intentional disruption (in that any code using these methods is kinda broken). So, it is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.
Let's make sure the change record updates get made now (just some easy deletions I think) as I checked and they haven't been yet. We can remove the issue tag once that's done. Thanks!
Comment #34
andypostUpdated inline with current state
https://www.drupal.org/node/2017231/revisions/view/8380013/8400985