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 .
Comment | File | Size | Author |
---|---|---|---|
#25 | set_entity_values_to-2418587-25.patch | 2.08 KB | Wim Leers |
Comments
Comment #1
marthinal CreditAttribution: marthinal commentedPatch attached.
Comment #2
dawehnerI 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?
Comment #3
neilmc CreditAttribution: neilmc commentedWorking with @kporras07 on a test for this here at DrupalCon Bogota.
Comment #4
neilmc CreditAttribution: neilmc commentedComment #5
neilmc CreditAttribution: neilmc commentedComment #6
marthinal CreditAttribution: marthinal commentedHi @neilmc. Thanks for working on it. :)
Well, I'm not sure about that
Because
// Unsetting a field means emptying it.
We set the value to array().
Comment #7
kporras07 CreditAttribution: kporras07 commented@marthinal, so; what do you suggest about this?
Comment #8
neilmc CreditAttribution: neilmc commentedHi @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:
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.
Hope this is helpful!
Comment #9
BerdirThis 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.
Comment #10
xjmI 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.
Comment #11
marthinal CreditAttribution: marthinal commentedUpdated Summary. Patch added again and should apply.
Thanks!
Comment #12
xjm@marthinal, did you intend to remove the entire issue summary?
Comment #13
xjmComment #14
xjmReverted 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.
Comment #15
xjmAnd reuploading the patch since reverting the summary apparently sent it away. :P
Comment #16
marthinal CreditAttribution: marthinal commented@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.
Comment #17
xjm@marthinal, you can't RTBC your own patch. :)
Comment #18
marthinal CreditAttribution: marthinal commentedOops... sorry :)
Comment #19
jackbravo CreditAttribution: jackbravo commentedThis looks good to me :)
Comment #21
jackbravo CreditAttribution: jackbravo commentedUps, maybe I forgot to update my local git repo.
Comment #22
AjitSRerolling
Comment #23
klausiWhy 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.
Comment #24
BerdirThe 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.
Comment #25
Wim Leers#24 makes a lot of sense IMO.
IS updated based on #24.
Patch still applies, but not clearly, so a straight reroll is attached.
Comment #26
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!