Problem/Motivation
1. Exchange the widget for the author field on a node form with "Autocomplete (Tags style)".
2. Create a new node and save it.
3. Now the saved author is "anonymous", but the submitted one was the currently logged in user.
The problem relies in NodeForm::buildEntity, where the submitted user_id is expected to be in $form_state->getValue('uid')[0]['target_id'], but when using the "Autocomplete (Tags style)" widget the form values uid structure looks like this:
array (
'target_id' =>
array (
0 =>
array (
'target_id' => '1')
1 =>
array (
'target_id' => '2')
)
);
In this case NodeForm::buildEntity will set the author uid to 0.
This leads to data loss and by definition the issue should be critical.
Proposed resolution
tbd
Remaining tasks
tbd
Issue category | Bug/Task/Feature because ... |
---|---|
Issue priority | Critical because of multiple reasons:
|
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff.txt | 3.44 KB | amateescu |
#44 | 2566419-44.patch | 9.32 KB | amateescu |
#44 | 2566419-44-test-only.patch | 6.46 KB | amateescu |
#43 | 2566419-fail.patch | 6.11 KB | jibran |
#41 | interdiff.txt | 5.72 KB | amateescu |
Comments
Comment #2
webchickSteps I took to verify this bug:
1. Created some content.
2. admin/structure/types/manage/story/form-display, set "Authored by" to "Autocomplete (tags style)"
3. Go back to site. Authors of content are all still correct.
4. Edit one of them.
5. Verify authoring information field says "webchick (3)"
6. Save the form without changing anything.
7. Upon save, author changes to Anonymous.
Well, shit. :P
Comment #3
jhedstromI'll look into this.
Comment #4
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNote that we have a pre-existing issue for updating that code: #2322525: Add separate SelectionHandler for entity_reference fields using user entity used in Author context
Comment #5
dawehnerThe fix is easy, just rely on how the widgets actually work.
While testing this I had a problem with saving the comment setting, but it seemed to be a temporary hickup, the reinstall worked.
Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI'm not sure that a separate selection handler or widget (as proposed in #2322525: Add separate SelectionHandler for entity_reference fields using user entity used in Author context) would make sense, because this is still an entity reference field after all and site builders should be allowed to use any widget for it.
A simple fix would be to add a setting to the user selection handler to default to the anonymous user if the referenced entity (user) does not exist. This could also be a widget setting for both autocomplete widgets, but it would have be a setting whose availability depends on a field storage setting, which I don't think we do anywhere else.
Other thoughts?
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@dawehner: the widgets will assign NULL to the entity reference field if nothing is typed in the input box, which is different than 0 :)
Comment #8
dawehnerI'm not actually sure whether we need the special behaviour? Its something most people don't have access to, so it might be better to have the same behaviour as normal?
Comment #10
tim.plunkettComment #11
jhedstromHere's a test that illustrates the current failure. Oddly, I couldn't find an existing test of the node form (the closest I could find was the NodeFormButtonsTest, which didn't seem appropriate.
Comment #14
justAChris CreditAttribution: justAChris as a volunteer commentedAdding related issue #2566603: Hiding the node 'Authored by' field from the edit form sets the author to anonymous user. Similar steps to reproduce, only using the -Hidden- option, which also apparently removes the author on edit (so same results).
Comment #15
hussainwebThis won't fix the tests but I am hoping to get some clue as to the failures. Also, since there is no other test with the default widget, this test adds it.
Comment #18
hussainwebIt seems removing the buildEntity method affects the translations somehow. After applying the patch, I was able to use the Autocomplete (Tags) widget just fine but as soon as I enabled translation, I couldn't change the node's owner from admin to another user, on any translation.
Comment #22
dawehnerLet's fix the first portion of the test failures, second later.
Comment #24
dawehnerTried to debug that a bit. When I put my breakpoint in
\Drupal\node\NodeTranslationHandler::entityFormEntityBuild
$entity->uid->list->0->values
is empty, but$entity->uid->list->0->properties['entity', 'target_id']
is set, see attached screenshot.Comment #26
dawehnerDebugged together with @amateescu on the issue. There seems to be just some old code lying around, which just doesn't work. Let's change that, which passes the tests for me locally.
Comment #27
catchSo I don't this is should be critical because of 'data loss', or at least, data only gets actually lost if you don't have revisions enabled.
The data being wrong from a form is 'definitely major', and depending on how bad it might be critical, but for me this one doesn't meet the 'data loss' = 'critical' definition at least not without a lot of qualification.
Comment #28
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSince we don't have #2490136: Enable revisions by default yet, the current HEAD behavior leads to data loss by default..
Comment #29
hchonovhmm I have revisions enabled and when creating a new content with this widget the author does not get saved.
So in both cases we have data loss.
Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI looked into this a bit more and there is a big problem with outright removing
NodeForm::buildEntity()
without taking #2322525: Add separate SelectionHandler for entity_reference fields using user entity used in Author context into consideration.I traced the problem way back to #2226493-88: Apply formatters and widgets to Node base fields, the patch that introduced the @todo for #2322525: Add separate SelectionHandler for entity_reference fields using user entity used in Author context. That patch changed the asserted behavior in order to make the tests pass, but it should have added a @todo as well to restore the original test behavior, because right now we don't have test coverage for "leave the author field blank for an existing node in order to assign that node to the anonymous user".
In short, this patch attached needs to pass if we want to be able to remove
NodeForm::buildEntity()
. (I've included the interdiff from #26 just to point out which other test will fail)Btw, it turns out we already have a test for the node edit page, it's just named badly :/
Comment #31
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIn fewer words, what I've tried to explain above is that we still need to do #6 in order to fix this issue :)
Comment #34
hchonov@amateescu:
the test is failing because Node::preSave is always setting the current user as owner if no owner has been set.
I think this is false here, as there might be exactly this use case where we want to create anonymously content or at least make this possible.
So fixing that and the test should pass.
Comment #35
hchonovremoving the debug from the test which I left accidentally. :)
Comment #36
dawehnerMh setting the anonymous user can also be potential problematic, given that it could result into making it possible to change the node by anonymous users (I mean we have permission available), what do you think?
Comment #37
hchonov@dawehner:
The anonymous user does not have by default the permission to do anything with the content. It is up to the site admin to decide if this is desired.
And we are setting the anonymous user as owner of a node only in case the default value for the current user was explicitly removed from the form, which should always be there. So the developer can set $form['uid']['#access'] = FALSE and then the current user would not be able to create content as an anonymous user.
Comment #38
dawehnerComment #39
dawehnerYou are right, its certainly not the default configuration.
As @amateescu told me, this is the default behaviour in Drupal 6 and 7, if you have an empty
value in there, you get the anonymous user, so this behaviour is battle provided, so +1 for reverting that behaviour change.
Comment #40
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat sounds right. I also checked creating a node through the API and the default value callback of the 'uid' field applies and sets the current user as the node author.
Comment #41
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAdded back proper test coverage for the autocomplete tags widget and also when the 'uid' field is hidden from the form display.
Comment #42
dawehnerNice improvement of the test coverage!
Comment #43
jibranUploading the test only patch for the last patch. Just for sanity.
Comment #44
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedJust tried to run the test-only patch locally and it doesn't fail. Here's one that fails properly.
Comment #46
plachWhy is there no
[0]
index here?Comment #47
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedBecause the tags widget handles multiple values in a single form element.
Comment #49
plachI see, then RTBC +1 :)
Comment #50
andypostthere should be a test that makes sure author assigned by default
Comment #51
webchickTesting this out...
- Create a new node: username retained.
- Re-edit existing node without changing anything: username retained.
- Add a revision, re-save form: username retained.
- Add a new revision, blank out username: author switches to anonymous, but username of prior revisions are kept.
plach approved, I think this is ready to go. Awesome work here, folks! :D
Committed and pushed to 8.0.x. Thanks!
Comment #52
webchickOh, bother. I cross-posted with #50. That lack of test coverage is a pre-existing condition though, so can we spin off a follow-up for it?
Comment #54
BerdirIMHO, we removed the case for leaving the field empty to get the anonymous user because that's not how entity reference widgets work, you can't do that with a custom user reference either. One reason of that conversion was to use the same behavior for all fields/widgets instead of having custom special cases. Pretty sure we actually updated existing tests back then.
The code in preSave() is IMHO just a left-over of the times before we had field default values.
Maybe we should just have made that field required, so you can't leave it empty?
Don't really care that much, just wanted to point out the reason for why it was like that.
Comment #55
yched CreditAttribution: yched commentedAgreed with @Berdir, it seems weird indeed that node "author" (uid) is not set as required and we instead catch the "not set" case at preSave() to fiill in the anon user. Semantically, it *is required*, we never save a node without an author ?
Comment #56
hchonovWhat about if I want to give to authenticated users the possibility to create anonymously content even when logged in?
Comment #57
jibran@hchonov then authenticated users have to set the author field value to
anonymous (0)
in the UI.Comment #58
hchonov@jibran:
Thanks, I didn't knew it is possible to asing from the UI the anonymous user.
@Berdir, @yched:
In this case I also think that we should make the author field required.