Problem/Motivation

If we leave as unset() the field is removed from the entity entirely instead of just returning null.

Using unset() works fine to make a field's contents inaccessible, thanks to \Drupal\Core\Field\FieldItemBase::__unset() (see #6).

But, it's misleading, because it sets the expectations we are literally removing that field. We're not. We're merely removing its values.

Proposed resolution

Therefore, $entity->set($field_name, NULL) is preferable over unset($entity->{$field_name}).

Remaining tasks

None.

User interface changes

None.

API changes

None.

Original report by @marthinal

We must set values to NULL instead of using the unset() method as suggested by @Berdir on https://www.drupal.org/node/2405091#comment-9571671 .

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marthinal’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

Patch attached.

dawehner’s picture

I totally agree, we should always do that.
Can you maybe add some little bit of test coverage to ensure that we don't do that anymore in the future?

neilmc’s picture

Issue tags: +LatinAmerica2015

Working with @kporras07 on a test for this here at DrupalCon Bogota.

neilmc’s picture

Issue summary: View changes
neilmc’s picture

Issue summary: View changes
marthinal’s picture

Assigned: marthinal » Unassigned

Hi @neilmc. Thanks for working on it. :)

Well, I'm not sure about that

If we leave as unset() the field is removed from the entity entirely instead of just returning null.

Because

  /**
   * Implements the magic method for unset().
   */
  public function __unset($name) {
    // Unsetting a field means emptying it.
    if ($this->hasField($name)) {
      $this->get($name)->setValue(array());
    }
    // For non-field properties, unset the internal value.
    else {
      unset($this->values[$name]);
    }
  }

// Unsetting a field means emptying it.

We set the value to array().

kporras07’s picture

@marthinal, so; what do you suggest about this?

neilmc’s picture

Hi @marthinal, happy to help :)

So as far as I can tell before the patch was implemented the field was being emptied out and after the patch it should be set to NULL. In trying to modify the test to verify this difference we encountered some difficulties. It appears the field is being removed from the entity elsewhere in the code regardless of the patch. I tried to verify this by changing the code in EntityResource.php around line 55 to the following:

      if (!$field->access('view')) {
        \Drupal::logger('RESPONSE')->notice($field_name);
        //unset($entity->{$field_name});
      }

I then set up an endpoint with the rest module, used hook_entity_field_access_alter to change the permission on a test field on my node and hit that node with a get request.

Despite commenting out the unset, the field is still being removed from the render array elsewhere in the code causing the assertion in RestTest.php, line 79 to pass regardless of whether the patch has been applied or not.

$this->assertFalse(isset($data['field_test_text']), 'Field access protected field is not visible in the response.');

Hope this is helpful!

Berdir’s picture

This is more or less a coding standard thing. There is no bug therefore also nothing we can write tests for. The existing tests passing is enough proof for this.

xjm’s picture

I spoke to @neilmc about this issue at Bogotá, and agreed it was a normal task and that the current behavior seemed minorly fragile (not explicit and not best practice), so I suggested noting that in the summary (for a branch maintainer's consideration) since the disruption is minimal.

I think we should add a bit more background information to the Problem/Motivation in the summary, since right now it's absent any context. (Search for "unset" on #2405091: Cannot create user entities - {"error":"Access denied on creating field pass"} for the prior discussion).

I'm also adding #2405091: Cannot create user entities - {"error":"Access denied on creating field pass"} to the "related issues" section so it's visible on both issues.

marthinal’s picture

Issue summary: View changes
FileSize
2.12 KB

Updated Summary. Patch added again and should apply.

Thanks!

xjm’s picture

@marthinal, did you intend to remove the entire issue summary?

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes

Reverted to restore the previous summary. The beta evaluation is required for a normal task. I still think the problem/motivation should explain why we are actually making this change, based on the previous issue, not just a reference to unspecified "coding standards". See #10.

xjm’s picture

FileSize
2.12 KB

And reuploading the patch since reverting the summary apparently sent it away. :P

marthinal’s picture

Status: Needs review » Reviewed & tested by the community

@xjm sorry for removing the summary :)

Just one thing, I think that using unset(), we are not removing the field from the entity. The field exists but the value is array() and the correct way is using set() method instead unset() afaik :) But the result is the same.

This is a RTBC then.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

@marthinal, you can't RTBC your own patch. :)

marthinal’s picture

Oops... sorry :)

jackbravo’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 2418587-11.patch, failed testing.

jackbravo’s picture

Issue tags: +Needs reroll

Ups, maybe I forgot to update my local git repo.

AjitS’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.08 KB

Rerolling

klausi’s picture

Status: Needs review » Needs work

Why do we want to do this? How as a service consumer can I distinguish between a field that I don't have access to and an empty field then?

Not sure if this would be a security issues if consumers see the field names of fields they don't have access to?

Yep, the issue summary needs an update first.

Berdir’s picture

The result is the same. We don't actually unset anything. The field objects stay, we just remove the values and set it back to an empty array in both cases.

That's exactly why I suggested it to be changed. unset() is misleading. We're not unsetting anything, we're just triggering __unset().

We have the field access API to care about access.

Wim Leers’s picture

Title: Set entity values to NULL instead of using unset() method. » Set entity values to NULL instead of using unset() method: unset() is misleading
Priority: Normal » Minor
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update
FileSize
2.08 KB

#24 makes a lot of sense IMO.

IS updated based on #24.

Patch still applies, but not clearly, so a straight reroll is attached.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 805a22e on
    Issue #2418587 by xjm, marthinal, AjitS, Wim Leers, neilmc, Berdir: Set...

  • catch committed d2e71e0 on
    Issue #2418587 by xjm, marthinal, AjitS, Wim Leers, neilmc, Berdir: Set...

Status: Fixed » Closed (fixed)

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