Admin setting 'Enable content type for scheduler' is ignored

I don't think we can conditionally add the fields when a realtime action of enabling the content type for scheduler is performed. My suggestion is that instead of hook_entity_base_field_info we use hook_entity_bundle_field_info as this has knowledge of the 'bundle' which we can use to retrieve the enabled setting.

On the API page it says

Bundle fields either have to override an existing base field, or need to provide a field storage definition via hook_entity_field_storage_info() unless they are computed.

So one idea might be to leave the entity definition as it stands, but just turn it on or off in the bundle function. Or maybe that's making too much work?

Something else to be aware of: This hook will be changed in https://www.drupal.org/node/2346347

Amazing timing! I am working on this right now and discovered that we cannot simply move the definition of the fields from hook_entity_base_field_info to hook_entity_bundle_field_info. When I tried that, the fields quite rightly did not show for non-enabled types, but for enabled types the data could be entered but was not saved.

Then I spotted that $form['publish_on']['#access'] was set in _scheduler_form_alter which is only called if one of the options was turned on. The fields are correctly hidden if at least one of the options is on, it is when both options are off that we get the default date entry fields showing up, in plain form just from TimestampDatetimeNoDefaultWidget.php with no custom descriptions, etc.

It would seem that the default #access value is TRUE. So I was just trying out setting $form['publish_on']['#access'] = FALSE in scheduler_form_node_form_alter for the case when we do not call _scheduler_form_alter. But I see from the changes in #2597233: The FALSE value for #group FAPI property does not have expected behaviour that if we call the helper form_alter for all cases then that might also solve the problem. I will test it both ways and let you know.

The change in #2597233: The FALSE value for #group FAPI property does not have expected behaviour does fix the problem of making sure the date input fields are not accessible for non-enabled content types. However, as explained in the first paragraph of https://www.drupal.org/node/2594615#comment-10725014 there are still a couple of fixes required in the cron process to make sure we do not act on non-enabled nodes. I have tested these changes both manually and via automated test results at https://www.drupal.org/pift-ci-job/131554

The attached patch is for reviewing and commiting here. The tests will not pass on this run, as the commit to the test files has not been done yet.

I have just re-tested, and the patch in #8 still applies and fixes the remaining problems. Now that #2597233: The FALSE value for #group FAPI property does not have expected behaviour has been fixed we get 42 passes and 0 fails, and the test class is all green

3		Scheduler.Drupal\scheduler\Tests\SchedulerNonEnabledTypeTest
✓		- setUp
✓		- testNonEnabledNodeType
✓		- checkNonEnabledTypes

I'd like someone else to check my patch, before I commit, as it is bad practice to make changes without another reviewing it.

I have addressed the @todo relating to how to cope with a node where the relevant date has been removed or somehow the db table is out-of-sync with the entity cache. We need to do this to avoid a cron run crash because $node->set('changed', $publish_on); fails badly but silently if the date is null. In the patch in #8 I had substituted the current time, but I now think that this is wrong. We should simply skip processing the node if something unexpected has happened to the data. Should we add a logger message to say that the node has been skipped? This would assist the author/admin to discover what has gone wrong.

The tests will need adjustment, because currentlty they fail even with this modified patch. I will look at that next.

Having fixed the tests we now get a full set of passes in SchedulerNonEnabledTypeTest using the altered cron code in #12 which abandons processing when the date is missing.

Here is an updated patch which does the same processing as in #12 but writes a log message to explain what has been done.

Good, we get 14 class passes and only 4 fails as expected. I have also tested each of the scenarios manually to check the dblog messages. All OK.

Setting back to 'needs review' to encourage others to review the patch before I commit.

OK. I have worked out how to get the proper field label. This was a @todo in the latest patch, can't upload a new patch right now, but will do later. It won't affect any review you do now though, that is worth doing, thank you.

Here's an updated patch showing the type name and field name instead of the machine names. Also interdiff from 16 to 22 in case you have started looking at the patch in #16

Thanks for the review, Pieter. In reply

1. Yes, I will add a @todo in the code and reference the API service issue
2. Good idea to improve the query, I will add @todo
3. "Not 100% sure if it is a good idea to log this. " When the query is improved, this code can be removed and we will not be logging it on every cron run. But I suggest that we leave the current patch code as-is for now, until the query is changed. Or alternatively we silently skip the node without any message
4. How would thowing the exception work? It is only during a cron run that we detect the bad data. Or can we do it a different way? Happy to learn what you had in mind

Jonathan

I've thought a bit more about the conflicting problem of (a) alerting the user/admin that the node will not be processed and explaining why, so that they understand and can fix it without raising support issues, versus (b) clogging up the dblog with a message every time, and leaving the node scheduled for eternity.

A way to solve this could be to log the error then remove the publish-on date from the node, so it does not keep getting selected. After all, the date should not be on the node in the first place. If the date got left on when the node-type had been enabled in the past this would be a relatively easy way to tidy up the bad data and continually add dblog rows. A possible danger of this is if the admin accidentally disabled scheduling and before they noticed their error a cron run cleared all the dates. Maybe the removing of dates should only be done if the scheduled date is a week in the past, that way the admins gets plenty of warnings in the log but it does not continue for ever.

Just putting ideas out there ...

Yes, I see now why we should not rely on the dblog and should throw exceptions instead. Also for the distinction between site admins and developers messages. Thank you Pieter for the detailed explanation. You are also right that we should not remove the dates as that is defintely a form of data-loss.

Here's a re-roll addressing all the points in #24 and #27. Here are the differences from the patch in #16 that you have reviewed. I've also added an interdiff for 16 to 28

1. Added @todo about converting logger to service, and referencing #2651338: Create a service for the Scheduler API
2. Added @todo about improving the entity query, referencing #2659824: Exclude node types that do not have scheduling enabled
3. Changed both error traps to exceptions. The first one will still be required even when the entity query is improved, as the API calls may add in un-enabled nodes. I used sprintf() to format the text as suggested here. This seemed cleaner than concatenating the text values because I needed to include quotes in the message
4. Moved the creation of logger_variables array to be next to the single logger call. This will all be changed later
5. Repeated all the above for the second function

I'm not sure if the exception class definitions needs any more documenting. The examples seem fairly brief, but let me know if you think it need more. Also, is it correct to define them in the file they are used, i.e. cron.inc, or should they be somewhere else common to any other exceptoins we might add in the future?

Good - as expected this test class is fully green with 58 passes and no fails:

3		Scheduler.Drupal\scheduler\Tests\SchedulerNonEnabledTypeTest
✓		- setUp
✓		- testNonEnabledNodeType
✓		- checkNonEnabledTypes

That's OK. I think I have covered all your points, and I'm happy that the exceptions are working OK. Would appreciate your review of the patch in #28. I've given the interdiff from 16.

This issue will fix the final non-passing test case. It will be good to have the committed codebase passing all green, as that will assist our general workflow on all other issues.

As there have been no objections (yes ok, no reviews either) in the month since I posted the patch in #28 on 8th Feb, in the interests of keeping things moving I decided to commit this. I will happily make amendements if the programming needs it, or to bring in line with coding standards.

... and now we have the committed codebase passing all test classes :-)

Unassigning.