Validate that unpublish date is entered if publish date is entered

This is only taken into account when the widget on the form display is set to 'Datetime Timestamp no default' otherwise the current date is set as default. I think maybe we should set this to be default in the module?

Also there's a comment in the node_validate() saying:

The unpublish-on 'required' form attribute may not be set in some cases, but a value must be entered if also setting a publish-on date.

I wasn't sure when this might be? So I stopped the unpublish field from being required, otherwise it never comes to this validation constraint.

Thanks for starting this. The patch in #1 does not apply because there have been some code changes in scheduler.module. Also it needed /dev/null instead of the 'a' files as the two contraint files are new. (or maybe there was a different way to apply do it, but my git apply was failing).

... I stopped the unpublish field from being required, otherwise it never comes to this validation constraint.

-  $form['unpublish_on']['widget'][0]['value']['#required'] = $unpublishing_required;

Actually, this line in edit.inc should remain as-is. If you look back at the definition of $unpublishing_required you'll see that it is not just the content type setting, but also takes into account whether the node is new, the current status and whether it is scheduled for publishing.

  // An unpublish_on date is required if the content type option is set and
  // the node is being created or the current status is published or the
  // node is scheduled to be published.

Here's a re-rolled patch, with only that one change reverted. I've tested the validation in the cases above, and the message is shown as required, so on initial testing it looks like this is working.

I'm not sure of our naming convention with regard to the constraints. This one is called 'messageUnpublishOnRequired' but it is more specific than that. However, this fix corrects one of the failing test classes, so we need to make a start on it, get the new constraint files created, and get one working . I'm happy to make ammendments afterwards to have more specific names, and also when we have worked out how multiple constraints will interact.

I think the error in the patch was due to me working on the same files for the other validation constraint issues so git didn’t see them as new. I’ll go back to the other patches and check that they have ‘/dev/null’.

That’s great that it works with the re-rolled patch, although I still don’t fully understand how it would get to the validation constraint when the unpublish_on field would always be required with the 'unpublish_required' setting.

Hi,

I still don’t fully understand how it would get to the validation constraint when the unpublish_on field would always be required with the 'unpublish_required' setting.

The ['#required'] = $unpublishing_required will set the field to be required in the cases where we know before hand that the field is required, as noted in the comments where $unpublishing_required is set. Yes, in these situations the validation constraint you have added will not get triggered. But in the cases where $unpublishing_required has been derived to be FALSE, the form field does not have ['#required'] and there is no little red star, this is where we need this constraint, because it depends on whether the user enters a publish_on date (where there was not one before the edit). It is necessary to do it this way to allow unpublished nodes to be editted without setting any dates, but to ensure that if a publish-on date is set then so will an unpublish-on date.

On the test branch, this would have been clear, because 5 tests in testRequiredScheduling which are were passing, fail under your original patch. But your patch does fix the one failing test in testValidationDuringEdit so that is great.

With regard to the /dev/null issue, I'd say do not bother re-rolling yet, because all the patches will need re-rolling again when we get the first one for each field committed. I'd like to commit this, and if you are ok please mark it RTBC. Happy to make changes later, if Pieter reviews and sees things we should do differently. My main focus is to get all the tests passing, then we will have a better environment in which to progress the other changes and fixes.

Oh I see now, thanks for explaining it :)

Ok I'll wait until we have those committed and then re-roll them.

Thanks Joe. It is good to get the first validation constraint added.

I copied the comment which was removed from original scheduler_node_validate() into SchedulerUnpublishOnConstraintValidator.php. I think it is worth not losing any code comments providing they are still applicable to the new code.

Ignore the failed patch above. It was queued but not run until the 8.x committed codebase passed all tests. That happened with my commit a few minutes ago, and then the untested patches have suddenly come to life and been run. This issue is already fixed.