When trying to use the Publish On date token with the Automatic Nodetitles module it never seemed to work. I tracked it down to the fact that the publish_on element is not yet a timestamp when that token is rendered for use in the node title; it is still the date as entered by the user.

I wrapped the calls to $node->publish_on in _scheduler_strtotime() and it seemed to work. I'm not sure if I needed to use that particular function, but I did.

After this I tested the tokens with Automatic Nodetitles and with Rules for examples of different times the tokens would be rendered. They both printed out the expected values.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TommyK created an issue. See original summary.

TommyK’s picture

TommyK’s picture

Status: Active » Needs review
TommyK’s picture

Oops, in the last patch I was a little paste-happy and replaced the $node->unpublish_on with $node->publish_on. The attached patch fixes this.

jonathan1055’s picture

Hi TommyK,

Thanks for reporting this. Just so I can replicate what you are doing, could you give me a link to the module you are using? Here is a 7.x search for auto node title which returns hundreds of hits - I looked through the first couple of pages but it is better if you tell me directly and give a link to the project.

Currently in 7.x we do not have test coverage for tokens, so yes it could be broken, but also your change it not being tested, so we need to do thorough manual tests before any commiting. We do now have token tests in 8.x so I should try to back-port that to 7.x too.

Jonathan

TommyK’s picture

https://www.drupal.org/project/auto_nodetitle is the module that I've tried it with. It might also be worth testing with https://www.drupal.org/project/auto_entitylabel which does the same thing for the most part, but I wasn't using that module on my site.

TommyK’s picture

Issue summary: View changes
TommyK’s picture

Here is an explanation of how I found the issue:

After enabling the Automatic Nodetitles module, in the Automatic title generation vertical tab on a content type I added the following PHP (and checked the Execute PHP in pattern. checkbox) to see what is available to me at the point in the saving process when Automatic Nodetitles kicks in (assumes Devel is also enabled):

<?php
  dpm($node);
?>

Then I save a node that has scheduling enabled and can see that the Publish On and Unpublish On dates are still formatted date strings and not timestamps in the Devel output. If I look at the Devel output of a fully saved node, I see that at that point it is a timestamp.

I hope this helps.

jonathan1055’s picture

Title: Depending on when token is needed, timestamp might not be available in scheduler.tokens.inc file » Fields can be string or timestamp in scheduler.tokens.inc
FileSize
3.12 KB

Thanks for the link to the project. I can see what is going wrong. auto_nodetitle generates the tokens via hook_node_submit() which is early in the edit-save process. At this point the Scheduler fields are still strings, as they are not converted into timestamps until hook_node_presave().

However, in many other scenarios, such as Rules 'before saving content' and 'after saving content' these are both executed after hook_node_presave, so we have to cater for input data in both formats. I have created variables $publish_on and $unpublish_on to hold the timestamp values, which makes the rest of the function more readable.

I would like to add simpletests to cover both of these scenarios, but for now here is a patch with just the scheduler_tokens changes.

jonathan1055’s picture

I have added tokens test coverage (back-ported from the new 8.x tokens test). This patch just covers the existing tokens code, it does not (yet) test the changes made in this issue.

  • jonathan1055 committed 30e19ed on 7.x-1.x
    Issue #2750467 by jonathan1055: Added tests for scheduler token...
jonathan1055’s picture

Now that basic tokens test coverage is committed, just to prove, I have re-queued the original patch from 4. This should fail the new tests.

The last submitted patch, 4: scheduler-token_strtotime-2750467-4.patch, failed testing.

The last submitted patch, 2: scheduler-token_strtotime-2750467.patch, failed testing.

jonathan1055’s picture

I have now added test coverage for the string input, which is problem that this issue addresses. Here's a patch with just the expanded test only, which should fail, showing that the current code does not perform correctly.

Status: Needs review » Needs work

The last submitted patch, 15: 2750467-15.scheduler-tokens.expanded-token-test.patch, failed testing.

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
7.08 KB

Here is the full patch, with the code fix (from #9) and expanded test (from #15).

  • jonathan1055 committed 7b39fe7 on 7.x-1.x
    Issue #2750467 by jonathan1055, TommyK: Fields can be string or...
jonathan1055’s picture

Status: Needs review » Fixed
Related issues: +#2112869: 7.x test coverage

Thanks TommyK for raising this issue. It prompted me to get more complete 7.x test coverage too.

Just to confirm, this fix is not required at 8.x.

Status: Fixed » Closed (fixed)

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