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

CommentFileSizeAuthor
#101 account-interface-1825332-101.patch48.47 KBaspilicious
#96 account-interface-1825332-96.patch92.53 KBBerdir
#96 account-interface-1825332-96-interdiff.txt568 bytesBerdir
#94 account-interface-1825332-94.patch48.46 KBBerdir
#94 account-interface-1825332-94-interdiff.txt595 bytesBerdir
#88 account-interface-1825332-88.patch48.45 KBBerdir
#88 account-interface-1825332-88-interdiff.txt7.57 KBBerdir
#86 account-interface-1825332-86.patch40.88 KBBerdir
#86 account-interface-1825332-86-interdiff.txt1.7 KBBerdir
#83 account-interface-1825332-83.patch40.48 KBBerdir
#77 account-interface-1825332-77.patch36.25 KBjrglasgow
#69 account-interface-1825332-69.patch37.13 KBBerdir
#69 account-interface-1825332-69-interdiff.txt1.72 KBBerdir
#67 drupal-1825332-67.patch37.13 KBdawehner
#67 interdiff.txt1.3 KBdawehner
#64 drupal-1825332-63.patch33.75 KBdawehner
#61 interdiff.txt1.02 KBandypost
#61 account-interface-1825332-61.patch37.06 KBandypost
#59 account-interface-1825332-59.patch36.03 KBandypost
#55 account-interface-1825332-55.patch51.8 KBBerdir
#55 account-interface-1825332-55-interdiff.txt4.03 KBBerdir
#53 account-interface-1825332-53.patch47.77 KBParisLiakos
#52 account-interface-1825332-52.patch48.02 KBParisLiakos
#43 no-interface-on-user-1825332-43.patch47.92 KBBerdir
#43 no-interface-on-user-1825332-43-interdiff.txt456 bytesBerdir
#37 account-interface-1825332-37.patch48.06 KBBerdir
#30 user-session-interface-1825332-30.patch48.49 KBParisLiakos
#30 interdiff.txt1.01 KBParisLiakos
#27 user-session-interface-1825332-27.patch48.49 KBBerdir
#27 user-session-interface-1825332-27-interdiff.txt9.59 KBBerdir
#21 user-session-interface-1825332-21.patch45.36 KBBerdir
#21 user-session-interface-1825332-21-interdiff.txt39.68 KBBerdir
#16 user-session-interface-poc-1825332-16.patch5.68 KBBerdir
#16 user-session-interface-poc-1825332-16-interdiff.txt4.27 KBBerdir
#14 user-session-interface-poc-1825332-13.patch3.13 KBBerdir
#13 user-session-interface-pox-1825332-13.patch0 bytesBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Component: other » base system
Issue tags: +Entity system, +Entity Access
plach’s picture

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.

fubhy’s picture

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: [P-1] Separate the account information from the user entity.
#1806514: Unify anonymous and registered users

plach’s picture

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

plach’s picture

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

fubhy’s picture

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

plach’s picture

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

fubhy’s picture

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.

dawehner’s picture

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
sun’s picture

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.

Berdir’s picture

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

Berdir’s picture

Status: Active » Needs review
Issue tags: +Entity Field API
FileSize
0 bytes

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.

Berdir’s picture

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

sun’s picture

+++ 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?

Berdir’s picture

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.

Berdir’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
39.68 KB
45.36 KB

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.

dawehner’s picture

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!

Dave Reid’s picture

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:

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.

Berdir’s picture

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

fubhy’s picture

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!

Berdir’s picture

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.

Berdir’s picture

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.

ParisLiakos’s picture

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

fubhy’s picture

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 ;).

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
48.49 KB

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

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

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.

Berdir’s picture

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.

ParisLiakos’s picture

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

Berdir’s picture

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

andypost’s picture

This one is really needed step to #1816218: [P-1] 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

Berdir’s picture

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

Dave Reid’s picture

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. :/

Berdir’s picture

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

webchick’s picture

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.

webchick’s picture

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.

Crell’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review

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

fago’s picture

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.

Berdir’s picture

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.

fago’s picture

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?

Berdir’s picture

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

fago’s picture

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

sun’s picture

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.

ParisLiakos’s picture

This is a reroll of #37

ParisLiakos’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.03 KB
51.8 KB

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?

Berdir’s picture

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.

andypost’s picture

patch needs re-roll

andypost’s picture

Status: Needs work » Needs review
FileSize
36.03 KB

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
37.06 KB
1.02 KB

Should be green now

dawehner’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
33.75 KB

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.

dawehner’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
37.13 KB

Forgot to add the AccounterInterface/UserSession object.

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.72 KB
37.13 KB

This should install again.

twistor’s picture

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?

Berdir’s picture

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?

fago’s picture

+++ 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?

fago’s picture

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.

benjifisher’s picture

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.

jrglasgow’s picture

Assigned: Unassigned » jrglasgow
jrglasgow’s picture

Status: Needs work » Needs review
FileSize
36.25 KB

rerolled the patch

jrglasgow’s picture

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.

Berdir’s picture

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.

Berdir’s picture

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

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Field API NG blocker
FileSize
40.48 KB

And here we go.

tim.plunkett’s picture

Title: Introduce an AccountInterface / UserInterface interface for use in \Drupal\Core namespaces » Introduce 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
40.88 KB

This should fix the installer.

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.57 KB
48.45 KB

More fixes.

Status: Needs review » Needs work

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

ParisLiakos’s picture

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

Berdir’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
595 bytes
48.46 KB

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
568 bytes
92.53 KB

Fixed that test.

aspilicious’s picture

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

aspilicious’s picture

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

Berdir’s picture

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.

msonnabaum’s picture

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:


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.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
48.47 KB

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

Berdir’s picture

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.

ParisLiakos’s picture

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

Crell’s picture

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

fubhy’s picture

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: Provide an interface for TempStore implementations).

+1 RTBC

Berdir’s picture

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

Crell’s picture

OK, works for me then.

alexpott’s picture

Title: Introduce an AccountInterface to represent the current user » Change 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!

fubhy’s picture

Awesome!

Berdir’s picture

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.

ParisLiakos’s picture

Title: Change notice: Introduce an AccountInterface to represent the current user » Introduce 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

andypost’s picture

Awesome!

Berdir’s picture

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.