Store empty scheduling dates as null not zero

This is actually only a flag in the views filter api ('allow empty' => TRUE). The filtering is done by views. I don't know what could be wrong on our side. I will have to play with it a bit.

I trying to figure this out. What operator are you talking about and how do you set it?

Oh, OK. Didn't know that.

It looks like there are no NULL values. It does not even work with a single value. Gotta dig deeper...

This does not work at all. The NOT NULL filter only works if both are not null (i.e. the node has no entry in the scheduler table at all). If at least one of the two dates is set the NOT NULL filter for the other value does not work. I am convinced that this has worked in D6, but I will have to verify that.

Steps to reproduce:
- create a node that has no scheduler dates set (node 1)
- create a node with publish_on set to a valid date (node 2)
- create a node with unpublish_on set to a valid date (node 3)
- create a view with node:title, scheduler:publish_on and scheduler:unpublish_on
- filter for scheduler:unpublish_on NOT NULL

-> Nodes 2 and 3 are visible in the view, although 1 AND 2 should be filtered out

I even tried all kinds of hack in scheduler_node_load, like not setting the values if they are empty and setting them to NULL explicitly, but to no avail.

Same thing in D6.

I think we will have to introduce NULL into the db columns (they are NOT NULL right now). This might break lots of other stuff (scheduler_node_load() comes to mind). I'd rather not do that now. I guess we can leave that for post one-oh, because it actually mimics the D6 behavior, where this is also broken. Opinions?

I mentioned in #1052002: Compare D7 hook_node_validate/presave with D6 hook_nodeapi(op=validate/presave) that I think storing unused dates as NULL would be preferrable. For expediency I think it is better to mimic the D6 behavior now, and get a D7 version out and being used. Then we can do the table update and code change as a separate task.

Setting this back to active. It was forgotten for 7.x-1.1 but it does need to be fixed as it is definitely a bug and gives the wrong behaviour. I agree with Eric that we should consider storing 'no date' as null not zero.

#2022919: Sorting by publish_on gives wrong result if unpublish_on is present is another example of this problem.

OK, here is a quick review following a read through of the code to find the changes required for storing empty dates as null instead of zero:

1. Change schema in .install
2. Write a db update to remove 'not null' from the existing table
3. scheduler_node_presave() - do not change empty to zero
4. scheduler_node_insert() and scheduler_node_update() - check that !empty() works
5. _scheduler_publish() - change test for whether the scheduler row can be deleted, and change update to null not zero

So it might not be as difficult as we first thought. There are also several places where the existing code will work ok after the db change, but can be simplified to make it more readable.

Moving this to the D8 queue. My review in #12 above showed that the work does not seem too tricky, but before proceeding to start this in D7 we need to have a discussion on how the dates (and indeed the entire Scheduler table) might be changed for D8. We can then agree on the best steps to fix this - as I think it's clear from this thread that storing 'no date' as zero is not the best.

Now that #2498203: Use field storage rather than a custom table is the method we are using for Drupal 8, this issue is no longer relevent for 8.x, so moving back to 7.x

This wont be change in the 7.x version now.