Here's the next in the comparison of D6 hook_nodeapi with D7 hook_node_{op}. This one is more complicated because the op values of 'validate' and 'presave' in D6 execute exactly the same code in hook_nodeapi(). However, in D7, this has been split out into separate parts. This is the correct thing to do, but we need to verify that it has been done right.

Attached is a line-by-line comparison of the code. Discussion will follow.

CommentFileSizeAuthor
comparison of node validate and presave.jpg908 KBjonathan1055

Comments

jonathan1055’s picture

jonathan1055 commented
  1. All the changes from time() to REQUEST_TIME are correct.
  2. During validate, the checks for whether to do anything have been changed by replacing "isset and has a value" with "not empty" ie
       if (isset($node->publish_on) && $node->publish_on && !is_numeric($node->publish_on)) {

    is now

      if (!empty($node->publish_on) && !is_numeric($node->publish_on)) {

    and likewise for unpublish_on

  3. Setting the final node values is now done only in presave.
  4. In Presave, if no value has been specifed for publish_on then a value of zero is substituted instead.
eric-alexander schaefer’s picture

eric-alexander schaefer commented

1. OK.
2. That's fine with me.
3. Looks Ok to me.
4. This could be the reason why the NOT NULL stuff with views is not working. Will be right back...

eric-alexander schaefer’s picture

eric-alexander schaefer commented
Status: Active » Fixed

4. It's OK. The unused values have been zero in D6 too. The views filter problem must be somewhere else.

So both hooks look OK. They are now much cleaner since they are separate functions.

jonathan1055’s picture

jonathan1055 commented

Good.
For a future enhancement, I'd really like to see the 'no date set' value represented as Null not zero. Zero is a date (1st Jan 1970, I know I am being pedantic here). I know that !empty() considers the value of zero to be empty, and that is maybe why we've been getting away with it, but it would be safer to have Null so that tests on whether the value is null would also give the desired answer. It would clean up code and make viewing the node data clearer.

Let's get D7 1.0 released, then continue this discussion, if you think it merits it?

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.