Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#69 | 1979260-69.patch | 8.48 KB | timmillwood |
#69 | interdiff-1979260-69.txt | 2.82 KB | timmillwood |
Comments
Comment #1
linclark CreditAttribution: linclark commentedComment #2
effulgentsia CreditAttribution: effulgentsia commentedThis 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?
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedThe 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.Comment #4
effulgentsia CreditAttribution: effulgentsia commentedTagging in case anyone who's been working on Entity(NG) Field API has ideas on this.
Comment #5
webchickAgreed we need tests for this.
Comment #6
klausiI 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:
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.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #8
klausiRelated issue to initialize default values on entity creation: #1777956: Provide a way to define default values for entity fields.
Comment #9
Crell CreditAttribution: Crell commented#1: 1979260-01-populate-uid.patch queued for re-testing.
Comment #10
BerdirWe 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..
Comment #11
larowlanAdded #2078387: Add an EntityOwnerInterface
Comment #12
altrugon CreditAttribution: altrugon commentedHello,
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:
Attached there is a screen shot of my successful post request, please note that the uid field is completely ignored.
Comment #13
linclark CreditAttribution: linclark commentedThanks 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!
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedHi 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.
Comment #15
linclark CreditAttribution: linclark commentedWhile 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.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedPerhaps 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.
Comment #17
linclark CreditAttribution: linclark commentedGood 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.
Comment #18
altrugon CreditAttribution: altrugon commentedThank 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.
Comment #19
linclark CreditAttribution: linclark commentedPlease post a new issue using support request as the category.
Comment #20
altrugon CreditAttribution: altrugon commentedlinclark here is the bug report #2113681: Node author can't be set when posting via HAL.
Thanks again to both of you.
Comment #20.0
altrugon CreditAttribution: altrugon commentedUpdated issue summary.
Comment #21
anavarreComment #22
klausiklausi opened a new pull request for this issue.
Comment #23
klausiSo 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.
Comment #24
fago+ protected $currentUser;
I'm not sure that makes sense? Why not just always use \Drupal::currentUser().
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?
Comment #25
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #26
klausiImplemented fago's comments, added a test case to cover ConfigurableEntityReferenceFieldItemList when entity_reference module is enabled. Interdiff: https://github.com/klausi/drupal/commit/a32cd1d3fb39107305bf30cf50f8abf6...
Comment #27
yched CreditAttribution: yched commentedNot 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.
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.
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)
Comment #28
fagoYep, 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:
Minor: Node terminology is author, so I'd suggest "Node author default value" here.
Minor: Comment too long.
Comment #29
yched CreditAttribution: yched commentedHalf 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).
Comment #30
yched CreditAttribution: yched commented(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)
Comment #31
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #32
fagoOpened #2226267: Improve default value handling of fields to be consistent for the general issue.
Comment #34
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #35
klausiThis should fix all test fails.
Interdiff: https://github.com/klausi/drupal/commit/91a69344ff3748a15bb1e0562c3c6e8b...
Comment #37
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #39
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #40
klausiRemoved initialization in node controller: https://github.com/klausi/drupal/commit/34979854b3e2968527a5a88e96cf27aa...
Fixed the merge conflict: https://github.com/klausi/drupal/commit/9c0ed242183f3fb1e771166b9f31df5e...
Comment #43
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedRerolled patch.
Comment #45
BerdirThose 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.
Comment #46
yched CreditAttribution: yched commentedagreed :-)
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)
Comment #47
BerdirNot 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?
Comment #48
RoSk0Added setDefaultValueCallback() to comment and file.
Moved getCurrentUserId() to ContentEntityBase class.
Created test for node only just to see how it works.
Comment #50
RoSk0OK, 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?!?
Comment #51
RoSk0Fixed tests, removed duplicated getCurrentUserId() from Comment class.
Comment #52
RoSk0For bots.
Comment #53
BerdirThe 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.
Comment #54
anavarrePer #53
Comment #55
andypostLooks no more valid cos we init with 0 now
Comment #57
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedIn 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.
Comment #58
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedComment #59
RoSk0Should this be getDefaultOwnerId?
Same as above?
Comment #60
fagoYeah, a trait would be nice here.
>Should this be getDefaultOwnerId?
Agreed.
Also, this needs tests - so back to nw.
Comment #61
tstoeckler1. Calling the field
uid
is very D6/D7 IMHO, I like to call the fieldowner
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.Comment #62
fago>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.
Comment #63
Berdir1. 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.
Comment #64
BerdirStarted 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.
Comment #66
Wim LeersWe also need this over at #1927648: Allow creation of file entities from binary data via REST requests.
Comment #69
timmillwoodRe-roll, plus moving things from
Node
toEditorialContentEntityBase
.Comment #75
AaronMcHaleFrom 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.