Receiving twice each of the following after setting/saving some configuration combinations:

Notice: Trying to get property of non-object in comment_notify_form_user_form_alter() (line 215 of …/comment_notify/comment_notify.module) …

Notice: Trying to get property of non-object in comment_notify_form_user_form_alter() (line 231 of …/comment_notify/comment_notify.module) …

Steps to reproduce:

1. Enable the module.
2. Save a user.

At first I thought it was because of a particular configuration I set up with comment_notify but it happens right after the module is installed without any configuration being done for the module or for the per-user settings.

The notices sometimes appear after setting some combinations of settings again, but this is hard to reproduce, and eventually the notices go away.

CommentFileSizeAuthor
#15 comment_notify-user-settings-defaults-2865943-15.patch841 bytessimonholt83
#14 comment_notify-user-settings-defaults-2865943-14.patch1.12 KBsimonholt83
#10 comment_notify-n2865943-10.patch419 bytesDamienMcKenna
#2 comment_notify-user-setting-defaults-2865943-2.patch605 bytesTommyK
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TommyK created an issue. See original summary.

TommyK’s picture

TommyK CreditAttribution: TommyK as a volunteer commented
Status: Active » Needs review
FileSize
comment_notify-user-setting-defaults-2865943-2.patch605 bytes

I think I figured this one out. We are looking at comment_notify_get_user_notification_setting($uid) in comment_notify.inc.

I don't understand &drupal_static(__FUNCTION__) much, but from my interpretation of the API docs (https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...) because the module was just installed, the function is being called for the first time after a reset and won't have the right information available. The $users object is NULL at this point.

We need to offer a condition for that, and the defaults should probably be supplied in this case.

function comment_notify_get_user_notification_setting($uid) {
  $users = &drupal_static(__FUNCTION__);
  // Use defaults if this is being called for the first time after a reset.
  if (!isset($users)) {
    $users[$uid] = comment_notify_get_default_notification_setting();
  }
  if (!isset($users[$uid])) {
    …
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented

The &drupal_static(__FUNCTION__) will get called once per page, it's a way to do caching inside of a single request cycle. I think that's slightly different than the scenario you described (being right after the module is installed).

Your change looks sane to me, though, and I'm inclined to commit it.

Did it fix the notice for you?

TommyK’s picture

TommyK CreditAttribution: TommyK as a volunteer commented

Apologies for the delay on this, but yes, once I made that change there was no notice even immediately after install.

MartinMa’s picture

MartinMa CreditAttribution: MartinMa as a volunteer commented

Just have the same message now, but months ago, i never have seen it on my (old) installation (with composer manager which is now abandoned). just moved to a installation with set up with composer ...

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Parent issue: » #2770889: Comment Notify Next release - 8.x-1.0-beta5
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Related issues: +#2925405: Notice in comment_notify_form_user_form_alter() function

FYI #2925405: Notice in comment_notify_form_user_form_alter() function is an alternative way of handling this.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Related issues: +#2850935: Error when the user has been created and edit his profile for first time

And #2850935: Error when the user has been created and edit his profile for first time seems to be another duplicate.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented

@DamienMckenna - is there one you think should be committed? Question to anyone, really.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
FileSize
comment_notify-n2865943-10.patch419 bytes

I think this would be a better approach, less duplicate code 'n stuff.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

Of course I just realized that would require a bunch of internal API changes elsewhere..

anawillem’s picture

anawillem CreditAttribution: anawillem commented

Hi Damien - when you say there are other API calls that will need fixed, are you saying that this patch will break other things, or that this patch is fine, but there are other places that need it as well?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

@anawillem: I think the problem is that the default isn't loaded properly across the module as a whole, though it has been almost a year since I last looked at it.

simonholt83’s picture

simonholt83 CreditAttribution: simonholt83 as a volunteer commented
FileSize
comment_notify-user-settings-defaults-2865943-14.patch1.12 KB

The problem with the patch in #10 is that it will prevent comment_notify settings from being saved from the user form. This is because comment_notify_get_user_notification_setting() is also used when updating comment_notify settings, to check for existing settings for a user. So if it always returns something it will always try to do an update in comment_notify_set_user_notification_setting

Instead we should just load the defaults directly from the form_alter, if there was no settings found for the current user being edited.

simonholt83’s picture

simonholt83 CreditAttribution: simonholt83 as a volunteer commented
FileSize
comment_notify-user-settings-defaults-2865943-15.patch841 bytes

Updated patch to fix exception on user register form.

anawillem’s picture

anawillem CreditAttribution: anawillem commented

Thank you @simonholt83!!!

AlexJ’s picture

AlexJ CreditAttribution: AlexJ as a volunteer commented

Hi, I'm getting this notice when saving a new role to a registered user with 8.x-1.0-beta2+0-dev,

"Notice: Trying to get property 'node_notify' of non-object in comment_notify_form_user_form_alter() (line 215 of ..." and also
"Message Notice: Trying to get property 'comment_notify' of non-object in comment_notify_form_user_form_alter() (line 231 of ... "

Can I use this patch now too?

simonholt83’s picture

simonholt83 CreditAttribution: simonholt83 as a volunteer commented

@AlexJ Yes, the patch should fix the notices you mention.

gnuget’s picture

gnuget
Primary language Spanish
CreditAttribution: gnuget as a volunteer and at Agaric commented
Status: Needs review » Closed (duplicate)

This was fixed at #2925405: Notice in comment_notify_form_user_form_alter() function

Thanks!