Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Allow publish_on
dates in the past for advanced use cases.
For example:
- When creating nodes automatically (via an API e.t.c) from an external source, where time offsets and time zone differences may cause the
publish_on
field of a node not to validate. - When creating nodes automatically (via an API e.t.c) from an external source, where the
publish_on
field is manually set may cause thepublish_on
field of a node not to validate for past entries.
Comment | File | Size | Author |
---|---|---|---|
#24 | 1819074-24-scheduler-publish_past_dates-7.x-1.1.patch | 8.26 KB | temaruk |
#17 | 1819074-17-scheduler-publish_past_dates.patch | 8.84 KB | jonathan1055 |
#17 | 1819074.15_to_17.interdiff.txt | 3.79 KB | jonathan1055 |
#16 | interdiff.txt | 3.76 KB | pfrenssen |
#15 | 1819074-15-scheduler-publish_past_dates.patch | 8.78 KB | jonathan1055 |
Comments
Comment #1
dolphinonmobile CreditAttribution: dolphinonmobile commentedHere is a patch that adds the following changes:
scheduler_publish_ignore
variable that allowspublish_on
dates in the past to be ignored if set.scheduler_publish_ignore
check box option for each node type: Ignore publish time that is not in the future (Advanced).Comment #2
jonathan1055 CreditAttribution: jonathan1055 commentedFor reference #1567136: Change $node->created date with scheduler module to value in the past and #1940072: Ability to publish in the past. have some more discussion on this topic.
Comment #3
pfrenssenMarked #1567136: Change $node->created date with scheduler module to value in the past as a duplicate of this issue.
Comment #4
pfrenssenReading through this issue and the duplicates there seem to be two different approaches for this:
I agree with Raymond that these are both advanced cases, so I would suggest to keep the current behaviour as the default, and allow the administrator to choose between either of the two approaches to fit their particular use case.
I do think this should be a site-wide setting rather than something that can be configured separately for each content type. Having different content types react differently just makes it more complicated I think. Also, since this probably does not affect most users, shall we put this under a separate "Advanced options" section?
Comment #5
pfrenssenHmm maybe this should be configurable per content type, seeing that both raymondwanyoike and bartoll do it this way I am probably mistaken in my opinion.
Comment #6
jonathan1055 CreditAttribution: jonathan1055 commentedThanks for summarising this problem so accurately. Yes I knew it was not an obvious solution. Maybe a nice idea for config.
Comment #7
pfrenssenGoing to work on this.
Comment #8
pfrenssenHere is a patch that offers both options. They have been put in an "Advanced options" fieldset on the node type edit forms. I've also included a test.
This patch has been written from scratch, but I have taken inspiration from the approaches of raymondwanyoike and bartoll, so they should be credited if this patch would get accepted.
Comment #9
jonathan1055 CreditAttribution: jonathan1055 commentedHi Pieter,
This is excellent work - thanks very much. I like the way you have reviewed the other issues and decided on a solution to help everyone. I have fully tested the changes and everything works as expected. However, I have made a few minor alterations so have re-rolled the patch. The main processing is unchanged, that all works fine.
Original
New
Great work! I think this will be a useful addition to make scheduler more flexible.
Jonathan
Comment #10
pfrenssen#1125772: If publish_on date is today publish now has been marked as a duplicate of this issue, but for the 6.x branch. Tagging this for backporting to 6.x.
Comment #11
pfrenssenThis is the interdiff between patches #8 and #9.
Comment #13
pfrenssenReviewed the changes. Overall the changes are looking really good, love the improvements to the text strings :)
'pubishing' misses a letter :)
The is_numeric() check is not needed, the other check is sufficient.
The way the assert messages are rendered has changed a while ago, we should not be using t() any more, but format_string().
Comment #14
jonathan1055 CreditAttribution: jonathan1055 commentedHi Pieter,
Thanks for the review. I've been away for a few days, hence only just seen this.
Glad you like the text changes. Yes, I will look at the is_numeric() test. We also had some feedback on this issue from someone else but they put their comments in the wrong issue, over on #1069668: Default time with user override in #42, so I will incorporate their point that we forgot to change the content creation time to match the scheduled publish time if that option is set.
By the way, did you enable the automated testing of patches? That's very useful.
Jonathan
Comment #15
jonathan1055 CreditAttribution: jonathan1055 commentedYou are right, that test should use !empty() instead of is_numeric(). Also I have removed the other is_numeric() test because it is redundant as you pointed out. But for safety I have made sure that we get $node->key set to zero if _scheduler_strtotime returns false (which it should not do here, because validation has already passed).
I have fixed the typos, added the 'change creation date to match publish date' lines, and also changed t() to format_string() in the new tests in .test file (not changed the existing ones here). I have removed the t() around 'Basic page' when it is a parameter inside format_string because I think you are saying that the test text does not get translated.
Here is a patch against the new dev 1.1+19. It also does not conflict with the code review standards changes.
Jonathan
Comment #16
pfrenssenThis is the interdiff between patches #9 and #15.
Comment #17
jonathan1055 CreditAttribution: jonathan1055 commentedSeems like I left a few of the calls to t() in the .test file. I only changed the first one to format_string(). Re-rolled for web_user to admin_user recent commit.
Comment #18
pfrenssen@jonathan1055: Yes, I enabled the automated testing, I find that incredibly useful :) Also, it is not really needed to check compatibility inbetween patches ahead of them being committed, these conflicts are usually very easy to solve and if not we can always tag them with "Needs reroll".
On to the review!
This is some clever trickery! I would use the boring (int) type casting, but as this works just as well and has a nice comment explaining it :)
This does not really belong in the setUp() of the test, since it will then apply to all tests in the class. There might be some tests that require this setting to be off.
If you want to test it here you would need to test both the touch option enabled or disabled. This does highlight that this behaviour is currently not tested. I think we better delegate this to a specific unit test that saves nodes.
Oh no, I suggested this right? I was completely wrong, we do need to use t() here. It was actually fine before. I must have misread it as the assert message being wrapped by t(). My excuses!
Comment #19
pfrenssen@jonathan1055, I still had my review open from yesterday so I didn't see your last post. I'm sorry for setting you on the wrong foot with the format_string() issue. The assert messages should not be put through t() since then they become available for translation on localize.drupal.org, which is useless since these assert messages are only ever read by developers and do not need to be translated. When we are testing translatable strings in the interface, we should run it through t() though. format_string() is helpful since it allows to put placeholders in the string without translation.
So a typical example would be:
I'm very sorry for letting you waste your time on this :(
Comment #20
pfrenssenCreated a followup issue for testing the 'touch' functionality as well as all the other logic in scheduler_node_presave(): #2112867: Provide a unit test for saving scheduled nodes.
I've incorporated the update of the $web_user in #17 and committed it to 7.x-1.x: 8d40d8a. Thanks everybody!
Marking as needs backport to 6.x.
Comment #21
jonathan1055 CreditAttribution: jonathan1055 commentedShould we really be back-porting this to Drupal6? I know in #10 above you linked #1125772: If publish_on date is today publish now as a duplicate and tagged this issue for port to D6. But this is more than just a bug-fix, it has entirely new functionality and features. I suggest that the D6 version should remain supported but only for fixing bugs or serious usability problems. We could be making a whole lot of work for ourselves entirely unnecessarily by back-porting, particularly as the original creators of these issues no longer seem to be active. We have enough open issues on D7 to work on.
I propose to leave this issue as fixed. If anyone wants to re-open it and make a good case for back-porting then we can discuss it.
Jonathan
Comment #22
pfrenssenMake sense! If someone needs this for D6 then patches are always welcome.
Comment #24
temaruk CreditAttribution: temaruk commentedI need a patch for this issue that applies against 7.x-1.1 and I need it to be referenceable from D.org, so forgive me commenting on a closed issue, but here it is.
Comment #25
jonathan1055 CreditAttribution: jonathan1055 commentedOK, no problem. I presume you need to share it with others. Looks like you based it on the patch in #15, is that right?
I'm not sure why you patched the .test file though.
Jonathan
Comment #26
temaruk CreditAttribution: temaruk commentedI based it on the actual commit that was made in 7.x-1.x. I created a branch off the commit that was tagged with 7.x-1.1 and cherry picked the commit (and fixed the resulting conflict) that is mentioned in #20 (8d40d8a).