Validate that the publication date is in the future

+++ b/src/Plugin/Validation/Constraint/SchedulerPublishOnConstraint.php
@@ -0,0 +1,37 @@
+   * @var string
+   */
+  public $messageDateNotInFuture = "The 'publish on' date must be in the future.";

I think this should be protected

I was using the core comment module validation (CommentNameConstraint.php) as a reference. The message variables in there are public.

Happy to update it though.

#2490570: Validate that the 'publish on' value matches the expected format
#2490574: Validate that the publication date is in the future
#2490576: Validate that the unpublish date is in the future
#2490578: Validate that the 'unpublish on' value matches the expected format
#2490580: Validate that the unpublish date is later than the publish date
#2490584: Validate that unpublish date is entered if publish date is entered

The patches for many of these validation issues clash with each other. They modify scheduler_entity_base_field_info
and many add the same validation name. Also they all create the same new file names.

Whats the best way forward with this? If each validation file is separate then they need to have distinct names, or maybe I have mis-understood it (which is very possible!)

Pick any issue which is on priority. If other issues are conflicting with this, then those can be postponed till this gets in and then just need to re-rolled the patches (if issue still valid).

I thought that there would be one validation constraint for each the publish_on and unpublish_on values. The only differences between these issues should be the two constraints and the checks within each constraint.

I've create a meta issue #2629820: [meta] node edit validation as parent for all of the validations

Patch did not apply. Re-rolled for 8.x dev of 2015-11-23

Changed the message variable name from $messageDateNotInFuture to $messagePublishOnDateNotInFuture. I know that the publish-on and unpublish-on messages will not be stored in the same class, but I thought it would be better to make the names explicit anyway, given that some of them already have the name in.

With ref to #3 and #4 I did not change it from 'public' to 'protected' but happy to go either way depending on what the consensus is. Do we know if the core comment moduld was following Drupal standards? According to the example in the Entity level validation constraints change record, it is public.

Thanks for re-rolling the patch and fixing the variable name. I would have thought the comment module was following the coding standards, and if the documentation has it as public, then I guess we're good :)

Now that the testing branch has been merged, this involved some changes to non-test files, so the patch in #9 no longer can be applied. But we will see the benefit of the new testing. This fix does not quite solve the problem ... here's the original constraint, re-rolled.

forgot to change to 'needs review' to allow testing.

As expected, we got 8 passes and 7 fails.

✗	testSchedulerPastDates
fail: [Other] Line 108 of modules/scheduler/src/Tests/SchedulerFunctionalTest.php:
An error message is shown when the publication date is in the past and the "error" behavior is chosen.

The reason that the constraint did not fix the failing test was that no default was provided for the getThirdPartySetting call, so our presumed default of 'error' was not the default, and blank was returned. For the main scheduler common settings we can (and do) supply defaults in scheduler.settings.yml but I do not know how to the equivalent for content-type third party settings. This default setting of 'error' is used in three places, split over three different files, and I consider it too much of an overhead when coding, to try to remember which is the default value. As a first solution I suggest we define constants for our defaults, and use them instead of replicating the hard-coded string or integer or Boolean, or whatever.

Another idea is: can we override/intercept calls to getThirdPartySettings and if null is returned we set our own default? I'm not sure that's possible or even desirable, just putting the idea out there.

For now, here is the patch re-rolled utilising the constant method. Instead of 8 passes and 7 fails we should now get 9 passes and 6 fails, with testSchedulerPastDates going from red cross to green tick (with a bit of luck)

That's odd. I was not expecting a fatal error, it runs cleanly on my local machine.

20:39:53 Fatal error: Call to a member function id() on a non-object in /var/www/html/modules/scheduler/src/Tests/SchedulerTestBase.php on line 58
20:39:54 FATAL Drupal\scheduler\Tests\SchedulerFunctionalTest: test runner returned a non-zero error code (255).
20:39:54 Drupal\scheduler\Tests\SchedulerFunctionalTest                 0 passes   1 fails                   

That function is nothing to do with the change, or the constraint. It should have worked just as before.
Lines 57 and 58 are:

    $node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
    db_update('node_field_data')->fields(array($key => time() - 1))->condition('nid', $node->id())->execute();

So it looks like the node did not get created correctly, and hence did not return an object. Gonna resubmit for testing, just to check it was not something else which caused a problem.

Can't see how to re-test an existing patch (I guess it might be limited, time based?) Here is an identical one, to simulate a re-test.

OK so it wasn't a glitch, we got the same bad result. Going to leave this and have a think about what to do. It runs fine on my local machine, but maybe the node create is somehow affected by the new constraint, which is not happening on local. I don't konw. If anyone has an idea, let me know!

Just re-checked and it turns out I was only running selective tests on my local machine and did not actually try this test. I did notice in one other place, in a different test, that the new \Drupal::service('date.formatter') was giving a different result from the old format_date() so I wonder if we are now inadvertantly sending a date in the past to helpTestScheduler. It could be that this new constraint might be doing a very useful job indeed.

OK, the problem is that a past date is being sent to helpTestScheduler when we expected it to be a future date. This caused the new constraint to trap the error and prevent the node from being created. I have some ideas about that, to do with date.formatter vs format_date and the default timezone which we are not catering for. However, that can all be addressed in a separate issue, first we need to protect the code and avoid the fatal error of trying to use a node id which does not exist. This should allow the full set of test to complete.

    $node = $this->drupalGetNodeByTitle($edit['title[0][value]']);
    if (empty($node)) {
      $date_time = $edit[$key . '[0][value][date]'] . ' ' . $edit[$key . '[0][value][time]'];
      $this->assert(FALSE, t('Node with %key = @date_time was not created.', ['%key' => $key, '@date_time' => $date_time]));
      return;
    }

This new patch adds a check (shown above) that the new node has been created and can be found as expected, and fails with a FALSE assertion instead of crashing. This will mean that testScheduler will have a fail (where previously they all passed) but hopefully it will also show that testPastDates now has all passes, which is exactly what this constraint issue was designed to fix.

Thats better! Now we get all tests running, have avoided the crash, and we get testSchedulerPastDates with an overall pass. So the constraint is working as expected, in both manual testing and automated testing :-)

I think this is good to commit, but I'd like some feedback on my idea of using the constants for getThirdPartySetting default values.

The new issue is #2630836: Default timezone vs User timezone in automated tests

Sorry that you had so much trouble with this but glad you got to the bottom of it and that the constraint worked. i’ll try and take a look at the default timezone issue to see why the date formatter service is returning something different.

I’m surprised the default values return blank but I like the idea of using constants for them.

Sorry that you had so much trouble ...

no need to apologise - you did not cause the problem! It's good that the constraint showed up the difficulty. I have made some progress on #2630836: Default timezone vs User timezone in automated tests and would welcome your ideas.

I’m surprised the default values return blank but I like the idea of using constants for them.

Good. Yes I prefer this to having to hard-code multiple ocurrences of the same string. Would it be possible to use an object property instead of a constant?

In #28 I have committed the check to avoid the test crash. So here is the patch from 22 without that change, to make sure we get the same result.

I've created a new issue #2633870: Revisit defaults for third party settings as it needs a code-wide approach, not just a little change within this validation issue.

I think we should hold off committing this constraint until we have resolved #2630836: Default timezone vs User timezone in automated tests because the constraint causes failures on dates which are not 'really in the past'. The constraint is working correctly but the test date formatting is not, and committing this patch will cause more trouble for other issues by giving confusingly false test results.

OK, so we are getting somewhere (slowly). The results (1 = before, 2 = after) show that the timezone problem has been fixed and helpTestScheduler now works, but we've gone backwards and the previously passing validation message for 'publish-on must in the future' is not being detected.

This is the same scenario as we had on #2490580: Validate that the unpublish date is later than the publish date where the tests would appear to be running without the patched code.

Taking a gamble and commit the patch from #29 anyway. Then we can see if the branch test result is any better.

It was not too much of a gamble as I had tested the patch both manually, and via interactive simpletests and via the run-tests.sh script. All gave the correct pass that I was expecting, and now we can see that the committed code also gives the pass - see attached. We get no fail for line 114, compared to the image 2 in #34. I also had a positive response in #26 regarding the default values.

I do not know why the DrupalCI seems to be running on unpatched code - I have looked for other users complaining of this but without finding anything. I could not find a forum in which to ask.

Now that we have a pass on the specific test relating to this constraint and error message this issue is fixed. Thank you to Joe for starting off this work.

Just for reference, there is a problem with patches not getting applied correctly and/or tests being run against unpatched code.
#2634114: Patches not applied correctly for contrib modules, PHP >= 5.4?

This is fixed so setting to Unassigned