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

Reference: https://www.drupal.org/core/beta-changes
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
CommentFileSizeAuthor
#30 AccounInterface.png27.04 KBandypost
#30 interdiff.txt640 bytesandypost
#30 remove_session_related-2347799-30.patch8.89 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,792 pass(es). View
#28 remove_session_related-2347799-28.patch8.93 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,790 pass(es). View
#28 interdiff.txt674 bytesandypost
#18 remove_session_related-2347799-18.patch8.52 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,715 pass(es). View
#18 interdiff.txt1.48 KBandypost
#16 interdiff.txt4.7 KBalmaudoh
#16 remove_session_related-2347799-16.patch7.74 KBalmaudoh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,719 pass(es). View
#4 remove_session_related-2347799-4.patch3.75 KBalmaudoh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,452 pass(es). View
#1 2347799-remove-session-related-methods-from-account-interface.diff3.73 KBznerol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,743 pass(es), 0 fail(s), and 5 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

znerol’s picture

Status: Active » Needs review
FileSize
3.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,743 pass(es), 0 fail(s), and 5 exception(s). View

Status: Needs review » Needs work
znerol’s picture

Status: Needs work » Closed (duplicate)
almaudoh’s picture

Status: Closed (duplicate) » Needs review
FileSize
3.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,452 pass(es). View

Rerolled. I think we should continue with this patch since #2345611: Load user entity in SessionHandler instead of using manual queries seems to be held up on other things.

xjm’s picture

Category: Task » Bug report

Discussed with @znerol -- this is actually a bug, because the methods do not work reliably.

xjm’s picture

Title: Remove session related methods from AccountInterface » Remove bugged session-related methods from AccountInterface
znerol’s picture

andypost’s picture

Issue tags: +Needs beta evaluation

+1, RTBC
Suppose it needs CR also

znerol’s picture

Issue summary: View changes

The 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. The hostname column is only present in the session table, but not in the user 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 the poll module uses ip_address() instead, comment does the same.

almaudoh’s picture

I think we should also handle the disparity btw User::getLastAccessedTime() and UserSession::getLastAccessedTime() in this patch.

znerol’s picture

I think we should also handle the disparity btw User::getLastAccessedTime() and UserSession::getLastAccessedTime() in this patch.

What about that: In the UserSession class rename the timestamp property to access (also bring comments inline with the ones in User/UserInterface entity) and then just remove the workaround from the cookie provider.

almaudoh’s picture

In the UserSession class rename the timestamp property to access

This would entail renaming the column in the {session} table (or transfer the workaround to the SessionHandler)

znerol’s picture

This would entail renaming the column in the {session} table (or transfer the workaround to the SessionHandler)

I don't think so. The timestamp column is set to REQUEST_TIME in SessionHandler::write(), it is not accessed at all in SessionHandler::read() and only used as a secondary index in order to purge old entries in SessionHandler::gc(). The session.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.

znerol’s picture

Status: Needs review » Needs work

As per #9ff, setting this to CNW, I will watch this from the side-line and stick to the reviewer role here.

Berdir’s picture

Agree with separating this from #2345611: Load user entity in SessionHandler 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.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,719 pass(es). View
4.7 KB

Done #9/#10/#11. Also removed the test coverage for the deleted code. Change records will be updated after commit.

znerol’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation
andypost’s picture

FileSize
1.48 KB
8.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,715 pass(es). View

Patch cleans remains in AccountProxy and user entity.

Do we really need empty AnonymousUserSession?

+++ b/core/lib/Drupal/Core/Session/AnonymousUserSession.php
@@ -18,9 +18,6 @@ class AnonymousUserSession extends UserSession {
    * Intentionally don't allow parameters to be passed in like UserSession.
...
   public function __construct() {
-    if (\Drupal::hasRequest()) {
...
   }

This makes this class empty with overridden constructor, so maybe we need follow-up to remove it

almaudoh’s picture

Do we really need empty AnonymousUserSession?

I left it in because of the comment:

  /**
   * Constructs a new anonymous user session.
   *
   * Intentionally don't allow parameters to be passed in like UserSession.
   */

Status: Needs review » Needs work

The last submitted patch, 18: remove_session_related-2347799-18.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review
znerol’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +needs change record updates

I very much like the diffstat here. Thanks!

After commit, one existing change record needs to be updated.

andypost’s picture

The AccountInterface still mentions sessions in some comments. Those should be reworded.

#9 - could be done in follow-up? I think user.module has some such comments

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Re #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?

znerol’s picture

The 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.

znerol’s picture

Filed #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).

andypost’s picture

Status: Needs work » Needs review
FileSize
674 bytes
8.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,790 pass(es). View

I think better to fine-tune the description

znerol’s picture

+++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -10,8 +10,8 @@
+ * Defines an object that has a user id, roles. The interface is implemented

The 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.

+++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -10,8 +10,8 @@
+ * both by the global user account and the user entity.

How about currently logged in user instead of global?

andypost’s picture

Issue summary: View changes
FileSize
8.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,792 pass(es). View
640 bytes
27.04 KB

Checked a way we describe interfaces - most of them one-liners

Also there's more then 2 implementations:

znerol’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. #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.

  • xjm committed f559ff7 on 8.0.x
    Issue #2347799 by andypost, almaudoh, znerol: Remove bugged session-...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Nice 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!

andypost’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.