Activity Tracker cannot be enabled if there are unpublished nodes

I have optimistically renamed this because I think based on reproducing the error, that you have got unpublished nodes. Is that true? As a workaround, you could:

1. Unistall Activity Tracker
2. Publish all nodes
3. Enable Activity Tracker
4. Unpublish nodes as needed

If you do that, you are going to get that error logged every time cron runs as long as this bug exists.

Test the patch.

We should add some test coverage here. Also should it just cast the bool to int instead of the ternary? Functionally the same but less logic.

Hi. This is my first time adding a drupal test. I'd be grateful for feedback.

This patch contains the test but no fix for the original problem, so I expect this test to fail, as it did locally.

./vendor/bin/phpunit --group tracker
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

............E.

Time: 5.31 minutes, Memory: 182.00MB

There was 1 error:

1) Drupal\Tests\tracker\Functional\TrackerTest::testTrackerEnableModuleWithUnpublishedNodes
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect integer value: '' for column 'published' at row 1: INSERT INTO {tracker_node} (nid, published, changed) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => 
    [:db_insert_placeholder_2] => 1506102203
)

Next, I'll add a patch with the fix and the test, which should pass.

Status: Needs review to get testbot to test.

Here is the patch with the fix. I am casting to int, following the suggestion at #9.

Test #12 passed, so back to RTBC.

Sorry. I don't think I'm allowed to do that myself. So back to Needs Review.

+++ b/core/modules/tracker/tracker.module
+++ b/core/modules/tracker/tracker.module
@@ -72,7 +72,7 @@ function tracker_cron() {

@@ -72,7 +72,7 @@ function tracker_cron() {
       db_insert('tracker_node')

So this code runs in cron, so shouldn't we explicitly trigger cron in that case?

You mean, we should trigger cron in the test?

Yes good point. So I can create a failing test by amending testTrackerCronIndexing(), and don't actually need to test the enabling of the module explicitly.

Here's the failing test.

And here's the test with the fix, so this patch should be green.

Tested #19 against 8.4.4 and it works.

I have some concerns though.

1) How is this a core module with a bug this big in it?

2) On installation, it seems to go through your whole site adding new records for every single node on your website. This seems risky at best for large websites.

3) Anyone can just open the /activity link? What?

Do we need new issues for this?
This really does not seem ok to me ...

@Pascal- Yes, we would need a new issue to overhaul what this module does when it is enabled.

#2951030: Invalid text representation: 7 ERROR: invalid input syntax for integer

Patch looks good.

(Though this is a bit of a WTF in the DB system - can we file a follow-up to either fix or document it?)

I am re-uploading the #19 patch to get a test run.

Nice find and fix!

Crediting @rOprOprOp for finding and reporting the bug.
Crediting @catch for insisting on test coverage and suggesting simpler logic.

Re #24 - you think we need to do something in drupal_schema_get_field_value() to change boolean values automatically to integers. I'm not sure that that is going to be a simple change.

Re #21.1 - tracker is not the most used module - it is not enabled in the standard profile for example. Bugs happen and it's great when they are reported and fixed!
.2 It does not go through all nodes. There's a configurable limit per cron run and this limit is used on module install.
.3 It's a feature of the module - it just lists the content a user has access to on the site. It does not list any content the user could not find with google.

Committed and pushed 713294e96e to 8.6.x and 384488fa5e to 8.5.x. Thanks!