Add the ability to find uid in users_data with $module, $key, or $value to user.data service

The value might be unique for your module, but there's nothing in the API/storage that guarantees that is for all values, so that special case for returning a single uid doesn't really make sense in the generic implementation?

Re-rolled patch. Please, review.

Corrected comments to correspond that the get() method will return.

Poor get method already does N different things, are we sure we want to pile on more?

Tests.

The new doxygen is 86 long, needs to be broken into two lines.

Reroll for 8.2.x

This patch is a completely ground up attempt -- not only I am not using the previous patches but I have rewritten UserData::get from the ground up, it's 5 lines of code shorter but at the same it is much more powerful -- allows for $value as this issue asks and also happens to cover every possible combination. I really like how the getDataProvider turned out, it reads like terse documentation (of course the interface doxygen has more):

      [$contents, $uid, $name, NULL, $value],
      [$contents, $uid, NULL, $value, [$name]],
      [$contents, NULL, $name, $value, [$uid]],
      [$contents, $uid, NULL, NULL, [$name => $value]],
      [$contents, NULL, $name, NULL, [$uid => $value]],
      [$contents, NULL, NULL, $value, [$uid => $name]],
      [$contents, NULL, NULL, NULL, [$uid => [$name => $value]]],

In answering my own problems with unit tests like this: it is a meaningful test, proven by a) I found a number of bugs with it b) you can change the implementation easily (but not the query) without the test breaking. There is, of course, more that could be done (multiple rows) but this seems to be enough -- for me.

A little simplification in the setup.

Testing non-scalar values. Of course there were bugs handling them. This begins to give at least a taste of just how massive testing this class ought to be.

This function is pretty overloaded, It would be good to add follow-up issues that added new methods for each possible return value, and then keep get() for BC.

However the current tests are passing and the new tests are welcome to such complicated code

Just one nitpick.

+++ b/core/modules/user/src/UserDataInterface.php
@@ -16,18 +16,26 @@
+   * @see \Drupal\Tests\user\Unit\UserDataTest::getDataProvider() ¶

Trailing whitespace.

Fixed the whitespace.

+++ b/core/modules/user/src/UserData.php
@@ -29,49 +29,46 @@ public function __construct(Connection $connection) {
+  public function get($module, $uid = NULL, $name = NULL, $value = NULL) {
+    $args = ['uid' => $uid, 'name' => $name, 'value' => $value];
+    if (!$args_not_set = array_filter($args, 'is_null')) {
+      throw new \InvalidArgumentException('All four arguments can not be set at the same time.');
+    }

I disagree, I think this adds way too much complexity and isn't the intention of the existing method.

The original method is for getting a value with a known module and possibly uid.

Now we're doing the reverse.

I think we need to take a step back and think about how we'd design this in a greenfield scenario.

We'd add a getUidGivenValue method.

But we can't do that here because BC.

So I propose we add a new service that extends the existing one - eg. user_data.reverse and mark the existing one deprecated to be removed in 9.0

then we don't overload the existing method with complexity

Sure. Here's a simple query service.

Pointless doxygen fixes.

A little bit of test simplification. Trying to keep the size down.

I like the service a lot. I do think it is a great addition, my review is only nit picks and one strategic decision (shared: FALSE).

Setting to RTBC to get D8 core committer feedback.

----

1. +++ b/core/modules/user/src/UserDataQueryInterface.php
   @@ -0,0 +1,56 @@
   +   * ¶
   ...
   +   * ¶
   ...
   +   * ¶
   ...
   +   * ¶
   

   nit - whitespace

2. +++ b/core/modules/user/src/UserDataQueryInterface.php
   @@ -0,0 +1,56 @@
   +   *   A list of result objects. Each object contains has module, uid, name
   +   *   and value properties.
   

   nit - remove contains

3. +++ b/core/modules/user/user.services.yml
   @@ -25,6 +25,12 @@ services:
   +    shared: FALSE
   

   This would be the first core service to use shared: FALSE (and should be lowercase FALSE). (The other ones are from zen feed reader)

   I do think we use factories for that in core so far. I don't think we should deviate from that pattern as it is more explicit.

   However I can see the argument that the factory would in essence do nothing and be just 'dead code'.

   However as it is a kinda new paradigm for core at this stage, this definitely needs a framework manager review.

   I am not opposed to it, just think that factories are clearer, but shared: FALSE is there for a reason, too.

Nits fixed. All I can say about shared: false is that I was glad when I found it. Yes, it's a new pattern but Drupal 8 definitely could use less boilerplate.

Unrelated fail, retested OK so going back to green.

This was a random test failure.

More of the same.

1. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,125 @@
   +      if (!is_scalar($this->value)) {
   +        $this->value = serialize($this->value);
   +        $query->condition('serialized', 1);
   +      }
   ...
   +      if ($row->serialized) {
   +        $row->value = unserialize($row->value);
   +      }
   

   Not tested logic.

2. +++ b/core/modules/user/user.services.yml
   @@ -25,6 +25,12 @@ services:
   +    shared: false
   

   I'm fine with this. It's a feature of our service container - seems fine to use it. We have specific code for it in \Drupal\Component\DependencyInjection\Container.

I rewrote the whole class. I liked this one so much better: the class only talks to the select object not the entire connection. The select object carries the state, not the query class. It's extremely small this way. At this point interdiff is pointless.

Well #36 looks really good but #37 is more inline with query service concept so RTBC for #37.

Tagging for visibility.

+++ b/core/modules/user/src/UserDataQuery.php
@@ -0,0 +1,89 @@
+  /**
+   * Constructs a new user data service.
+   *
...
+  public function __construct(SelectInterface $select) {
...
+  /**
+   * Factory method for the class.
+   *
...
+  public static function create(Connection $connection) {

Question: What do we gain when using a separate factory method here?

It seems the 'ud' is hard-coded in both, so I imagine any special custom code would need to override both as well.

Storing the select instead of the connection looks good to me, but I don't see the need for the factory ...

Can someone explain, please?

I don't see any mention of https://www.drupal.org/project/externalauth in this issue.

Also I think I've seen a core issue to bring some of that into core, but wasn't able to find it again.

We were trying to remove users_data in Drupal 8, but didn't manage to do it, not sure about making it easier to use - could equally go for trying to deprecate it.

Re #41 if you override the constructor you can use this class for anything that support the simplest form of condition method and execute. Nothing presumes a SQL database select really. That was my evil plan with this. We could equally pass in a connection to the constructor and retrieve the select there but running logic in the constructor felt wrong. All in all, the class cares about a select so let's pass it a select. You can't do that from the services.yml alone so we needed something to go for it.

Re externelauth we kill Authmap from that project, yes, I am aware and glad we do. We could try to deprecate user data sure but ... it's here for now and Authmap hardwires SQL while userdata being in core and backend overrideable and all that, doesn't really. Just my two cents. I'd say keep it but change UserData::get into a wrapper for the new query class because it's reasonably horrible spaghetti and then the maintenance cost is very low for keeping it.

Back to RTBC, I agree that keeping users_data is a good idea.

The advantage to K/V is that it can have conditions, which is sometimes needed for user specific data.

And just using fields was sometimes a little brittle for users, when what you need is not needed on every entity load of the user.

So I think its a pretty good compromise.

I could accept won't fixing this on the condition of having a "save this field only" mechanism. There are other use cases (denormalizing flag or comment counts) for that. Currently if this were in a field you'd lose the meaning of user->changed as the entity would change every time their oauth token expires... until that (very hard) issue this simple one needs to suffice for user.

So whilst reviewing this again I decided to look at \Drupal\user\UserData and I read the ::get() method. That method is not a limited version of this service therefore it's confusing to know which one you want. So one option would be to deprecate it in favour of this new service. But the other thing that strikes me is that if you are going to override user.data or user.data.query using the backend_overridable tag then you've have to do both - which seems OTT. Shouldn't the user.data service provide this ability and then you only got once place to override?

Perhaps the simplest way to address #46 is just to have one service.

1. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,89 @@
   +class UserDataQuery implements UserDataQueryInterface {
   
   +++ b/core/modules/user/tests/src/Kernel/UserDataQueryTest.php
   @@ -0,0 +1,95 @@
   + * @coversDefaultClass \Drupal\user\UserData
   

   Mismatch

2. +++ b/core/modules/user/src/UserDataQueryInterface.php
   @@ -0,0 +1,56 @@
   +namespace Drupal\user;
   +
   +interface UserDataQueryInterface {
   

   This deserves some docs.

3. +++ b/core/modules/user/src/UserDataQueryInterface.php
   @@ -0,0 +1,56 @@
   +   * Set the module value.
   ...
   +   * Set the user account ID the data is associated with.
   ...
   +   * Set the name of the data key.
   ...
   +   * Set the value stored.
   ...
   +   * Run the query.
   

   All of these should be "Sets" and then "Runs"

4. +++ b/core/modules/user/user.services.yml
   @@ -25,6 +25,13 @@ services:
   +    class: Drupal\user\UserDataQuery
   +    shared: false
   +    tags:
   +      - { name: backend_overridable }
   +    factory: ['Drupal\user\UserDataQuery', 'create']
   +    arguments: ['@database']
   

   Super nitpicky, but could these be reordered? class, factory, arguments first, and then shared/tags.

   Also is this the first service we use the class itself for a static factory method? Interesting to me since it mirrors plugins and ContainerInjectionInterface, but slicker.

5. +++ b/core/modules/user/user.services.yml
   @@ -25,6 +25,13 @@ services:
   +    shared: false
   

   Ooh, first time I've seen this. Seems we use it for Zend feed plugins. http://symfony.com/doc/current/service_container/shared.html says it gives us a new instance, and yeah that seems super important in this case.

   Seems like it'd be worth a kernel test that fails if that line is removed?

Also I just read #46 and now I'm equally confused.

EDIT: crossposted with #47, that works for me, and removes the need for the extra test coverage I requested.

Addressing @tim.plunkett's review.

Thanks, that's an interesting direction but you no longer need create in this version I beleive.

Note however this will break any modules implementing UserDataInterface . I guess that's better than subtly breaking for not having the service. Those modules will be rare, no doubt.

Removed create, no other changes.

Ops, I left in a stray use statement. Fixed. Pity __call can't satisfy an interface the whole class could be collapsed into a single method call. Interdiff to 49.

1. +++ b/core/modules/user/src/UserData.php
   @@ -77,6 +77,13 @@ public function get($module, $uid = NULL, $name = NULL) {
   +    return new UserDataQuery($this->connection->select('users_data', 'ud'));
   

   Shouldn't the UserDataQuery be a service instead of creating it here?

2. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,78 @@
   +    $this->select->condition('module', $module);
   

   I believe this class can never return any values if multiple module conditions have been set. Therefore the method should check if a module condition has been set already and throw an exception OR it should add the condition as an OR condition.

3. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,78 @@
   +    $this->select->condition('uid', $uid);
   

   This should also check if a condition has already been set or add it as an OR condition.

4. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,78 @@
   +    $this->select->condition('name', $name);
   

   Same here

5. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,78 @@
   +    if (!is_scalar($value)) {
   

   And here

6. +++ b/core/modules/user/src/UserDataQueryInterface.php
   @@ -0,0 +1,61 @@
   +  public function module($module);
   

   The name 'module' does not describe what this method does. Shouldn't this be setModuleValue or setModuleCondition?

7. +++ b/core/modules/user/src/UserDataQueryInterface.php
   @@ -0,0 +1,61 @@
   +  public function uid($uid);
   

   The name 'uid' does not describe what this method does. Shouldn't this be setUidValue or setUidCondition?

8. +++ b/core/modules/user/src/UserDataQueryInterface.php
   @@ -0,0 +1,61 @@
   +  public function name($name);
   

   The name 'name' does not describe what this method does. Shouldn't this be setDataKeyName or setDataKeyNameCondition?

9. +++ b/core/modules/user/src/UserDataQueryInterface.php
   @@ -0,0 +1,61 @@
   +  public function value($value);
   

   The name 'value' does not describe what this method does. Shouldn't this be setValue or setValueCondition?

10. +++ b/core/modules/user/src/UserDataQueryInterface.php
    @@ -0,0 +1,61 @@
    +  public function execute();
    

    This violates the single responsibility principe. The method both executes a query and returns it's result.

    I suggest adding a getResults() method.

> Shouldn't the UserDataQuery be a service instead of creating it here?

You are of course aware that the exact opposite was done by a core committer in #47.

> The method both executes a query and returns it's result.

So does db_select()->execute().

As for the rest of the requests, I will not overengineer a small class like this. Simply ->module() won't be called twice or if it is, their problem. We didn't build such safety blocks into db_select() or EQ although certainly we could've.

https://medium.com/@chx/the-terrible-cycle-of-contributing-to-drupal-cor... 4. is very, very relevant.

Re #54 well I think the points about multiple calls to ->module etc.. are valid.
I think #54.1 came from the IS which I had failed to update - my fault.
I agree with @chx about #54.10 - I think execute returning a traversable is just fine.

Patch incoming.

Yep this behaves differently from a db query but this has a slightly different aim. It's meant to be a something that retrieves data from the user.data service based on a limited set of conditions you supply it. Seems elegant to make it work regardless of how often setFoo() is called.

Unfollowed. I never unfollowed a work patch before but there's a first for everything.

So @chx didn't like the move away from querybuilder-lite, however the points @legolasbo raises in #54 about calling the condition setting methods more than once are valid. I think the behaviour of #53 is unexpected. The patch in #58 solves this but it opens another problem how to query for NULL values. After some discussion in IRC I think the best way forward is to use an exception since setting the same condition twice is never the right thing to do with this simple query service.

1. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -10,35 +11,14 @@
   +  protected $sets;
   

   Since this is an array, why not initialize it as an empty array?

2. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,109 @@
   +  /**
   +   * Stores which conditions have been set.
   +   *
   +   * @var array
   +   */
   +  protected $sets;
   

   Let's initialize this array

3. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,109 @@
   +    $this->setter('module', $module);
   +    return $this;
   

   If we make setter return $this then these setters can be simplified to return $this->setter(...);

4. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,109 @@
   +    $is_scalar_value = is_scalar($value);
   

   Nit: $isScalarValue

5. +++ b/core/modules/user/tests/src/Kernel/UserDataQueryTest.php
   @@ -0,0 +1,179 @@
   +  /**
   +   * Data provider for testUserDataQuery.
   +   *
   +   * @return array
   +   */
   +  public function userDataQueryProvider() {
   ...
   +  /**
   +   * Data provider for testUserDataQueryMultipleMethodCalls.
   +   *
   +   * @return array
   +   */
   +  public function userDataQueryMultipleMethodCallsProvider() {
   

   Let's put these utility methods at the top of the class. Also, they should be 'private' or 'protected'

6. Nitpick: The tests don't test if the setter methods return self.

The findings above are no deal breakers however. Would be nice to see them changed but RTBC anyway.

Re #61
1. Because that is not necessary.
2. Because that is not necessary.
3. Because reading return $this; is very simple. As a dev you don't have to think whilst return $this->setter() you have to work out what that returns.
4. Not required and not yet implemented in core.
5. There is no standard for order and nope they shouldn't be private or protected.
6. See 3

We can add some implicit coverage of the chaining very simply.

About 5. I know there is no standard for order, but grouping tests and utility methods improves readability of the code which is why is suggested it.

@legolasbo but there is an order - the providers come immediately after the test method that they provide for. This order is almost standard in core tests.

So the \Drupal\Tests\user\Kernel\UserDataQueryTest::testSetValue was not quite testing what I hoped for. After making it test the adding of the serialized condition properly I realised that the addition of this condition seems unnecessary. Since if you already have a serialized value why would you want to unserialize it to pass it to setValue to serialize it again.

Better names for things.

+++ b/core/modules/user/tests/src/Kernel/UserDataQueryTest.php
@@ -0,0 +1,189 @@
+    // and re-running the query does not break is the value is not scalar.

Typo: ..if the value is not scalar.

Other than that I have nothing else to add to this patch.

RTBC + 1, I like how the original service and the new query class are combined.

Fixing #71

We need a change record for this.

And also this is broken because serialized condition is not set so if someone demented enough stored b:0; as a string value and then queried for FALSE then you'd get the former as well.

@chx good point. It seems a shame to not cope with the case where you already have a serialized value. I can easy imagine that happening.

Fortunately we can have the best of both worlds.

Seriously it's going overboard the only thing I was missing is a $this->select->condition('serialized', 1); noone will want (or should) use a setSerializedValue. If we want that method, make it protected please.

I guess NW for #78.

1. +++ b/core/modules/user/src/UserDataInterface.php
   @@ -32,6 +32,15 @@
   +  public function getUserDataQuery();
   

   Adding a new method to an existing interface is a BC break, we need to add a new interface, and have UserData implement that too.

2. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,120 @@
   +  protected $existingConditions;
   

   this should be initialized to an empty array

3. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,120 @@
   +  public function setSerializedValue($value) {
   

   Is there a reason why this is public?

4. +++ b/core/modules/user/src/UserDataQuery.php
   @@ -0,0 +1,120 @@
   +    if (isset($this->existingConditions[$name]) && $this->existingConditions[$name]) {
   

   !empty?

Just nitpicking: Since $is_scalar_valueis used just once, is_scalar() can be used directly inside the if() statement.

+  public function setValue($value) {
+    $is_scalar_value = is_scalar($value);
+    if (!$is_scalar_value) {

Also, it is sufficient to use !empty($this->existingConditions[$name]), instead of isset($this->existingConditions[$name]) && $this->existingConditions[$name].

+  protected function createCondition($name, $value) {
+    if (isset($this->existingConditions[$name]) && $this->existingConditions[$name]) {
+      throw new ExistingConditionException("Can not add another condition on '$name'");
+    }

I would instead use the following code.

  /**
   * {@inheritdoc}
   */
  public function setValue($value) {
    $serialize = !is_scalar($value);
    if (serialize) {
      $value = serialize($value);
    }
    $this->setSerializedValue($value, $serialize);
    return $this;
  }

  /**
   * {@inheritdoc}
   */
  protected function setSerializedValue($value, $serialized = TRUE) {
    $this->createCondition('value', $value);
    $this->select->condition('serialized', (int) $serialized);
    return $this;
  }