I hacked the concept of 'non-required subscription fields' into notifications_subscription.class.inc (and companion code in notifications.pages.inc).

(Below is a patch with whitespace changes, because otherwise indents are wrongly unchanged on a few lines... and a patch without whitespace changes, because the whitespace one is confusingly large.)

The real-world example I am using this for, is a rooms-for-rent site. People can create a subscription to be notified of new rooms which are below a user-specified rental price and/or above a user-specified surface. This patch implements the groundwork to make the 'or' in that sentence possible.**

---

So, questions:

- Do you think this is something you want to implement into Notifications_Subscription?
Or is it better off as a separate class ('child' class of Notifications_Subscription)? I'm almost sure there is some provision for that in your code.

- Do you envision existing subscription fields being editable, in the future? The class currently does not allow that (and I didn't bother trying to understand why exactly), so I made use of that. If it will allow editable subscription fields in the future, I'll need to do more coding to allow for that (and / or I just made your job harder).

---

Some remarks that come to mind when looking back over what I did:

1) $this->fields has had a 'scope change' here. Next to the name of the field it used to store only a value. Now it also stores a field property. And field values & properties obviously need to be retrieved from different sources. (Values from the db:{notifications_fields}; properties from definitions in code.)

2) To mark fields as 'not required', this should be specified in the field definitions of hook_notifications('subscription fields'). Each non-required field should have a key-value "'required' => FALSE," added. Obviously if that does not exist, the default is TRUE... so my patch will not influence any existing behaviour, until a field is defined as "'required' => FALSE,".

3) point 1 means that there now is a call to notifications_subscription_fields() in the class. (In get_type_fields().) There wasn't before.

4) after a call to load_fields() or build_submission()=>set_properties(), the 'required' property of fields will not be set correctly. I didn't bother to - since a loaded field is never editable. Saves time/coding.

5) the above two calls (setting values from the database or from a form submission) will set only those fields which have values. So the number of fields set in the object may be less than the number of fields defined for this description.
check_fields() has been adjusted to count for that.

6) This check_fields() was the only code that needed real adjustment. (Next to
- set_type_fields() making an extra call to get the 'required' properties
- normalize_fields() getting an extra 'allowed structure of input values', used from set_type_fields()
- validate_submission() getting $form as an extra argument, to check for required-ness of the parsed form fields)

7) I just made that 2nd argument to validate_submission() an optional one, to not break compatibility... but maybe you want to change that and/or shuffle the two arguments around. Up to you.

8) Just throwing a remark out there: I have not yet seen the use of the 'field' key in a field definition of hook_notifications('subscription fields'). I see the 'field' keys defined in notifications_content_notifications()... but I haven't seen them used so far. The field names are in the array keys already (and at least the Notifications_Subscription class doesn't use them. It retrieves fields from hook_notifications('subscription types'). )

---

** I can provide the module that does this, as a practical example. (Maybe as a little example module to be included in notifications_extra? or just publish it on my own blog; it has a pretty narrow focus). But I still need to clean it up.

This is also an example of how to implement 'subscription fields' that are meant to be larger/smaller than a node's value, instead of 'exactly equal to'. I found absolutely no documentation on how to do this, so it has cost me quite some time/brain exercise, to determine the structure & intent of the existing 4.x code, and how to implement this.

I can structure my notes and write them up, as the beginnings of some documentation/article-like text for other people who want to do the same. I think some people will be able to use it. But it will probably take another proof reader to rewrite it into an article that's understandable to a wider audience.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

roderik’s picture

Hmm, this patch already doesn't apply properly to the latest 4.x-dev version (july 22nd) anymore.
I'm trying to keep up to date with 4.x-dev, but unfortunately I haven't set up my system perfectly. I'm not properly 'rebasing' all my individual patches, and I've been doing some other minor ones (issues 828556, 828860, 853318, 862080, 862258) so it's a drag to isolate them.

If someone gets to this issue, and wants me to isolate this patch again, give me a holler. (I'm hoping some issues will be committed by then ;-) )

In the meantime, my latest files (latest -dev with all above patches applied) can be found at my bzr repo: http://bzr.wyz.biz/drupal/6-patched/files/head:/sites/all/modules/notifi...
(files: notifications.pages.inc & includes/notifications_subscription.class.inc)

roderik’s picture

FileSize
12.25 KB
1.83 KB
26.52 KB

I got my git together. These apply to the latest -dev - BUT you must have #853318: form_set_error argument fix in user/UID/notifications/add/TYPE form applied already.

Patch 1: whitespace/comment changes. Separated so as to not distract patch reviewing.

Patch 2: refactoring check_fields(). Doesn't have to do with the patch per se, and you don't have to apply it to apply 3. I just assume that people will agree on the sensibility, but I'm not opening a different issue for it.

Patch 3: the actual functionality.