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

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug/Task/Feature because ...
Issue priority Critical because of multiple reasons:
  • Data loss of the author information, its annoying for sites
  • That data loss for itself is less problematic given that its not actual content often
  • What makes it way more problematic is the fact that user access is controlled the by the UID so the anonymous user could get access to edit a node (if you configured Drupal to do so)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

webchick’s picture

Steps 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

jhedstrom’s picture

Assigned: Unassigned » jhedstrom

I'll look into this.

amateescu’s picture

dawehner’s picture

Status: Active » Needs review
FileSize
1.13 KB

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

amateescu’s picture

I'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?

amateescu’s picture

@dawehner: the widgets will assign NULL to the entity reference field if nothing is typed in the input box, which is different than 0 :)

dawehner’s picture

I'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?

Status: Needs review » Needs work

The last submitted patch, 5: 2566419-3.patch, failed testing.

tim.plunkett’s picture

Issue tags: +Needs tests
jhedstrom’s picture

Assigned: jhedstrom » Unassigned
Status: Needs work » Needs review
FileSize
2.27 KB
2.27 KB
3.4 KB

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

The last submitted patch, 11: 2566419-11-TEST-ONLY.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2566419-11.patch, failed testing.

justAChris’s picture

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

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
3.91 KB

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

Status: Needs review » Needs work

The last submitted patch, 15: 2566419-14.patch, failed testing.

The last submitted patch, 5: 2566419-3.patch, failed testing.

hussainweb’s picture

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

The last submitted patch, 11: 2566419-11-TEST-ONLY.patch, failed testing.

The last submitted patch, 11: 2566419-11.patch, failed testing.

The last submitted patch, 15: 2566419-14.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.93 KB
1.51 KB

Let's fix the first portion of the test failures, second later.

Status: Needs review » Needs work

The last submitted patch, 22: 2566419-22.patch, failed testing.

dawehner’s picture

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

The last submitted patch, 22: 2566419-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.9 KB
999 bytes

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

catch’s picture

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

amateescu’s picture

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

Since we don't have #2490136: Enable revisions by default yet, the current HEAD behavior leads to data loss by default..

hchonov’s picture

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

amateescu’s picture

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

amateescu’s picture

In fewer words, what I've tried to explain above is that we still need to do #6 in order to fix this issue :)

Status: Needs review » Needs work

The last submitted patch, 30: 2566419-30.patch, failed testing.

The last submitted patch, 30: 2566419-30.patch, failed testing.

hchonov’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

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

hchonov’s picture

FileSize
3.81 KB

removing the debug from the test which I left accidentally. :)

dawehner’s picture

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

hchonov’s picture

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

dawehner’s picture

Issue summary: View changes
dawehner’s picture

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

amateescu’s picture

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.

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

amateescu’s picture

Added back proper test coverage for the autocomplete tags widget and also when the 'uid' field is hidden from the form display.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice improvement of the test coverage!

jibran’s picture

Uploading the test only patch for the last patch. Just for sanity.

amateescu’s picture

Just tried to run the test-only patch locally and it doesn't fail. Here's one that fails properly.

The last submitted patch, 44: 2566419-44-test-only.patch, failed testing.

plach’s picture

+++ b/core/modules/node/src/Tests/NodeEditFormTest.php
@@ -163,7 +163,7 @@ public function testNodeEditAuthoredBy() {
+    $this->checkVariousAuthoredByValues($node, 'uid[target_id]');

Why is there no [0] index here?

amateescu’s picture

Because the tags widget handles multiple values in a single form element.

The last submitted patch, 44: 2566419-44-test-only.patch, failed testing.

plach’s picture

I see, then RTBC +1 :)

andypost’s picture

+++ b/core/modules/node/src/Entity/Node.php
@@ -91,9 +91,9 @@ class Node extends ContentEntityBase implements NodeInterface {
-      $this->setOwnerId(\Drupal::currentUser()->id());
+      $this->setOwnerId(0);

there should be a test that makes sure author assigned by default

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Testing 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!

webchick’s picture

Oh, 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?

  • webchick committed dbf74b3 on 8.0.x
    Issue #2566419 by amateescu, dawehner, jhedstrom, hchonov, hussainweb,...
Berdir’s picture

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

yched’s picture

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

hchonov’s picture

What about if I want to give to authenticated users the possibility to create anonymously content even when logged in?

jibran’s picture

@hchonov then authenticated users have to set the author field value to anonymous (0) in the UI.

hchonov’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.