Missing validation allows Unpublish date to be blank

Here's the new test, but without the new validation. This patch should fail one test.

Here's the patch with the new validation and the test. Also removed two debug messages from the tests, which do not need to be there.

Hi AlexKirienko,
Yes, you are right. If the 'unpublishing required' option is turned on, then publishing the node directly should come under the same validation rules as setting a publish-on date. In both cases the user should enter an unpublish-on date. The code change you made in sheduler_node_validate() is not quite right because you have altered the existing validation. We need to retain the check for !empty($node->publish_on) but also add the new check against $form_state['values']['status']

The changes for removing validation when deleting a node have already been fixed in other issues, so that should be OK.

This extra validation needs to be changed at 8.x first. Here's a patch with an updated test to check for it - the class testValidationDuringEdit should now fail one assertion.

As expected the new assertion fails. Here's a patch with the additional constraint and the new test. This should pass.

[By the way, it's really nice to be able to test patches at a selected version, and not have to change the issue version. I think this is a relatively new feature]

That is a bit strange. The new test class passed, but the failure was in a completely different class which was not altered by the patch.

Show: Failing classes All
✓	✗	
34	1	Scheduler.Scheduler
1	1	Scheduler.Drupal\scheduler\Tests\SchedulerPermissionsTest
✓		- setUp
	✗	testUserPermissions

fail: [Other] Line 44 of modules/scheduler/src/Tests/SchedulerPermissionsTest.php:
The node was created and saved when the user does not have scheduler permissions.

That shows the benefit in having a good test coverage. More investigation is required ...

OK, I can see what has happened. The new constraint has prevented a user from creating a published node if the user does not have scheduling permissions but the 'unpublishing required' option is enabled. That is good! If a user cannot set an unpublish date because they have not been granted scheduler permissions then it is correct that they should not be able to create a published node, as that node will not have an unpublish-on date.

I think the correct result in this case is that the user should only be allowed to create an unpublished node. Other user roles would then be able to schedule the node. This may require a change to the message text, as the user would see "To publish this node you must also set an 'unpublish-on' date" but they would not have access to the unpublish-on field. But the principle of not letting them create/save a published node is correct.

Updated patch with the following changes:

1. Changed the text of the new validation message to "Either you must set an 'unpublish on' date or save this node as unpublished."
2. In SchedulerValidationTest, as well as checking the validation message text, I added additional assertions to check that the node is not saved
3. Improved comments in SchedulerUnpublishOnConstraintValidator
4. Changed SchedulerPermissionsTest so that it does not test with the 'required' options set
5. Created #2707411: Conflict between 'required publishing' and not having scheduler permission to deal with this problem, and added @todo in the permissions test, referencing that issue

Now a patch for D7 with the new validation (as per 8.x), updated testValidationDuringEdit() and a new test function testPermissions() which we did not have at 7.x before.