Contrib.social

$notify in FieldableEntityInterface::set() is useless

Open on Drupal.org →
Created on 6 December 2014, over 10 years ago
Updated 25 April 2025, 4 months ago

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

📌 Task
Status

Postponed: needs info

Version

11.0 🔥

Component

entity system

Created by

🇺🇸United States mile23 Seattle, WA

Live updates comments and jobs are added and updated live.
  • stale-issue-cleanup

    To track issues in the developing policy for closing stale issues, [Policy, no patch] closing older issues

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 4 months ago →
    🇺🇸United States smustgrave

    Thank you for creating this issue to improve Drupal.

    We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

    Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

    Thanks!

  • Status changed to Needs work 3 days ago
  • Comment 3 days ago →
    🇺🇸United States smustgrave

    Actually looking at the tasks now, this still seems relevant. Question though do we need to go the deprecation route?

  • Merge request !12965deprecate it → (Open) created by fago
  • Comment 3 days ago →
    🇦🇹Austria fago Vienna

    I agree it should be deprecated for now, not just removed. Some child implementations might be using it, so silently removing the parameter would render the child implements having a weird undocumented $notify parameter. Besides ContentEntityBase I see it implemented in \Drupal\simple_oauth\Authentication\TokenAuthUser::set

    I'd argue the change needs no unit tests, even when removing the parameter, there is 0 change in behavior.

    I added a MR for deprecating it.

  • Pipeline finished with Success
    3 days ago
    Total: 934s
    #570446
  • Comment 3 days ago →
    🇺🇸United States smustgrave

    Thanks but this seems to be missing a number of changes from previous patches.

    And needs to be properly be deprecated with a trigger_error and CR

  • Comment 2 days ago →
    🇦🇹Austria fago Vienna

    What changes are you missing?

    When we keep the parameter, there is no change necessary to ContentEntityBase + I argued why we don't to extend unit tests here. I don't see any other changes.

    > And needs to be properly be deprecated with a trigger_error and CR

    Not sure, this is helpful / needed in this case. Since the parameter was already doing nothing before, there is no change and no possible breakage to bug the developer about.

    But if preferred, I can add it when a FALSE or NULL value is passed instead of the TRUE default.

  • Comment 2 days ago →
    🇺🇸United States smustgrave

    Still would need to be properly deprecated.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024