Problem/Motivation

Currently, if a JSON object is posted with just the type and title set, it fails if comment module is turned on because comment is looking for $node->uid. It should be possible to post without explicitly stating who you are, since we already know that from your user.

Proposed resolution

Check whether the entity has a uid property, and if it does and it is not set, populate it with the requesting user's uid.

Related issues

#2078387: Add an EntityOwnerInterface

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

linclark’s picture

Status: Active » Needs review
FileSize
1.25 KB
effulgentsia’s picture

This makes sense to me, but are we comfortable committing it without test coverage? If not, is there test coverage that can be added here separately, or should this be merged into #1972116: Add REST test coverage for nodes?

effulgentsia’s picture

The one thing here that's a little unsettling is making the uid property have magic meaning based on its name. But I don't have a better idea at the moment.

effulgentsia’s picture

Issue tags: +Entity Field API

Tagging in case anyone who's been working on Entity(NG) Field API has ideas on this.

webchick’s picture

Issue tags: +Needs tests

Agreed we need tests for this.

klausi’s picture

I think this is wrong because we should not special-case any entity field in REST module at all. The problem is really in the entity API, consider the following snippet:

$values = array('title' => 'test node', 'type' => 'article');
$node = entity_create('node', $values);
$node->save();

This will fail on a Drupal standard installation profile with an exception that the uid is not set for some node_comment_statistics DB table insert.

I see two ways to fix this:
1) make entity_create() for nodes self sufficient, i.e. node module populates the uid if we think this is an essential field that should never be NULL.
2) Fix the code that does the node_comment_statistics insert to be more robust in case there is no author present.

I personally favor 2) especially in this case: on a new node there are no comments, so the DB record is initialized with a bogus value for last_comment_uid anyway.

effulgentsia’s picture

make entity_create() for nodes self sufficient

I agree with that, but that's a separate issue. For example, yes, entity_create() should end up defaulting uid to 0 if it's not otherwise specified, or we should make NULL a valid value. But when REST module calls it, it knows that there's actually a real logged in user initiating the request, so I think this issue is a good place to figure out how it can pass along that info in a clean way.

klausi’s picture

Related issue to initialize default values on entity creation: #1777956: Provide a way to define default values for entity fields.

Crell’s picture

#1: 1979260-01-populate-uid.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Needs work

We could introduce an author_field or so, as a sub-type of entity_reference_field, that would provide an applyDefaultValue() implementation that would set it to the current user. That would then work all the time, everywhere. If that's what we want.

We've also thought about an EntityAuthorInterface in the comment as field issue, to be able to identify entity types that have an author/owner, with methods like getAuthor()/getAuthorId()/setAuthorId(). That wouldn't do anything automatically, but then rest.module could support all entity types that implement it, without having to special case nodes or a uid property.

comment.module would be happy with either solution, it could check for the interface or it could look for an author_field field.

The current snippet is wrong because in a non-BC-decorator entity, ->uid is always set if the property exists (would need to be $entity->uid->isEmpty()). It's also broken because users have a uid too, but there it's actually the id, obviously, we do *not* want to set that to the current user ;)

Happy to guide anyone who's interested in working on that..

larowlan’s picture

altrugon’s picture

FileSize
132.92 KB

Hello,

I've been playing with D8 and REST and I also run into the problem that it is basically impossible to set the author of a node using a POST request.

I took a look to #2078387: Add an EntityOwnerInterface but in their last patch (#60 at the moment I wrote this) there is no reference to the rest module at all, what make me think that this author issue is still active for POST request.

I agree that assuming the author of a node when no uid is specified can lead us into problem so perhaps the best way to do this would be to follow the same method that is applied when you create the node through the UI:

  1. By default the author of the node is the logged user that is creating the node
  2. If the user has the permission to access the Authoring Information panel, then read the "uid" specified on the POST request (if not uid specified then use option #1)

Attached there is a screen shot of my successful post request, please note that the uid field is completely ignored.

linclark’s picture

Thanks for taking a look. What your discussing is slightly different than this issue and would be a bug.

If you have permission to set the author property, then you should be able to POST an entity and set the author field. The specified value should be used even if it would overwrite the automatically populated value which this issue addresses.

Could you post a new issue to report this bug and post the link to it back here? Thanks!

effulgentsia’s picture

Hi altrugon,

Looks like in your screenshot, you're posting via HAL. HAL is a hypermedia format, so references to resources must be specified in _links or _embedded. If you do a GET request for the node with HAL, you should see the author's UUID in the _embedded section. You should be able to POST it the same way.

Note that there's #1892320: Add deserialize for JSON/AJAX for allowing non-HAL JSON posts, but that's not in yet.

linclark’s picture

While it is true that the HAL isn't correct, I'm pretty sure that this should still work. That is to say, we don't yet limit entity references to being in the _links/_embedded section on import.

I believe this is likely a bug.

effulgentsia’s picture

we don't yet limit entity references to being in the _links/_embedded section on import

Perhaps not (though we should), but we do limit it to a property named 'uuid' (rather than 'target_id'), and since that is not an actual property of ER fields, it should really be submitted within _embedded rather than in the regular field payload where there's an expectation of subkeys corresponding to real properties.

linclark’s picture

Good point, the EntityResolver doesn't know how to handle internal IDs.

Either way, this is unrelated to the issue. altrugon, if you would like help figuring out how to use HAL to POST, please post a support request and link it here.

altrugon’s picture

Thank you so much for the quick reply guys.

As you can see on the screen-shot I also tried posting via JSON but wasn't able to get anything done. There is little information so far and the only documentation that I have found was https://drupal.org/node/2098511 last updated by linclark. I did also check several Test modules but wasn't able to find a sample where the uid was specified.

I'm ok on putting more time into this issue but I could use some help/instruction about how to properly construct the object that I have to send to Drupal, I was able to figure out the body by myself but no luck so far with the author.

effulgentsia I only have one user on this site and the uuid is NULL, what make me think that sending a uuid for my user via POST is not going to work either.

linclark’s picture

Please post a new issue using support request as the category.

altrugon’s picture

linclark here is the bug report #2113681: Node author can't be set when posting via HAL.

Thanks again to both of you.

altrugon’s picture

Issue summary: View changes

Updated issue summary.

anavarre’s picture

klausi’s picture

Status: Needs work » Needs review
FileSize
5.65 KB

klausi opened a new pull request for this issue.

klausi’s picture

Title: Automatically populate uid on POST requests » Automatically populate the author default value with the current user

So actually we can do this on the field API default value level. We are already populating the node author with the current user for node forms, so we can also do that on the API level.

TODO:
* remove default populating code in node form submissions
* add a test case to cover the weird behavior of ConfigurableEntityReferenceFieldItemList which tries to convert UUIDs.

fago’s picture

Status: Needs review » Needs work

+ protected $currentUser;
I'm not sure that makes sense? Why not just always use \Drupal::currentUser().

-    $fields['uid'] = FieldDefinition::create('entity_reference')
+    $fields['uid'] = AuthorFieldDefinition::create('entity_reference')

Should make entity reference baked in and have create() throw a LogicException if a different field type is passed.

+ ->setSetting('target_type', 'user');
Should be default for the class, maybe set in create() as well.

AuthorFieldDefinition should be OwnerFieldDefinition, or maybe even OwnerReferenceFieldDefinition to make clear it's an entity reference?

klausi’s picture

Status: Needs work » Needs review
FileSize
8.69 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Component: rest.module » entity system

Implemented fago's comments, added a test case to cover ConfigurableEntityReferenceFieldItemList when entity_reference module is enabled. Interdiff: https://github.com/klausi/drupal/commit/a32cd1d3fb39107305bf30cf50f8abf6...

yched’s picture

Not too hot on an standalone AuthorFieldDefinition. Specialized field definition classes is something we avoided so far, and turned down as a pattern in #2145115: Improve DX creating field definitions.

  1. +++ b/core/lib/Drupal/Core/Field/AuthorFieldDefinition.php
    @@ -0,0 +1,35 @@
    +  public function getDefaultValue(EntityInterface $entity) {
    +    if (!isset($this->currentUser)) {
    +      $this->currentUser = \Drupal::currentUser();
    +    }
    +    return $this->currentUser->id();
    +  }
    

    Couldn't this be implemented using a dedicated FieldItemList class instead ? All per-field one-off behaviors are done that way currently, rather than by adding a new definition class.

  2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldType/ConfigurableEntityReferenceFieldItemList.php
    @@ -20,7 +20,9 @@ class ConfigurableEntityReferenceFieldItemList extends FieldItemList {
    +    // Only convert UUIDs if there is at least one actual target_uuid default
    +    // value specified.
    +    if ($default_value && isset($default_value[0]['target_uuid'])) {
    

    Checking on delta 0 sounds a bit fragile. Cannot look at code right now but I'd think that this can be fixed by more bullet-proofing of the arrays / loops in the code below (like, exit if we didn't find any target_uuid)

fago’s picture

Not too hot on an standalone AuthorFieldDefinition.

Yep, discussed this and the relationship to #2145115: Improve DX creating field definitions. Different to #2145115: Improve DX creating field definitions this is not a requirement - if you miss it and/or do not know about this class you can still create entity references. So I don't see the problem of "having to know" about it being problematic in this case - you just have to know class FieldDefinition which you extend and adapt for your case/instance.

But on the plus side the default value (or possibly also custom allowed values) are metadata, and thus do fit on the field definition. So if it's per-field instance metadata I think it makes sense to put it in the definition class, but if it's per field type metadata I'd still agree that we should keep all the logic and per field type variations in the item class.

As an alternative we'd have to do custom field item classes, but that gets problematic/confusing here as the entity reference class is swapped out by the module. Then we'd end up with a mix of swapped out and not swapped out entity reference item classes by default :/

Patch looks good besides of that minor remarks:

  1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeOwnerDefaultTest.php
    @@ -0,0 +1,54 @@
    +      'name' => 'Node owner default',
    

    Minor: Node terminology is author, so I'd suggest "Node author default value" here.

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeOwnerDefaultTest.php
    @@ -0,0 +1,54 @@
    +   * Tests that the current user is set as author when entity_reference module
    +   * is enabled.
    

    Minor: Comment too long.

yched’s picture

Half convinced :-/.
So each time a base field will need a computed default value, it will need to add a new dedicated definition class ?

IMO a better approach would be to allow callbacks for dynamic default values (configurable fields allow this already).

yched’s picture

(also - yeah, e_r.module switching different Item and ItemList classes is really a nuisance IMO, we should try to get rid of that. Different issue though)

klausi’s picture

FileSize
9.01 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

fago’s picture

Status: Needs review » Needs work

The last submitted patch, 31: 1979260.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
13.59 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

FileSize
14.19 KB

The last submitted patch, 34: 1979260.patch, failed testing.

klausi’s picture

FileSize
15.08 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 37: 1979260.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
15.13 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Berdir queued 39: 1979260.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 39: 1979260.patch, failed testing.

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
13.75 KB

Rerolled patch.

Status: Needs review » Needs work

The last submitted patch, 43: drupal8-1979260-43.patch, failed testing.

Berdir’s picture

Issue tags: -Needs tests
+++ b/core/modules/quickedit/src/Tests/EditorSelectionTest.php
@@ -34,7 +34,9 @@ class EditorSelectionTest extends QuickEditTestBase {
+    $this->installSchema('user', array('users'));

Those need installEntitySchema('user') now.

Based on #2226267: Improve default value handling of fields to be consistent, I think we should not have a custom FieldDefinition class but just allow to store a special value like CURRENT_USER or a constant that we can then process in the field item (list?) class.

yched’s picture

I think we should not have a custom FieldDefinition class

agreed :-)

but just allow to store a special value like CURRENT_USER or a constant that we can then process in the field item (list?) class.

Not sure how this could be formalized, though ?

In another issue (I thought it was here, but apparently not), I suggested adding a FieldDefinitionInterface::setDefaultValueProvider($class), where $class implements DefaultValueProviderInterface, whose getDefaultValues() method returns the default values.

That is, the "allowed values callback" that FieldInstanceConfig offers, but overhauled for an OO era (which allows us to better document what the callback expects)

Berdir’s picture

Not as fancy, but #2226493: Apply formatters and widgets to Node base fields added a "->setDefaultValueCallback(array('Drupal\node\Entity\Node', 'getCurrentUserId'))" to the node uid field, and I'm also adding that for comments in the corresponding comment issue.

Which IMHO means that this issue is pretty much resolved, we might want to add it to other entity types too, the only other non-test implementation one core seems to be file?

RoSk0’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
5.43 KB

Added setDefaultValueCallback() to comment and file.
Moved getCurrentUserId() to ContentEntityBase class.
Created test for node only just to see how it works.

Status: Needs review » Needs work

The last submitted patch, 48: drupal8-1979260-48.patch, failed testing.

RoSk0’s picture

OK, I suppose that fix for EditorFileReferenceFilterTest and TypedDataTest would be adding $this->installEntitySchema('user'); to setUp() method of the test. Am I write?
Also NodeFieldOverridesTest needs to be fixed.
But why is the tests only patch is green?!?

RoSk0’s picture

FileSize
7.02 KB
2.55 KB

Fixed tests, removed duplicated getCurrentUserId() from Comment class.

RoSk0’s picture

Status: Needs work » Needs review

For bots.

Berdir’s picture

The tests pass because this was already fixed and you're just applying the same pattern for other entity types. So we should have test coverage for them as well. Instead of adding a ton of tests, I wonder if we can't just extend some tests with a few assertions, as most entity types should already have some tests that create entities.

anavarre’s picture

Status: Needs review » Needs work

Per #53

andypost’s picture

Looks no more valid cos we init with 0 now

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

slashrsm’s picture

In CRM core we're implementing a trait that implements EntityOwnerInterface methods and provides a helper method that provides base field definition.

We think it would make sense to move that in core to solve this problem.

See #2713131: Use default value callback for "uid" base field on all entities and #2714423: Implement EntityOwnerInterface function on EntityOwnerTrait for more information.

slashrsm’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2.14 KB
RoSk0’s picture

  1. +++ b/core/modules/user/src/EntityOwnerTrait.php
    @@ -0,0 +1,87 @@
    +      ->setDefaultValueCallback('Drupal\user\EntityOwnerTrait::getDefaultAuthorId');
    

    Should this be getDefaultOwnerId?

  2. +++ b/core/modules/user/src/EntityOwnerTrait.php
    @@ -0,0 +1,87 @@
    +  public static function getDefaultAuthorId() {
    

    Same as above?

fago’s picture

Status: Needs review » Needs work

Yeah, a trait would be nice here.

>Should this be getDefaultOwnerId?
Agreed.

Also, this needs tests - so back to nw.

tstoeckler’s picture

1. Calling the field uid is very D6/D7 IMHO, I like to call the field owner on my entities. At the very least the field name should be kept in a property so it is overridable.

2. To be in line with e.g. revisionLogBaseFieldDefinitions() we should IMO use a plural method name and return an array (i.e. including the field name) directly here, even though it's only 1 field.

3. AFAIK default value callbacks have to return the value of the field item list, i.e. in this case an array of user IDs. At least that is what Node does ATM.

fago’s picture

>1. Calling the field uid is very D6/D7 IMHO, I like to call the field owner on my entities. At the very least the field name should be kept in a property so it is overridable.

Totally. I think the field should be owner by default to match the interface, while it should be overridable to match the entity type lingo - e.g. for nodes it should be just "author" imo.

Berdir’s picture

1. Fine with a property, but every entity in core still uses uid, so not sure if we should default to something else. Also, you can't just define a property and override it in the class: https://3v4l.org/qYs5E. So we'd need a method that returns it I guess? Using entity keys for it has the annoying side-effect of making the schema not null.

2. Per #2713545: [policy, no patch] Are changes to ContentEntityBase base field definitions API breaking, we can not ever make changes to that method anyway, so not sure we benefit from making it multiple but I don't really care. Single return made it IMHO easier to extend with e.g. a custom description.

3. It accepts anything that $field->setValue() accepts..\Drupal\Core\Field\FieldItemList::applyDefaultValue(). We use this in crm_core and it works fine for us.

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.44 KB

Started using the trait, not quite sure yet what makes sense for the default settings, e.g. revisionable/translatable. The problem is that e.g. making it revisionable/translatable by default also affects entity types that aren't actually translatable/revisionable. Would be nice to just have defaults that don't do anything if they entity type doesn't use those things.

Plan to also use it for the test entity types, which have user_id, so we can make it dynamic there.

Status: Needs review » Needs work

The last submitted patch, 64: 1979260-64.patch, failed testing.

Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review
Issue tags: +Workflow Initiative
FileSize
2.82 KB
8.48 KB

Re-roll, plus moving things from Node to EditorialContentEntityBase.

Status: Needs review » Needs work

The last submitted patch, 69: 1979260-69.patch, failed testing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

AaronMcHale’s picture

From looking at the latest patch it looks like this is no longer relevant since #2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface is now in 8.7.