Exclude node types that do not have scheduling enabled

This is a good idea, thanks Pieter for creating this issue. It makes more sense to eliminate the nodes at the start, not have to filter them later.

When changing the queries we should also take the oportunity to use the condition ->exists('publish_on') instead of ->condition('publish_on', 0, '>') as thid is safer and more generic. The test for greater than zero was left over from previous releases where we stored empty dates as 0, when they should have been null.

Secondly, we should order the nodes in a pre-determined sequence so that repeated testing gives the same expected order. I suggest we order by date then by node id, ie

    ->sort('publish_on')
    ->sort('nid')

Added the changes as suggested in #2

Forgot to perform a diff against the 8.x-1.x branch.

Yes, those additions are good, but you have forgotten the actual, original reason for this issue:

adding a condition to the query to only include node types enabled for the action being processed.

This issue was created so that we eliminate the nodes at the start. It is just a minor efficiency improvment. I guess the way to do this is to create an array of all enabled node types and add that as a condition.

The tests currently pass because for safety we throw the exceptions, as you can see if you run locally with verbose debug, as I added a call to the dblog page.

I created a helper function to retrieve the node types that have scheduler enabled (meaning the node_type has the publish_on and/or unpublish_on set to true in the third party settings), so they result of that function can be used in the entity query. If necessary, we can split it out even further (when unpublishing, only select the node types where unpublish_on is set, regardless of publish_on).

Hey, thanks for doing this. First thing I noticed is a typo in SchedulerManager.php

->addField('unpubish_on', 'INNER', NULL)

That may well explain many if not all of the test failures.

Yes, I copied the changes from #5 manually and it shows... Let's try again.

That's good.

If necessary, we can split it out even further (when unpublishing, only select the node types where unpublish_on is set, regardless of publish_on).

This is easy, and we might as well do it, to make the query return only the node types applicable. I passed parameter $action into the new function scheduler_get_scheduler_enabled_node_types then use ($action) to make it available in the callback function. For consistency with the rest of the module, I also changed the default FALSE to $config->get('default_' . $action . '_enable') as we do not hard-code the defaults anymore (see #2633870: Revisit defaults for third party settings and #2682579: Move constants out of global space into settings and schema)

Something is odd. The test failures #11 above do not happen in my local environment, and as far as I can see they are unrelated to the code changes in this patch. They are however, exactly the same set of failures as in #2756147-2: Remove @file tag docblock from .php files where the class matches the filename which I could not understand either.

Also the failures did not alter the status from 'needs review' to 'needs work', the status remains unchanged. Maybe there is something up with the testbots?

OK, I can replicate these failures locally, having downloaded the lastest dev of D8.2 - It looks like something has changed between the passes in #10 on June 24 and the fails at #11 on June 28. More investigation required.

I have created #2757625: Previously passing tests now fail in Drupal 8.2 - Change assertRaw to assertText to deal with this problem.

I have fixed #2757625: Previously passing tests now fail in Drupal 8.2 - Change assertRaw to assertText and requeued the patch in #11 above. All seems fine again.

@mr.baileys are you OK with my changes, shown in the interdiff 10 to 11?

Thanks @mr.baileys for this. I assume you are OK with my changes interdiff 10 to 11 but if you spot something tell me.