Support for multi-lingual translations and differing scheduled dates

Applying the same concept, but for unpublish schedules as well.

This patches seem really palliative, though, as they don't address some possible configuration regarding i18n:

1 - Schedules could either be independent or synchronized among languages, such as https://www.drupal.org/node/1182450.
2 - Should we allow the user to choose which languages CAN be scheduled?

Nice work so far.

Forgot the patch:

Now that I payed a little more attention to the code I see this patch is actually doing something completely unexpected: it is performing a scheduled job to ALL translation that contain a scheduled job, but not necessarily the ones that should be target. For instance, if I say that the English version of a node should be publish on a day, and the Spanish version on the day that follows, both are going to end up publish when the English version gets published.

Last patch had a bug related to unpublishing the node translations. Here goes an updated one.

You have to set the status to 'needs review' then automated tests will run on the patch files you upload.

By the way, we had this functionality in 7.x, and used https://www.drupal.org/project/i18n hook_i18n_sync_options() to give admins the option to always keep the dates in sync for translated nodes. We need to find out if the equivalent code has moved to core for 8.x as we do not want to re-invent things.

I think the code hasn't moved, because without this fix, the cron is broken in D8.

I don't quite understand what you mean. What I was talking about was the question of whether the I18n sync functionality has moved into 8.x core now that multi-lingual support is in core, as there is no i18n module in Drupal 8. The functionality to keep selected custom fields synced between translations sounds like a basic requirement which might be available without us having to write it for each contrib module.

The test is wrong. Doesn't make sense to publish a node if the publish_on value is less than REQUEST_TIME

In your patch, the changes for src/Tests/SchedulerFunctionalTest.php simply have the effect of always skipping the assertion for 'node is published after cron'. Maybe you did not mean this, but all you have done is remove the assertions which were failing. You can't just do that ;-)

Can you explain why you think the test is wrong?

if you watch the $node->publish_on->value BEFORE and AFTER the db_update behind, the value keep the same

It will not have changed because the db_update does not affect the $node object directly. To see the effect you need to clear the cache then re-load the node. Here is an example of this code, from SchedulerNonEnabledTypeTest.php:

    // Reload the node.
    $this->nodeStorage->resetCache([$node->id()]);
    $node = $this->nodeStorage->load($node->id());

Then you should be able to see that the value has changed.

This version is to work with this patch (https://www.drupal.org/files/issues/2790459-5.move_rules_to_submodule.patch) applied.

Small fix for the patch from comment #45. This patch only will work if the patch posted here: https://www.drupal.org/files/issues/2790459-5.move_rules_to_submodule.patch has already been applied.

The latest Patch seems to be working only on Publish, not unpublish

        // Do not process the node if it still has a publish_on time which is in
        // the past, as this implies that scheduled publishing has been blocked by
        // one of the hook functions we provide, and is still being blocked now
        // that the unpublishing time has been reached.
        $publish_on = $node->publish_on->value;
        if (empty($publish_on) || $publish_on <= REQUEST_TIME) {
          continue;
       }

This code seems to be doing the opposite form witch its intended. If empty() will make the the loop jump out and go to the next time.
I've changeed it to:

if ($publish_on) {
          continue;
        }

That way it doesn't matter is the publish on is older than request time or not.
We also need to look if the unbuplish_on value is set BEFORE triggering the event otherwise there is not point in even doing it since the node has several languages.

if(empty($node->unpublish_on->value)){
          continue;
        }

Hi Kopin and adrianopulz,
Thank you for your new contributions to this thread. However, I am still very sceptical about this whole approach. It should not be Scheduler's job within the publish/unpublish cron functions to be making decisions on what other nodes to process. That should all be done way before the cron job processing. Nodes should be scheduled, then the cron processing simply picks up the nodes that are due now.

Please see my comments in #11 and #13 as these have not been addressed. The syncing of node data should really be out of scope of Scheduler altogether, so the whole approach here may be wrong.

Also, my question in #29 was not resolved. I believe that the current test is correct, and the change made to the test in this patch simply removes the failing assertions which are caused by the code changes. This gives a false impression that the code changes are correct. The point about using node_save() and not db_update() is a slight diversion here. The tests in this class are to check the most basic functionality of Scheduler, without doing anything more than the minimum to prove it. I may look into using node_save() here, but that is a separate issue.

We need to use pre-existing functionality in D8.x (either core or contrib) which cater for field data sync.

lucas.constantino had some very good observations in #6 and #8 regarding what this patch is doing, and questioning why. I think we need to step back and make those decisions at a higher level first, then we can decide on how to achieve it.

In our case we NEED to be able to Publish and unpublish different translations at different times. I think it's a functionality that should exist. The content is not always translated right away and published at the same time.

Without this patch, even if you are not publishing any translations, the basic scheduler functionality doesn't work with multilingual enabled on the site.

In our case we NEED to be able to Publish and unpublish different translations at different times.

Absolutely, I totally agree.

Without this patch, even if you are not publishing any translations, the basic scheduler functionality doesn't work with multilingual enabled on the site.

I will investigate and try to find a different method to solve this, as I do not believe that we need to get all translations of a node during the cron processing. It should be driven by the scheduling dates already on the nodes.

I now realise the fundamental difference between D7 and D8 multi-lingual sites. In 7.x each translation was a different node with a different id, but in 8.x all translations are associated with one node id.

There was nothing wrong in principle with the functional test, apart from the fact that the node_field_revision table now also needs to be updated in addition to node_field, as the ->getTranslation() method uses this. I may re-write that test completely (it was the first one we wrote in 7.x and has been converted verbatim to 8.x). For now, I have made a quick fix commit to update node_field_revision.

Attached is a patch, now just for the SchedulerManager.php file. It is mainly doing the same as the above patches. Two small changes to unpublish()
(1) Need to skip not just if there is no unpublish date on this translation, but also if there is a date the future.

// If the current translation does not have an unpublish on value, or it
// is later than the date we are processing then move on to next.
$unpublish_on = $node->unpublish_on->value;
if (empty($unpublish_on) || $unpublish_on > REQUEST_TIME) {
  continue;
}

(2) the test for a publish_on date should only skip the processing if the date was in the past. This is to cater for the future posibility of allowing a publish_on date to be after the unpublish_on date, that is, to allow re-publishing.

// Do not process the node if it still has a publish_on time which is in
// the past, as this implies that scheduled publishing has been blocked
// by one of the hook functions we provide, and is still being blocked
// now that the unpublishing time has been reached.
$publish_on = $node->publish_on->value;
if (!empty($publish_on) && $publish_on <= REQUEST_TIME) {
  continue;
}

I also changed the sequencing of tests, to first skip translations which are not due to be processed. It is better to get these out of the way, and not call other hook funtions on content we know right away will be ignored. I also added better commenting.

Please test this patch and let me know if it continues to work for you. In the meantime I will start writing automated tests for the translation processing.

Hi jkamizato,
Did you make a comment? If so, it is blank. It would be good if you can test the patch in #56.

Patch #56 is working for me. Thanks!

Thanks for marking RTBC. However, before I commit the changes I want to add tests. This patch should fail with the current code as it only contains the test file, not the fixes in #56

As intended, the new test fails without the code fix.
Here's the patch with both.

Thanks jkamizato for starting off this issue. The main cron processing to publish translations is now committed.

All is fine when the scheduling dates are set to be translatable and independent on each translation. However, I discovered a slight problem when the date is set to be not translatable - this is where a scheduler date entered on one translation is copied to all other translations. This works fine, but our automatic unpublishing of a scheduled node, which is done in hook_node_presave() is only executed for the translation being saved. The status of the other translations is not changed. Hence we can get the situation where a new translation is scheduled to be published, the date is set on all translations, but the existing published translation remains published. This could be said to be an admin/setup problem. If you want every translation publish-on date to be synchronised then you should also set the node status to be synchronised too. That solves the problem.

Does anyone have thoughts on this? We could give a message if the dates are synchronised but status is not set to be synchronised. I do not think we should force both settings. Adding code to hook_node_presave() could be done, but that may be over-complex.

I have created a follow-up #2871164: Ensure Scheduler dates and status field have consistent translatable settings
This original issue can be now be marked 'fixed'.

Thanks everyone for your help on this. Please check the follow-up issue, I would like to hear the opinions of those who actually use scheduled date translations on real sites.