i18n support - Synchronize scheduled dates in translated nodes

Hoi Joost,

In D6 I fixed this with a custom module:

/**
  * Implementation of hook_i18nsync_fields_alter().
  */
function cannedconceptdotcom_i18nsync_fields_alter($fields, $type) {
  // add scheduler fields to i18n sync options in admin/content/types
  $fields['extra']['#title'] = 'Extra';
  $fields['extra']['#options'] = array(
    'publish_on' => t('Scheduler publish on'),
    'unpublish_on' => t('Scheduler unpublish on')
  );

}

/**
 * Implementation of hook_form_alter().
 */
function cannedconceptdotcom_form_alter(&$form, &$form_state, $form_id) {
  if($form["#id"] == 'node-form') {
    $i18nsync_fields = i18nsync_node_fields($form['type']['#value']);
  }
  // prefill draft translations with translation source scheduler data if necessary
  if($form['#node']->translation_source && $form['scheduler_settings'] && in_array('scheduler', (array)$i18nsync_fields)) {
    $form['scheduler_settings']['publish_on']['#default_value'] = format_date((int)$form['#node']->translation_source->publish_on, 'custom', $form['scheduler_settings']['publish_on']['#date_format']);
    $form['scheduler_settings']['unpublish_on']['#default_value'] = format_date((int)$form['#node']->translation_source->unpublish_on, 'custom', $form['scheduler_settings']['unpublish_on']['#date_format']);
  }

I know you're on D7 but maybe this helps...

Cheers

See patch attached. It's a real quick win thanks to how this module and the i18n_sync module are programmed.

To prefill scheduler data from a source translation node to a new translation, I extended my previous patch.

How can I test this? What are the prerequisites to triggering this functionality?

You can test this if you have a content type which is translatable and has scheduler enabled.
1) create a node
2) add a translated version of that node
3) change scheduler information on one of those two nodes and notice that the scheduler information won't be changed accordingly on the other node

Applying my patch will let you keep that information synchronized, automatically.
Ps: this requires i18n_sync, otherwise you won't see the settings you need.

Hello,
This looks like a useful addition for Scheduler. I'd like to test your patch and have got this far:

1. Downloaded i18n module from https://drupal.org/project/i18n
2. Downloaded Variable module https://drupal.org/project/variable
3. Enabled Core: Locale, Content Translation
4. Enabled Variable: Variable
5. Enabled Multilingual Internationalisation : Internationalisation, Synchronize translations
6. In Configuration -> Languages -> added new language

Not sure what to do next. When I edit a node I do not see any place to create a translation of it. Obviously there is something else I have not done. Please can someone check the above, and then tell me the next steps. Happy to test this but given that I have never worked on a multi-lingual site it needs to be more fully explained and easy to follow.

Thanks
Jonathan

Next thing you need to do is configure a content type to be multilingual.
You can read about it here: https://drupal.org/documentation/modules/translation

After that, you'll be able to translate a piece of content into another language.

Thanks. OK to continue the testing:

7. Structure -> Content Types -> Edit
8. In Publishing settings: Multilingual support = Enabled with translation

9. In Synchronise translations: Select the node fields (I did all of them). The scheduler fields are not shown yet
10. Edit a node, you now get 'choose a language' drop-down selection. Choose a language
11. Enter scheduler publishing date and save
12. The node now has an extra tab 'Translations', allow a new node to be created which is a translation of the first one. Click this and create a translation of the node
13. The scheduled date will be empty in the translated node

Now I applied the patch, and in Content Types -> Synchronise, we get the two scheduler fields. Tick both.

Then when editing one of the nodes which are linked, the scheduled dates are automatically replicated in the other node (or nodes). Also when making a new translation the dates are copied to the new node.

The original patch did not apply properly because of other changes it was made, but I have re-rolled it here. The content is identical, apart from minor coding standards and some wording changes. The actual php is identical. It works, hence marking this RTBC.

Thanks very much PieterDC and Bartezz. Excellent addition to Scheduler.

Jonathan

Excellent! This looks like a great enhancement. And thanks again to jonathan1055 for providing such a thorough review (as always!)
Committed: http://drupalcode.org/project/scheduler.git/commitdiff/7ea9b0e

Thanks Rick. The commit is actually http://drupalcode.org/project/scheduler.git/commitdiff/e38eae4

Just discovered a problem with this when the nodes are processed during cron. For example, when a scheduled node and its translation are being published in a cron run, the update to the first node is processed ok. This removes $node->publish_on from the first node (ok) but during the publish and save actions, the translated node also gets its $node->publish_on removed. Then in cron, when processing the second node, we get an error that publish_on is an undefined property.

There are (at least) two ways to fix this:

1. Given the fact that the second node becomes published automatically due to synchronisation, it does not need to be processed by scheduler cron. We could check to see if $node->publish_on is still set before processing the next node in the loop. If skipping, could give a log message to say linked translated node is now also published. The advantage of this is that it is simple to code. The disadvantage of this is that other things will not be processed, eg implementations of hook_scheduler_api()
2. Attempt to inhibit the synchronisation updates if the processing is being done in a cron run. Then scheduler cron will publish all three just as if they were not linked. Maybe trickier to program, but the full scheduler cron processing will still be done on all nodes. This may be important for other hooks.

Maybe there are other ways to solve it though.

Jonathan

This error is not so apparent when the node also has an unpublish_on date, because the row still exists in the scheduler table. I think that is why I missed it during my initial testing. If rules are invoked on the subsequent nodes we get major errors and cron processing stops. Ideally we want to avoid the synchronisation of these fields during cron, so that scheduler can process and publish all nodes (and thus invoke all hook_scheduler_api calls and any rules which might be triggered). I tried to use hook_i18n_sync_translation() which can act on the translated nodes, but this is called only after the translations, so the data has already been altered.

So neither option 1 nor 2 above were appropriate. Hence the solution I decided on was to store the initial value of publish_on for the nodes being processed in _scheduler_publish(). Then during the subsequent loops, if the node's publish_on date has been removed it can be restored from the stored array. This allows the node to be processed as normal and any hooks or rules will be triggered properly using the correct date.

The same is done when unpublishing. This highlighted a pre-existing anomoly in _scheduler_unpublish() where part of the processing was done inside an if-block testing for $status==1. This is unnecessary, and was left over from a much earlier version of the code (probably when publishing and unpublishing were done in the same function). I have removed this check in the patch, because it prevents part of the processing of the subsequent nodes. It's been wrong but unnoticed (although having no effect) for a while, so good to get it tidied up, particualrly as it causes problems here.

Incidentally, the database statements are also completely redundant as these are now done in scheduler_node_update(). They can be removed, but this is not needed for this fix, so can be done in a separate patch.

Another problem I found was in scheduler_i18n_sync_options() where the two scheduling fields are defined regardless of the content type. They shoudld only be defined if publishing and/or unpublishing are enabled for that content type. The second parameter holds the type, so this is easy to fix, and is included in this patch.

Attached is a patch against 1.1+15 of 25th Sept.

Here is a re-roll following the changes made to .module in #1566024: Coding Standards and Coder Review

However, the patch does alter the code at the same places as #2011692: Prevent cron errors when scheduled nodes no longer exist and #1898784: Make log revision text translatable so whichever is committed first, the others will need to be re-rolled.

Patch needed re-rolling due to #1898784: Make log revision text translatable being committed in dev 1.1+30

Re-roll following 1.1+34

The original patch committed in #10 has a mistake: it accesses the publication properties from $node->scheduler['publish_on'], instead of from $node->publish_on.

I discovered this while writing tests for #1979460: Allow other modules to prevent scheduled nodes from being published / unpublished. These tests were mysteriously failing because I made the same mistake! (ref comment #3 of that issue). This got me intrigued as to why these properties are available twice, and I found it is for backwards compatibility with the Drupal 5 and 6 versions of Scheduler (ref. Compare D7 hook_node_load with D6 hook_nodeapi(op=load) comment #12).

This is very confusing, and using $node->scheduler['publish_on'] is just plain wrong, these properties are nowhere used inside Scheduler, so changing them will not have the desired results.

It is too late to remove these duplicate properties now, as it will break websites that rely on them, but we should definitely get rid of this for the D8 version, and the affected code of this issue should be fixed.

I've taken a look at what Jonathan needed to do to get this functionality to work properly, and it feels to me that if such major changes are needed to the core functionality of Scheduler then we probably need a different approach. I feel this should work without requiring any changes to _scheduler_publish() and _scheduler_unpublish().

Going to mark this as a release blocker. We should definitely not create a new release with this broken implementation in it.

We should figure out what is really going on here, is perhaps it is due to the node_save() in i18n_sync_node_translation() that triggers scheduler_node_presave() again, causing the node to be (un)published? I'm just making guesses, but somehow the node is changed in the middle of our scheduling process. It should not be our responsibility to act on that.

Maybe we should revert the commit for this issue for the time being while we find a good solution? Also seeing how tricky this issue is we should perhaps do this in a separate glue module? That would keep the majority of our users away from harm, while the people that need this can enable the glue module.

Added a warning and todo's in commit 8a5b624.

Thank you for spotting this - I did not notice that PieterDC had used the wrong properties in the original patch in #3. It is easy to correct that. Reading my comment that you found, I think those fields must have been added in Drupal5 to allow themes to use the formatted date strings, but I do not know the reason why all four items where put into a 'scheduler' array. Yes, let's remove the two duplicated values from the array in D8.

This is very confusing, and using $node->scheduler['publish_on'] is just plain wrong, these properties are nowhere used inside Scheduler, so changing them will not have the desired results.

I agree that the code should be changed, but the incorrect properties where only used as source, they were not being set so that is why all testing worked ok and the values were copied to the new node correctly. But yes, it should be corrected.

We should figure out what is really going on here, is perhaps it is due to the node_save() in i18n_sync_node_translation() that triggers scheduler_node_presave() again, causing the node to be (un)published?

Yes, i18n_sync_node_translation() copies the values from the node being changed to its translation node(s) and calls node_save() which in turn invokes scheduler_node_save(), which updates/deletes the row in the scheduler table. That is exactly the problem. I agree that my fix, whilst simple to understand, may not be the proper solution. I did try to do it another way (see my point 2 in #11 and the first paragraph of #12).

The ideal solution is for the publish_on and unpublish_on fields not to be synchronised during a cron update, but only when doing a normal node edit. Function i18n_sync_node_translation() gathers information on which fields are to be synchronised. I will look again the hooks provided by i18n_sync to see if there is a way to modify the list and only add the scheduler fields if not running via a cron run.

Maybe we should revert the commit for this issue for the time being while we find a good solution?

I would say leave it in for now. It's only in the -dev release. I will update on my findings, then we can review the decision again.

Jonathan

I've found two other ways we can solve this problem. The simplest is to call the function i18n_sync(FALSE) at the start of scheduler_cron() and i18n_sync(TRUE) at the end. This tells i18n_sync that node synchronisation is disabled, which is exactly what we want during the scheduler cron run. Nothing we do to any node should affect the translations. I have tested this and it works exactly as expected.

Just for reference (and to record my research) the second method would be to add more information into the $options array in scheduler_i18n_sync_options, specifically we can (if running via a cron job) add a 'field_name' entry to publish_on, which will have the effect of attempting to use this as a callback function when doing the translations, instead of just blindly copying the data. If that function does not exist (say, we call it scheduler_do_not_sync) then we can selectively not sync our fields during cron but continue to sync them on normal node edits. This has a slight disadvantage in that any other fields (for example, node->status) will still be synced during cron, and that may have a detrimental effect on rules or actions which are intended to be triggered by scheduler doing the changes.

Here is a patch against 1.1+35 using the i18n_sync() switch, and also chaning $node->scheduler['publish_on'] to $node->publish_on etc.

Jonathan

Here's the patch against 1.1+37 - I forgot the last commit added @todo in the middle of the function I was fixing.

I think that if you need to add module-specific code to scheduler to make third party modules work, then this is a clear sign that is something lacking in the Scheduler API. It should be possible to solve this without making a single change to Scheduler. There might be other modules that do a node_save() after being invoked by hook_node_update(). We need a hook!

I'm going to look into this soon. This will be my next priority after I have completed my current iteration on #1979460: Allow other modules to prevent scheduled nodes from being published / unpublished.

Yes, I see your point about not using third-party functions. But in this case, we are utilising the function specifically provided by i18n_sync to assist in this situation in a simple way. If I added a @todo to remind us to re-write this when exporting to D8 would you be ok if we committed this to D7?

I've had some further thoughts on this and I do not think it can be solved within Scheduler alone if we do not want to call the i18n_sync() switch directly. But there is a very neat way to do it if i18n_sync were to provide a hook to allow other modules to control whether sync should be done. I have written the hook and posted #2136557: Provide hook for modules to turn off synchronisation to the i18n issue board.

This gives flexibility to either turn off all syncing during our cron runs, or turn it off for the nodes we are processing, but leaving possibly other nodes (which might be updated via rules or anything else) to be synced if required.

Currently in Scheduler, the sync process is broken. Depending on what response we get from the i18n sync team, it might be a case of reverting the commit in #10 and then re-rolling the patch with all the necessary code.

Let's go a step further...
1. OP clearly means D6 'content translation', but nowadays we are way in D7, almost in D8, and most people use 'Entity Translation'.
2. current scheduler contains its own 'date' input, and uses hook_node_operation(). So scheduler is not ready to use custom entities (or terms, users).

So, let us consider the following: let Scheduler use the Field API, and break it up in pieces:

1. Scheduler adds a (new) 'scheduler' field type, or (even better) can re-use an existing Date field of an entity.
2. Scheduler (already) has cron capabilities to read the date, and to start cron when date has started/ended.
3. Scheduler contains (the current) 'default' action to set the 'published' property, to preserve the existing functionality
4. Scheduler contains (new?) hooks to integrate with Actions, Rules or custom code, to set whatever you like

Using the Field API solves this issue, and opens up all entity type for Scheduler!

-

Hi johnv,
Thanks for your ideas. We might review the merits of using Field API for the D8 version of Scheduler, but that is not going to happen in D7. Regarding integration with Rules and Actions, the issue #773510: Integration with Rules module is just about to be committed.

I think we need to concentrate on resolving the unwanted date sync during cron, then we can get this issue resolved.

Thanks again for your suggestions, and I'm pleased you are taking an interest in Scheduler.

Jonathan

Here is a patch to fix the two separate problems with the scheduler code, scheduler_i18n_sync_options() as explained in the last paragraph of comment #12 and scheduler_field_attach_prepare_translation_alter() as explained in comment #16. I think it would be good to get these committed, then we can concentrate on the final (difficult) problem of unwanted date syncs during cron.

Jonathan

Committed the two small bug fixes from #30 in commits d372aab3 and 9a06c40, thanks Jonathan!

Now we need to tackle the syncing problem. Yay :)

Thanks for committing those two fixes.

So, to summarise the final problem - When a scheduled node and its translation(s) are being published in a cron run, the update to the first node is processed ok. This removes $node->publish_on from the first node (ok) but during the publish and save actions, the translated node(s) also get $node->publish_on removed. Then in cron, when processing the subsequent node(s), we get an error that publish_on is an undefined property.

I have identified five possible ways to solve this so far:

1. Given the fact that the second node becomes published automatically due to synchronisation, could we just leave it to be done that way, and not process it via Scheduler. We could check to see if $node->publish_on is still set before processing the subsequent node(s) in the loop.
2. Allow the dates to be removed during sync, but store and recover them via extra functionality in _scheduler_publish() and _scheduler_unpublish()
3. Inhibit synchronisation by calling the isync function i18n_sync(FALSE) at the start of scheduler_cron() and i18n_sync(TRUE) at the end. This tells i18n_sync that node synchronisation is disabled during the scheduler cron run.
4. To prevent the syncing we might be able to add to the $options array in scheduler_i18n_sync_options(). Specifically we could (if running via a cron job) add a 'field_name' entry to publish_on, which will have the effect of attempting to use this as a callback function when doing the translations, instead of just blindly copying the data. If that function does not exist (say, we call it scheduler_do_not_sync) then we can selectively not sync our fields during cron but continue to sync them on normal node edits.
5. There would be very neat way to do it if i18n_sync were to call a hook to allow other modules to control whether sync should be done on a per node basis. This has been requested in #2136557: Provide hook for modules to turn off synchronisation

The problem with (1) is that implementations of hook_scheduler_api() will not be executed for the subsequent nodes. Nor will any rules which to react to Scheduler publish/unpublish events.

Option (2) works, but it is messy coding, and adds complexity to the two _scheduler functions which is not used in the majority of cases. (see comment #12 for the code for this)

Option (3) works well. It tells isync to do nothing to the translated nodes when Scheduler operates on the first node. This is exactly what we want - as nothing we do to any node during scheduled cron publishing should affect the translation nodes directly. A patch for this code is in comment #20. One disadvantage of this solution is that we are calling a function directly which owned by isync, so there is a tiny possibility that it may change. We could ask the isync maintainers about likelihood of this.

Option (4) is complex, and not actually tried yet. It has a disadvantage in that any other fields (for example, node->status) will still be synced during cron, and that may have a detrimental effect on rules or actions which are intended to be triggered by scheduler doing the changes.

Option (5) would work well if the maintainers of isync implemented the change code I discussed in comment #27 above and posted in #2136557: Provide hook for modules to turn off synchronisation. This gives flexibility to either turn off all syncing during our cron runs, or turn it off for the nodes we are processing, but leaving possibly other nodes (which might be updated via rules or anything else) to be synced if required.

To sum up, (1) and (2) are non-runners, (3) we could implement now, (4) has not been tried yet, (5) we have to convince the isync maintainers to call the hook from their i18n_sync_node_check()

Thanks for the great write-up! I was apprehensive of this issue because of the complexity and have been postponing it for weeks, but this makes it very clear.

I guess the most practical solution would be to go with option 3 for now, and add a @todo linking to a followup issue to solve this neatly using the proposed hook in 5. Then at least we can move on with this issue for now. This is currently the only release blocker.

Yes I think that option 3 is actually a reasonable solution. But we could develop option 4 or option 5 later in a new issue.

Here is a patch which implements option 3 and includes a TODO referencing both the existing issues.

Awesome, thanks a lot! I shortened the documentation a bit and committed it to 7.x-1.x: commit d6a449b9. Very glad that this is in!!

  // During cron runs we do not want i18n_sync to make any changes to the       
  // translation nodes, as this affects processing later in the same cron job.  
  // Hence we save the i18n_sync state here, turn it off for the duration of    
  // Scheduler cron processing, then restore the setting afterwards.            
  // @todo Replace this workaround with a hook implementation when issue        
  //   #2136557 lands.                                                          
  // @see https://drupal.org/node/1182450                                       
  // @see https://drupal.org/node/2136557

Awsome indeed, this is the final big issue before we release 7.x-1.2

[edit: The link above is now working fine - must have been a temporary fault]