Currently we are referencing \Drupal\user\User in some (mostly entity related) places in \Drupal\Core:

  • Entity.php
  • EntityAccessController.php (not yet committed)
  • EntityAccessControllerInterface.php (not yet committed)
  • AccessibleInterface.php
  • EntityTranslation.php
  • Field.php

This violates the rule that we should never depend on module level code in \Drupal\Core or \Drupal\Component namespaces. We could solve that problem by adding a AccountInterface / UserInterface to our \Drupal\Core namespace. It's a question of terminology. I would prefer a UserInterface interface because, even though we are currently using $account as variable name in those places it's actually a user object (account == authentication information // user == an actual user).

Files: 
CommentFileSizeAuthor
#101 account-interface-1825332-101.patch48.47 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 55,074 pass(es).
[ View ]
#96 account-interface-1825332-96.patch92.53 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,234 pass(es).
[ View ]
#96 account-interface-1825332-96-interdiff.txt568 bytesBerdir
#94 account-interface-1825332-94.patch48.46 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 56,792 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#94 account-interface-1825332-94-interdiff.txt595 bytesBerdir
#88 account-interface-1825332-88.patch48.45 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#88 account-interface-1825332-88-interdiff.txt7.57 KBBerdir
#86 account-interface-1825332-86.patch40.88 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,911 pass(es), 330 fail(s), and 3,483 exception(s).
[ View ]
#86 account-interface-1825332-86-interdiff.txt1.7 KBBerdir
#83 account-interface-1825332-83.patch40.48 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#77 account-interface-1825332-77.patch36.25 KBjrglasgow
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-77.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#69 account-interface-1825332-69.patch37.13 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-69.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#69 account-interface-1825332-69-interdiff.txt1.72 KBBerdir
#67 drupal-1825332-67.patch37.13 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#67 interdiff.txt1.3 KBdawehner
#64 drupal-1825332-63.patch33.75 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#61 interdiff.txt1.02 KBandypost
#61 account-interface-1825332-61.patch37.06 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-61.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#59 account-interface-1825332-59.patch36.03 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,934 pass(es), 1 fail(s), and 8 exception(s).
[ View ]
#55 account-interface-1825332-55.patch51.8 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#55 account-interface-1825332-55-interdiff.txt4.03 KBBerdir
#53 account-interface-1825332-53.patch47.77 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 53,888 pass(es), 618 fail(s), and 312 exception(s).
[ View ]
#52 account-interface-1825332-52.patch48.02 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-52.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#43 no-interface-on-user-1825332-43.patch47.92 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,501 pass(es), 214 fail(s), and 11,437 exception(s).
[ View ]
#43 no-interface-on-user-1825332-43-interdiff.txt456 bytesBerdir
#37 account-interface-1825332-37.patch48.06 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 53,876 pass(es).
[ View ]
#30 user-session-interface-1825332-30.patch48.49 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,580 pass(es).
[ View ]
#30 interdiff.txt1.01 KBParisLiakos
#27 user-session-interface-1825332-27.patch48.49 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 53,323 pass(es).
[ View ]
#27 user-session-interface-1825332-27-interdiff.txt9.59 KBBerdir
#21 user-session-interface-1825332-21.patch45.36 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 53,318 pass(es).
[ View ]
#21 user-session-interface-1825332-21-interdiff.txt39.68 KBBerdir
#16 user-session-interface-poc-1825332-16.patch5.68 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,433 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#16 user-session-interface-poc-1825332-16-interdiff.txt4.27 KBBerdir
#14 user-session-interface-poc-1825332-13.patch3.13 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#13 user-session-interface-pox-1825332-13.patch0 bytesBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

Component:other» base system
Issue tags:+Entity system, +Entity Access

If we weren't in bad shape with thresholds I'd say this deserves a major priority. AAMOF it's indicating we have strict coupling between many Core subsystems and the User system, which should probably be split into a Core subsystem and the UI in the usual User module.

It's bad, really bad, but it doesn't break anything. Let's not freak @webchick out by setting it to major :P.

I think even though this is not really related it still shows that the hard-wired connection between $user and $account ('connection' is not even the right word for the current situation... right now both are the same thing actually) really sucks.

#1816218: Separate the account information from the user entity.
#1806514: Unify anonymous and registered users

It's bad, really bad, but it doesn't break anything. Let's not freak @webchick out by setting it to major :P.

Yeah, sure :D

I think even though this is not really related it still shows that the hard-wired connection between $user and $account ('connection' is not even the right word for the current situation... right now both are the same thing actually) really sucks.

+100

Btw, if we solve this properly we will probably be able to make the User module optional, which would be a Good Thing.

Looks like we could use a META issue to carve out the overall concept. We got only 4 weeks left. What should we do?

I don't think this refactoring would be bound to feature freeze. However, yes, we need to lay out a battle plan.

Priority:Normal» Major

Due to drupal_anonymous_user() still returning a stdClass object we can't even start implementing the SIngle Entity Access API yet... It breaks for anonymous users because it typehints for the User entity object. So, let's fix it now. Bumping to major.

After some discussion in IRC we came to the following conclusions:

  • You certainly don't want to put the user as global, as you might not need it
  • Additional you want to decouple the user from the "account" on the site
  • In order to allow all kind of access checks you still want to be able to load the user, so let's create a new Account object
  • This object should have a getUser(or something else) which loads the user via an injected and optional EntityManager

I think there's a lot (perhaps even too much) overlap with the Symfony sessions revamp here.

In most cases where we're referring/type-hinting a User $account, we're actually referencing a user session.

That's why I linked to the other existing issues in #6 already.

What we could potentially do here in the meantime would be to:

  1. Introduce a Drupal\Core\Session\UserSessionInterface,
  2. along with a Drupal\Core\Session\UserSession class, which for now, would simply resemble the columns/properties of {session}.
  3. In turn, drupal_anonymous_user() would return a new UserSession(array('uid' => 0));
  4. and Drupal\user\User would have to implement UserSessionInterface, too.

That would most likely be a completely temporary thing, since the Symfony sessions revamp will refactor these parts entirely.

#11 seems actually to be a very sensible thing, I was coming to the same conclusion while working on #1818570: Convert users to the new Entity Field API, that and the recent drupal_anonymous_user() (which now returns a manually instantiated User object) makes it an even bigger mess and considerably more complicated.

I'll try to start a patch here.

Status:Active» Needs review
Issue tags:+Entity Field API
StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here's a start that adds the interface, user session class, implements in User and returns it from drupal_anonymous_user() (which we should probably rename to anomymous_session or so?)

Posting to see if this breaks anything. Maybe some context tests but I think they're incorrectly using it anyway (and therefore incorrectly changed that function).

My suggestion for this issue would be to add these changes (with proper documentation of course) and change type hints of existing User $something that actually resemble a session and not a user entity. For example the entity access stuff. Then we can open various novice follow-ups to type hint more functions with it and actually use the interface methods instead of public properties and finally make those protected. We will need that to remove the EntityBCDecorator.

But already the initial patch and some key changes in affected functions would remove a lot of hacks in the User NG patch where I'm shuffling with the BC decorator and User class to get through some of the type hints.

StatusFileSize
new3.13 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Interesting "feature". Renamed the patch locally (should be poc, not pox) but already had it selected.

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -0,0 +1,33 @@
+  public $uid;
+  public $hostname;

We're missing $sid, $ssid, $session, and we should probably have corresponding getSessionID(), getSecureSessionID(), and getSessionData() for them?

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -0,0 +1,33 @@
+  public function label() {
+    // @todo Use configuration.
+    return t('Anonymous');

Hm. Does this belong in here?

StatusFileSize
new4.27 KB
new5.68 KB
FAILED: [[SimpleTest]]: [MySQL] 53,433 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Right, added that and also converted authenticated sessions to it.

Status:Needs review» Needs work
Issue tags:-Entity system, -Entity Access, -Entity Field API

The last submitted patch, user-session-interface-poc-1825332-16.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Entity system, +Entity Access, +Entity Field API

The last submitted patch, user-session-interface-poc-1825332-16.patch, failed testing.

Issue tags:+sprint

Tagging.

Will fix the test fail asap, will also see what happens if I type hint user_access() and the entity access methods.

Status:Needs work» Needs review
StatusFileSize
new39.68 KB
new45.36 KB
PASSED: [[SimpleTest]]: [MySQL] 53,318 pass(es).
[ View ]

This should fix the test fail.

Also started changing/adding the interface in some places to see if this works and if we're still passing some stdClass objects around. Patch is quite big due to that but it's mostly a simple search & replace.

That's just some feedback on top of the obvious missing documentation parts.

+++ b/core/includes/bootstrap.incundefined
@@ -2044,8 +2044,8 @@ function drupal_hash_base64($data) {
+ * @return Drupal\Core\Session\UserSessionInterface

Missing "\"

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -256,7 +257,7 @@ public function getIterator() {
+  public function access($operation = 'view', UserSessionInterface $account = NULL) {

Just in general i'm wondering whether we should rename the variable here, as $account seem to be refer to the user account aka. the entity and not the session.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessController.phpundefined
@@ -117,7 +118,7 @@ protected function access(EntityInterface $entity, $operation, $langcode = LANGU
    *   (optional) The user for which to check access, or NULL to check access
    *   for the current user. Defaults to NULL.
Maybe also change the description here?
<code>
+++ b/core/lib/Drupal/Core/Entity/EntityAccessControllerInterface.phpundefined
@@ -7,8 +7,7 @@
-// @todo Don't depend on module level code.

Nice!

I still kind of feel like this is backwards from the intent of every other entity typehint in core. I get that the global is a session. But for the most part, these hooks and access callbacks should want actual User entities, not sessions. Maybe I'm wrong in that assumption. I would expect something more like the following:

<?php
function hook_entity_access($entity, $op, UserInterface $account = NULL) {
  if (!isset(
$account)) {
   
$account = $GLOBALS['session']->getUserEntity();
  }
  .. do
logic ..
}
?>

After reading several times, I just still find UserSessionInterface $account = NULL backwards to me. Intentionally not changing issue status.

@Dave Reid: Because then you always have either a user entity or a session and right now, we already mix them, a lot. Let's assume you're passing that $account to user_access() then, how would you type hint that? We need a common interface or we end up with the mess that we have right now.

I thought about adding a getUserEntity() but that essentially means that we then again have a reference to the user.module entity class in Drupal\Core, which we want to avoid. You can do that almost as easily with user_load($account->id()).

And yes, the naming there is strange with $account/$user but that's too big to change it here. There is an issue to rename global $user to global $session.

Thanks for reviving this issue @berdir!

@Dave Reid: Because then you always have either a user entity or a session and right now, we already mix them, a lot. Let's assume you're passing that $account to user_access() then, how would you type hint that? We need a common interface or we end up with the mess that we have right now.

I thought about adding a getUserEntity() but that essentially means that we then again have a reference to the user.module entity class in Drupal\Core, which we want to avoid. You can do that almost as easily with user_load($account->id()).

And yes, the naming there is strange with $account/$user but that's too big to change it here. There is an issue to rename global $user to global $session.

Exactly... In short: We want to decouple these core systems from the user entity. There is no other way to untangling the mess that we have now.

Also, the assumption that the session has to be tied to a user is wrong (which we won't fix here, but still).

I still kind of feel like this is backwards from the intent of every other entity typehint in core.

That's the point, there should not be any entity typehints in core (except for typehints to their interfaces of course).

+++ b/core/lib/Drupal/Core/Session/UserSessionInterface.phpundefined
@@ -0,0 +1,16 @@
+  public function getRoles();

That's should not be part of the interface? Otherwise we again hard-wire this with the concept of users/roles. If you need the roles, pull them from the user entity.

+++ b/core/lib/Drupal/Core/TypedData/AccessibleInterface.phpundefined
@@ -32,6 +34,6 @@
    * @todo Don't depend on module level code.

I believe we added quite a few of those. Are you sure you didn't miss any?

Other than that this looks really good, glad someone is working on it seeing that the Symfony session issue has become rather quiet lately.

I also agree that $account should be renamed. But that's follow-up (Novice) material!

The session might at some point become not hardwired to the user but we're a long way away from that. Our permission system is tightly coupled to a user with roles, we first would have to change that before there's even a reason to really untangle all of this. A user-less session that we can't do anything doesn't bring us a single step further. Remember services authentication in tmgmt_server? :)

I've added getRoles() because we already load them at the moment and having to load the user to get the roles would be a performance regression that wouldn't bring as anything (we can later on change to lazy-load them once everything is converted to use the method) and because it will help me with the conversion to user NG (the entity BC Decorator doesn't support that data structure at the moment) and as soon as User is fully converted, the way you access the roles on the user entity will change and be different to get the roles from the user session.

So until we untangle sessions and a user for real, which I really don't expect to happen in 8.x, we need that method.

StatusFileSize
new9.59 KB
new48.49 KB
PASSED: [[SimpleTest]]: [MySQL] 53,323 pass(es).
[ View ]

Updated the patch with some documentation and fixed what I could of the reviews.

I don't think we should go much further with this issue. Next steps:
- Rename the global to session (existing issues already)
- Decide when to use $user/$account/$session and update core accordingly
- Add type hints wherever it makes sense
- Convert $session->something to the methods, possibly add some more methods if it makes sense, then make the properties protected.

Status:Needs review» Reviewed & tested by the community

this is a good step to the right direction..it just has two newlines missing but that shouldn't hold this, since it will help both user conversion to NG and the session conversion issue

Status:Reviewed & tested by the community» Needs work

this is a good step to the right direction..it just has two newlines missing but that shouldn't hold this, since it will help both user conversion to NG and the session conversion issue

Agreed, but let's at least fix the code style before putting this in the RTBC queue ;).

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
new48.49 KB
PASSED: [[SimpleTest]]: [MySQL] 53,580 pass(es).
[ View ]

To be honest i always thought git was fixing this automagically, but i just tried and thats not the case..so here is a patch that adds newlines and fixes a docblock

Status:Needs review» Reviewed & tested by the community

Thanks. No, git does not fix it automatically unless you tell it to :).

Priority:Major» Normal
Status:Reviewed & tested by the community» Needs review

As far as I can tell, this is just normal-level refactoring. There was maybe a call for it to be major back when drupal_anonymous_user() returned a stdClass in #9, but it doesn't anymore.

Calling this thing a "UserSessionInterface" is too "exposing implementation details" for my liking...

-  public function access($operation = 'view', \Drupal\user\Plugin\Core\Entity\User $account = NULL) {
+  public function access($operation = 'view', UserSessionInterface $account = NULL) {

I understand the reasoning for typehinting with an interface rather than an implementation. But I don't think the name is intuitive enough to not be a pretty large DX regression from before. Anyone with a pulse could tell you what a User is. What's a UserSessionInterface? I wouldn't know without reading the docs. Those docs btw say:

+ * Defines an object that has a user id, roles and can have session data. The
+ * interface is implemented both by the global session and the user entity.

I cannot for the life of me figure out why you'd want an interface to be implemented by two things that have zero in common with each other. That sounds like over-abstraction to me.

Does this patch actually make sense to do on its own? If it's true that this would get ripped out and re-written when we switch to Symfony session handling, to me I'd rather just do that, rather than implement something that's supposed to be a temporary shiv that we might get stuck with for 4+ years. This isn't the stage of release to be introducing things that need further clean-up.

Thanks for the feedback.

- This is major, unfortunately. The drupal_anonoymous_user() change actually made it worse, not better. Because now the global user is a User object for anon but is still a stdClass for authenticated users. So we IMHO have a bigger mess than we had before.

- global user and that anon user change are making my life hard in #1818570: Convert users to the new Entity Field API because I can no longer user drupal_anonymous_user() in session.inc (just have a look at how ugly that patch is right now :)) and I have to switch back and forth between the BC decorator and a user object now and it will definitely block the full conversion as the global user will then have a different structure.

- "Anyone with a pulse could tell you what a User is." And they would be wrong :) Because what we actually want here is a global user/user session (which is NOT User, see above) *or* a user entity. The same for things like user_access() (which right now doesn't have a type hint at all, because of this. For entity access, we just hacked our way around it but that's not a solution).

- That is exactly what this interface defines. Something that represents a user, not necessarily a user entity (which is essentially just how the user is stored but not the only way one can be represented). Maybe we just need a different name? Just SessionInterface, maybe something like AuthorizeInterface? or simply Account/UserInterface that would live in Drupal\Core, as the issue title still says?

- I am not so sure about this being ripped out with Symfony session stuff. It will be moved into the container, maybe renamed, but the concept of it will stay I *think*. And, the symfony session stuff is still under development and it's not guaranteed to make it in.

Priority:Normal» Major

or simply Account/UserInterface that would live in Drupal\Core, as the issue title still says?

+1 for that
Account/UserInterface both seem good to me, maybe AccountInterface describes it better

Mh, AccountInterface could also imply that we should use $account when it's something that could be both (is type hinted with AccountInterface) and $user only when we're actually dealing with a User Entity (type hinted with EntityInterface/UserInterface/User, depending on the outcome of the type hint entity issue). And global user would be in the container or at least renamed to $session or so, I guess.

That would make a lot more sense than the current mix that is basically "Use $account if it could maybe clash with global $user".

This one is really needed step to #1816218: Separate the account information from the user entity.
For example of terminology the Account could be usable even in install time but User exactly storable entity

StatusFileSize
new48.06 KB
PASSED: [[SimpleTest]]: [MySQL] 53,876 pass(es).
[ View ]

Here's a re-roll using AccountInterface. Simple search & replace in the patch, so it might not make 100% sense yet.

The more I think about this, the more having a hybrid of session and user seems just wrong. If we want to truly decouple session handling using other libraries or technologies, is having them tied to our entity system (via AccountInterface) wrong? I would think session is a completely separate object, that has an IP, session ID, and user ID. User entities are separate and contain nothing about the session. It seems wrong to typehint on AccountInterface and not something more like UserEntityInterface. Just lots of things "feel wrong" to me. :/

@Dave: This patch represents the status quo. It allows us to get that interface in and *start* untangling session and user. It is a fact that we right now mix up session and user randomly, Adding an interface and limiting ourself to methods defined by that interface is IMHO a first step towards a stricter separation.

If we later on decide that user entities should not be able to mask themself as an account (masking as user session interface is strange, less sure about account interface, that doesn't seem that bad to me), we could add a method or something that would allow to create an account based on a user entity and pass that around instead.

However, whether or not we want to do that, we *need* this interface unless we want to make everything a user entity, which would IMHO be a step back. This at least allows us to stay on the same position and does not prevent us from going forward later on.

I don't get that, though. It seems like we're making our first step a step backwards by conflating two things that are not remotely the same.

I would expect Users and Sessions two be two different things (because they are, completely and utterly different), and Users to include a Session via object composition (a "has a" relationship).

If those functions are expecting a session, they should be typed with SessionInterface, and if they're expecting a user, they should be typed with UserInterface, which should include a session property. I'm failing to understand why this isn't the case.

However, whether or not we want to do that, we *need* this interface unless we want to make everything a user entity, which would IMHO be a step back. This at least allows us to stay on the same position and does not prevent us from going forward later on.

Can you explain why this would be a step back? It seems to me like whether you're talking about logged in or authenticated uesrs, you're talking about users.

I believe what Berdir is referring to is that global $user is not in fact a user. It's a session with some user data crammed into it as a short-sighted performance optimization. Ish. So putting ugly cojoined interfaces around that at least acknowledges how bad it already is, so we know where the lines are we need to cut down to separate things.

StatusFileSize
new456 bytes
new47.92 KB
FAILED: [[SimpleTest]]: [MySQL] 53,501 pass(es), 214 fail(s), and 11,437 exception(s).
[ View ]

Yes, exactly.

@webchick yesterday said that clean-up time is the time to clean up not make status quo explicit. I disagree. We first need to make implicit dependencies like this explicit so that we can understand it and start to untangle them.

Let's assume we're not adding that interface, wondering how far we'd come.

Status:Needs review» Needs work

The last submitted patch, no-interface-on-user-1825332-43.patch, failed testing.

Status:Needs work» Needs review

Back to needs review for the moment, the patch in #43 wasn't supposed to pass.

I like how the patch makes use of AccountInterface, but I'd not implement it with both Session and Users.

I'd implement it only with Users, so the user entity is one implement of a user account while there might be others which might not use the Entity system at all. So avoid hard-coding the dependency on the Entity-System and possibly even Drupal here - what I think was the initial idea behind this issue.

Sessions should be able to get you a proper account object, but they are not one. Thus, a session comes with an associated account object - which might be a registered account or not. So you should be able to retrieve it from the session.

Consequently, I'd drop all session related methods from the AccountInterface and a method like getAccount() to the session instead. Thoughts?

If those functions are expecting a session, they should be typed with SessionInterface, and if they're expecting a user, they should be typed with UserInterface, which should include a session property. I'm failing to understand why this isn't the case.

Why should I be able to derive a session from an arbitrary user object - there need not be a session at all. To me the connection should be the other way, a session comes with an user account so I can access it from there.

Doing that means that we need to load the user entity even for anon users and basic things like access checks. Right now, the code is specifically optimized to not do that as we load the roles the user has. So doing that would be a performance regression, especially combined with the NG conversion.

It also means we have to fix all those fails and exceptions now, in this issue. Getting the exception in like this allows us to make additional changes on top of this. We can still work towards separating session and account/user but we can do it step by step.

True - I thought about keeping the global non-entity user for now, but base it upon a Session a class and
make functions like user_access() accept both, or just default to talking to the session? E.g. have

param SessionInterface|AccountInterface $user

I realize that's the goal you had in mind with making both implement the same interface. But I see the point of webchick that this is confusing.

Or could we implement a session AccountInterface object which proxies the full user entity and lazy-loads it only if necesary?

Well, accept both can't be type hinted, that's what the common interface would be for :)

Having a method that either accepts this or the other interface sounds at least as confusing to me :)

Doing something fancy like proxying IMHO requires that all code that does something with it is using methods (I'd like to avoid more __get() magic) and to do that, we need this in as a first step :)

That's my main idea here, see also #42:
1. Get an interface in that describes the current situation and makes it explicit. That is where I disagree a bit with @webchick in #40, we're not *making* those things the same here by adding the interface. We're just documenting the fact this is already the case. Has been since many versions.
2. Convert code to use those methods instead of public properties.
3. Improve it further, proxy things, allow to get one thing from the other, ...

Doing something fancy like proxying IMHO requires that all code that does something with it is using methods (I'd like to avoid more __get() magic) and to do that, we need this in as a first step :)

I don't see a problem with __get() as EntityNG will add them anyway, however I'd prefer to not have another proxy object. E.g. we've not figured out how we type-hint really, so this might create troubles elsewhere.

1. Get an interface in that describes the current situation and makes it explicit. That is where I disagree a bit with @webchick in #40, we're not *making* those things the same here by adding the interface. We're just documenting the fact this is already the case. Has been since many versions.

Indeed, you convinced me this is the best option we've now. :/ But then, could we document that at the session that this should be moved to a getAccount() helper in future. This would aid anyone who looks at it and tries to understand why the session functions as account :D

I fully agree with @Berdir and @Crell that we need clearly identify and explicitly denote our existing garbage, in order to potentially clean it up properly later.

Thus, this patch is a step forward, regardless of which exact incarnation lands. Details are pretty much irrelevant. The entire code is scheduled for replacement/deletion anyway. The focus needs to be on progress.

Even if we unexpectedly had to release with this shiv, it would be an improvement compared to status quo. But that's unlikely to happen, because (1) we have plenty of months left, and (2) we're still shooting for replacing the entire session handling altogether.

I'd recommend to move forward with @Berdir's patch that was originally RTBC'ed.

StatusFileSize
new48.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-52.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is a reroll of #37

StatusFileSize
new47.77 KB
FAILED: [[SimpleTest]]: [MySQL] 53,888 pass(es), 618 fail(s), and 312 exception(s).
[ View ]

urm i forgot git pull, last patch will fail
also added some inheritdocs while i was there

Status:Needs review» Needs work

The last submitted patch, account-interface-1825332-53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.03 KB
new51.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I always knew that @inheritdoc is bad, look how many fails it caused :p

Fixing the comment access controller that got in in the meantime ;)

@sun: Thanks for the comment, not sure which patch you meant, the initial one with UserSessionInterface or the newer one (that was never RTBC'd) with AccountInterface? And what do others think?

Status:Needs review» Needs work
Issue tags:+Entity system, +sprint, +Entity Access, +Entity Field API

The last submitted patch, account-interface-1825332-55.patch, failed testing.

patch needs re-roll

Status:Needs work» Needs review
StatusFileSize
new36.03 KB
FAILED: [[SimpleTest]]: [MySQL] 55,934 pass(es), 1 fail(s), and 8 exception(s).
[ View ]

Re-rolled with

class User extends Entity implements UserInterface {
...where
interface UserInterface extends EntityInterface, AccountInterface {

Let's see broken tests

Status:Needs review» Needs work

The last submitted patch, account-interface-1825332-59.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new37.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-61.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.02 KB

Should be green now

Status:Needs review» Needs work
Issue tags:+Entity system, +sprint, +Entity Access, +Entity Field API

The last submitted patch, account-interface-1825332-61.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new33.75 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Here is a simple rerole, which tries to improves just little things in the docs. In general the docs should make clear why there is a need for an AccountInterface and the advantages of having one.

Here is a simple rerole, which tries to improves just little things in the docs. In general the docs should make clear why there is a need for an AccountInterface and the advantages of having one.

Status:Needs review» Needs work

The last submitted patch, drupal-1825332-63.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.3 KB
new37.13 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Forgot to add the AccounterInterface/UserSession object.

Status:Needs review» Needs work

The last submitted patch, drupal-1825332-67.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.72 KB
new37.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-69.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should install again.

Looks good, quick question,

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -173,9 +173,38 @@ class User extends Entity implements UserInterface {
   /**
-   * Implements Drupal\Core\Entity\EntityInterface::id().
+   * {@inheritdoc}
    */
   public function id() {
     return $this->uid;

How will this inheritdoc resolve since EntityInterface and AccountInterface both have an id() method?

Good question, depends on the implementation I guess. It's essentially the same, so doesn't really matter.

Alternatively, we could also rename id() on AccountInterface to getUserId() or so, then they would be separated but then User would have both getUserId() and id() and you would have to use either one depending on which interface you are currently working against. Not sure what's better?

+++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -0,0 +1,58 @@
+ * Defines an account interface which represents the current user.

That's not true, is it? When user accounts implement that as well, it's not necessarily the current user. So it represents accounts, optionally it's the currently logged in account.

+++ b/core/lib/Drupal/Core/Session/AccountInterface.php
@@ -0,0 +1,58 @@
+ * Defines an object that has a user id, roles and can have session data. The

It can have session data, but when? I suppose when the user is the logged in user only?

Good question, depends on the implementation I guess. It's essentially the same, so doesn't really matter.

Yes, it has to be the same - else the interfaces would conflict with each other. So by extending AccountInterface and EntityInterfaace UserInterfaces says that the methods match.

Status:Needs review» Needs work
Issue tags:+Entity system, +sprint, +Entity Access, +Entity Field API

The last submitted patch, account-interface-1825332-69.patch, failed testing.

Assigned:Unassigned» jrglasgow

Status:Needs work» Needs review
StatusFileSize
new36.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch account-interface-1825332-77.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

rerolled the patch

Assigned:jrglasgow» Unassigned

Status:Needs review» Needs work
Issue tags:-Entity system, -sprint, -Entity Access, -Entity Field API

The last submitted patch, account-interface-1825332-77.patch, failed testing.

Status:Needs work» Needs review

#77: account-interface-1825332-77.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Entity system, +sprint, +Entity Access, +Entity Field API

The last submitted patch, account-interface-1825332-77.patch, failed testing.

I'll get back to this after the user NG conversion is in, it's going to conflict heavily with that.

Status:Needs work» Needs review
Issue tags:+Field API NG blocker
StatusFileSize
new40.48 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

And here we go.

Title:Introduce an AccountInterface / UserInterface interface for use in \Drupal\Core namespacesIntroduce an AccountInterface to represent the current user

Retitling, UserInterface already exists and this isn't restricted to OO code.

Status:Needs review» Needs work

The last submitted patch, account-interface-1825332-83.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.7 KB
new40.88 KB
FAILED: [[SimpleTest]]: [MySQL] 54,911 pass(es), 330 fail(s), and 3,483 exception(s).
[ View ]

This should fix the installer.

Status:Needs review» Needs work

The last submitted patch, account-interface-1825332-86.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.57 KB
new48.45 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

More fixes.

Status:Needs review» Needs work

The last submitted patch, account-interface-1825332-88.patch, failed testing.

patch was just constantly being retested:/
i asked on irc and xjm cancelled

I've seen that before, with the same testbot I think. Going to re-test to try my luck with a different bot, I only made very small changes and the one before worked.

Status:Needs work» Needs review
Issue tags:-Entity system, -sprint, -Entity Access, -Entity Field API, -Field API NG blocker

#88: account-interface-1825332-88.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Entity system, +sprint, +Entity Access, +Entity Field API, +Field API NG blocker

The last submitted patch, account-interface-1825332-88.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new595 bytes
new48.46 KB
FAILED: [[SimpleTest]]: [MySQL] 56,792 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Ok, the patch was really bad. Let's try this.

Status:Needs review» Needs work

The last submitted patch, account-interface-1825332-94.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new568 bytes
new92.53 KB
PASSED: [[SimpleTest]]: [MySQL] 57,234 pass(es).
[ View ]

Fixed that test.

As discussed in irc it should be awesome if we had a "hasPermission" function that takes an array of permissions and return TRUE or FALSE.

Like the OOP version of user_access...

Status:Needs review» Needs work

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/Formula.phpundefined
@@ -68,7 +68,7 @@ public function query($group_by = FALSE) {
-    $this->query->addWhere(0, $formula, $placeholders, 'formula');
+    $this->query->add_where(0, $formula, $placeholders, 'formula');
   }

This patch is conflicting badly with view functions renames in seeveral places

Not just that, I forgot to do a rebase before I uploaded the new patch. Will not have time to fix that today, so if someone has a minute, just need the interdiff applied to the previous patch.

Absolutely agree with hasPermission(), but I think that's a follow-up, yes? :)

Note that hasPermission() does not render getRoles() unnecessary, there still places that use the role ids, locale cache, views roles access checker and so on.

I like this for the most part, but I think we can improve the design a bit.

The term "Account" only makes sense to us in the context of how we used global $user in the past. Outside of that, it's rather ambiguous. It might be nice to also use this as an opportunity to encapsulate user_access as aspilicious pointed out.

I might instead do something like this:

<?php
class Session {
  public function
__construct($values) {
   
$this->session = $session;
  }
  public function
id() {
  }
  public function
getHostname() {
  }
  public function
getData() {
  }
}
class
CurrentUser implements CurrentUserInterface {
  public static function
createFromSessionId($sid) {
   
// fetch values from db, create a session object to instantiate the appropriate
    // implementation of CurrentUserInterface
 
}
  public function
__construct(Session $session) {
   
$this->session = $session;
  }
  public function
isAnonymous() {
  }
  public function
getRoles() {
  }
  public function
getSession() {
    return
$this->session;
  }
  public function
hasPermission($perm) {
   
// most of the contents of user_access
 
}
}
class
AnonymousUser extends CurrentUser {
  public function
getRoles() {
    return array(
DRUPAL_ANONYMOUS_RID);
  }
  public function
isAnonymous() {
    return
TRUE;
  }
}
class
AuthenticatedUser extends CurrentUser {
  public function
__construct(Session $session, UserInterface $user) {
   
$this->session = $session;
   
$this->user_entity = $user;
  }
  public function
isAnonymous() {
    return
FALSE;
  }
  public function
getRoles() {
    return
$this->user_entity->getRoles();
  }
}
// There is probably a better name for this
class UserOne extends AuthenticatedUser {
  public function
hasPermission($perm) {
    return
TRUE;
  }
}
?>

There are probably details I didn't catch in there, but the basic design should be pretty clear.

This also may be too much of a refactor for this issue. I'm open to addressing it in a followup if so.

Status:Needs work» Needs review
StatusFileSize
new48.47 KB
PASSED: [[SimpleTest]]: [MySQL] 55,074 pass(es).
[ View ]

Yeah we need a followup for that.
Here is a reroll

I like @msonnabaum's ideas, but it seems a too big step at this point. For example, it's not clear to me how we'd deal with scenarios where we load a user entity and want to check it's permissions or something like that, which we do a lot and currently use the CurrentUser and a User entity as them being the same thing.

I'm not saying that's good, I'd love to clean that up but that seems like a big patch and a lot of changes, while this is relatively simple and describes how it currently works. Thanks to the BC decorator, a stdClass and a User entity can still be accessed in the same way but that will change and things will break badly. So we need a common interface between the two as long as we use them interchangeably, which I'm not sure we can change in 8.x.

For example, once this is in, and if the symfony session stuff gets in, we could easily switch the session related methods to a getSession() which returns a symfony session object.

Status:Needs review» Reviewed & tested by the community

ok, i am gonna try this again, i think many points have made, and it is very clear why this patch is needed

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -0,0 +1,115 @@
+ * @todo: Change all properties to protected.

Could we perhaps be a bit stronger with this statement, and state that access to the public properties is deprecated? That way if someone doesn't change their code to use the methods we can legitimately tell them "You're doing it wrong". :-)

Also, related: #1966334: Convert user_access to User::hasPermission()

That said, I like moving the access check onto the object directly.

This patch allows us to move all sorts of things to Drupal/Core which are currently hosted by the user module. (For example #1966334: Convert user_access to User::hasPermission() or TempStore which I already created an issue for at #2008884: Move TempStore (and eventually TempStoreFactory) to Drupal\Core and remove the user module dependency).

+1 RTBC

@Crell: I could make it more explicit but removing the BC layer for the user entity will *require* us to do that anyway. So we will :) And I will get started with that (Add more methods to UserInterface/AccountInterface as we need it, start converting code to them) as soon as this lands.

OK, works for me then.

Title:Introduce an AccountInterface to represent the current userChange notice: Introduce an AccountInterface to represent the current user
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed 4d85fa4 and pushed to 8.x. Thanks!

Awesome!

Status:Active» Needs review

Created https://drupal.org/node/2017231

Still a moving target, so I kept the change notice short. For example, that example makes it obvious that we should totally add the isAnonymous() method suggested by @msonnabaum. I also still hope that global $user moves to *something* else.

Title:Change notice: Introduce an AccountInterface to represent the current userIntroduce an AccountInterface to represent the current user
Priority:Critical» Normal
Status:Needs review» Fixed
Issue tags:-Needs change record

Made a couple of changes, i think its good enough now..we will hopefully revisit it

Awesome!

Issue tags:-sprint

Removing sprint tag. I'm working on the account and user interfaces in #2017207: [meta] Complete conversion of users to Entity Field API.

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