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
Here's what Drupal\Core\Entity\ContentEntityBase::set() looks like:
/**
* {@inheritdoc}
*/
public function set($name, $value, $notify = TRUE) {
// If default language or an entity key changes we need to react to that.
$notify = $name == 'langcode' || in_array($name, $this->getEntityType()->getKeys());
$this->get($name)->setValue($value, $notify);
}
Note the {@inheritdoc}. That comes from Drupal\Core\Entity\FieldableEntityInterface, which says this:
* @param string $field_name
* The name of the field to set; e.g., 'title' or 'name'.
* @param mixed $value
* The value to set, or NULL to unset the field.
* @param bool $notify
* (optional) Whether to notify the entity of the change. Defaults to
* TRUE. If the update stems from the entity, set it to FALSE to avoid
* being notified again.
The relevant part being $nofity, which is supposed to allow you to change the notification behavior.
I'm not entirely sure what the notification behavior is supposed to do; I can see wanting to put off a database flush until many fields are changed, so maybe that's what it's for. The documentation doesn't tell us...
The point is that if you look at the implementation above, you see that ContentEntityBase::set() just obliterates $notify without honoring it.
That wouldn't be such a big deal because, you know, maybe this implementation needs to deal with it differently, but ContentEntityBase is the only class which implements FieldableEntityInterface.
I'm left wondering what's up.
Proposed resolution
- Figure out whether to update the interface, or whether
ContentEntityBase::set()is buggy. - Fix whatever needs fixing.
- Write a test to prove that it's fixed.
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | interdiff.txt | 657 bytes | Mile23 |
| #28 | 2388501_28.patch | 7.12 KB | Mile23 |
Comments
Comment #1
Mile23This comes about from trying to write a test: #2388537: Expand unit testing for Drupal\Core\Entity\ContentEntityBase
Comment #2
Mile23I discussed this with @dawehner in IRC and it's important to honor
$notifybecause otherwise we might propagate circular notices.Here's a patch which explores what a solution might be. Will the testbot totally barf?
Comment #3
Mile23Right or wrong, I wrote a test for it. :-)
Note that I had to slightly refactor the existing tests in order to isolate my test from their expectations. This is an illustration of why it is completely wrongheaded to use setUp() to set expectations.
Comment #4
Mile23And with the test only, no change to
set().Comment #6
Mile23Comment #7
Berdir$notify is about notifying the parent. ContentEntityBase/Interface never has a parent, it is always the root of the typed data structure.
What is passed on to the field is a different kind of notification flag.
$notify is probably a left-over from the type when content entities extended directly from TypedDataInterface. We can probably just remove that from the interface?
Assigning to fago to confirm this.
Comment #8
fagoYep, exactly! Let's remove it from the interface.
Comment #9
BerdirOk, that makes this a novice task then.
Remove $notify from ContentEntityBase and FieldableEntityInterface including the docblock documentation. Do not touch anything else.
Comment #10
BerdirComment #11
Mile23My understanding is that we care about
$notifybecause setting itFALSEallows you to prevent circular notifications.Also, if we're changing the interface, it's a bug.
Comment #12
BerdirNo, we don't care about notify and this is a task as we're not fixing a bug here, just removing a useless parameter.
Comment #13
rpayanmPlease review :)
Comment #14
Mile23Simpler code means simpler test. :-)
Comment #15
BerdirNo, $notify *inside* the method needs to stay, only the parameter should be removed.
Comment #16
BerdirComment #17
Mile23That's why I asked. :-)
Comment #18
Mile23Comment #21
daffie CreditAttribution: daffie commentedThe implementation of the set() function has changed. A reroll is not enough.
Comment #22
Mile23Comment #23
Mile23Here's a reroll of 17, and the changes I made to get it to work. The interdiff is between the reroll and the new patch.
Comment #24
daffie CreditAttribution: daffie commented@Mile23: It all looks good to me. I have only one problem. In the function testSet() there is a lot of preparation for a test, but a actual test assert is never called.
Comment #25
Mile23We could assert NULL for the void set(), but we're covered with these other expectations.
For proof you can run it with --strict. :-)
Comment #26
Mile23Comment #27
daffie CreditAttribution: daffie commented@Mile23: Let me start by saying that you are technically right.
I have two arguments to change it to an assertion:
The rest of the patch gets a RTBC for me.
Comment #28
Mile23Well, if that was the test, there'd be an assertion. :-)
But your point is taken, so here we go.
Comment #29
daffie CreditAttribution: daffie commentedThe patch removes the parameter $notify from the method set(). From the class and from the interface.
The test class is updated.
The comments are in order.
It all looks good to me.
The patch gets a RTBC from me.
Comment #30
alexpottI think that the test changes here are superfluous - writing tests to hit every line with an @covers can be futile since it can become more difficult to maintain the test than code. And rather than testing ContentEntityBase::set() what we should be testing is ContentEntityBase::get() since that has logic about getting translations - but such a change is out of scope.
Comment #31
daffie CreditAttribution: daffie commented@Mile23: I have looked at the code again and I think that it does no harm if we rename generateFixtures() back to setUp(). If I am wrong, please tell me.
What do you think about the idea of alexpott for adding testing for the method get()? In a new issue of course.
Comment #33
Mile23Try it and run the test. It will fail, because the former
setUp()code generates expectations for every test, which aren't met by the tests added by the patch. It's an incorrect use ofsetUp(). The easiest way to deal with that is to just rename it.Be specific about what you *do* want, and I will make that. Otherwise, the point is to prove what the changes do, and to make those changes maintainable.
So then either change scope or create another issue.
Comment #34
BerdirThe changes don't do anything, they remove dead code that has no meaning and can not be made to do anything.
Just upload a patch without any test changes, I don't think they are necessary here. It is not a bugfix that we need to have regression tests for.
Comment #35
Mile23Oh, then you want this issue: #2388537: Expand unit testing for Drupal\Core\Entity\ContentEntityBase Writing unit tests is really code analysis, wherein you find things like superfluous flags to interfaces.