🤖 *This review was written by AI (OpenCode) under human supervision. The responses are from @phenaproxima.* ### Overview We reviewed the telemetry code in `drupal_cms_helper`. The overall design is sound. Events go into a queue. Nothing is sent until the site opts in. Cron sends events in batches. Opting out clears the queue. Below are the issues we found, with responses from the developer. ### Progress Commit [d18ad1ef](https://git.drupalcode.org/project/drupal_cms/-/commit/d18ad1ef68ccc1ad41c0a4ed1e96870cab3f6017), on the `telemetry-fixes` branch, makes these changes: - Fixed: the opt-in event is only logged when the value changes to on. The queue is only cleared on opt-out. - Fixed: the leftover `console.log()` is removed. - Fixed: the XOR line in `onModuleInstall()` is replaced with a plain `if`. - Documented, not fixed: a comment now says `DRUPAL_CMS_TELEMETRY` only accepts `0` and `1`. The code still casts with `(bool)`, so values like "false" still turn telemetry on. Everything else below is still open. ### Decisions for the product owner The opt-in checkbox is ticked by default. This was reviewed by someone familiar with GDPR, and it's on purpose. It nudges people toward yes, but nothing happens until they save. One part still needs a look. The login event sends a hash of each user's ID. That's data about the site's users, not the admin, so the admin can't consent for them. It may need a different legal basis. Module and recipe names are sent as they are. A custom module name like `acme_bank_sso` could identify a client. There's no sure way to tell custom code from contrib. But there are good signs. An extension from a `drupal/*` Composer package is contrib. So is one with a `project` key in its `.info.yml` file. Anything under `core/` is core. We could send everything else as "custom", with no name. The UI and docs call the data "anonymous". It isn't. It's pseudonymous. We agree the wording should change. No one at Drupal CMS or Amplitude can identify a person from this data. But someone with the site's database could match login hashes to users. The docs should say that. The checkbox label should use plain words, not "pseudonymous". There's no setting in `settings.php` to turn telemetry off. You can turn it off with `drush sset drupal_cms_telemetry 0`, and that should be documented. But state can't be deployed, and any admin can turn it back on. Setting it with Drush also leaves queued events behind. Those get sent if telemetry is turned back on later. One option: `status()` checks state first, then falls back to a setting. That gives organizations a default, but not a guarantee. The product owner should decide which wins: state, the setting, or "a setting of `FALSE` always wins". Any setting will also stop the opt-in dialog from showing. The install-time environment variable needs a clear place in that order too. ### Bugs to fix The `DRUPAL_CMS_TELEMETRY` environment variable is cast with `(bool)`. That means "false", "no", and "off" all turn telemetry on. The fix is to check `$_SERVER`, then `$_ENV`, then `getenv()`, and parse the value with `FILTER_VALIDATE_BOOL`. Unset or invalid values should be ignored. **Partly addressed in [d18ad1ef](https://git.drupalcode.org/project/drupal_cms/-/commit/d18ad1ef68ccc1ad41c0a4ed1e96870cab3f6017):** a comment now documents that only `0` and `1` are valid. The `(bool)` cast is unchanged. The opt-in event is logged every time the Site information form is saved with the box ticked. It should only be logged when the value changes to on. **Fixed in [d18ad1ef](https://git.drupalcode.org/project/drupal_cms/-/commit/d18ad1ef68ccc1ad41c0a4ed1e96870cab3f6017).** If Amplitude rejects a batch, the batch is retried forever. New events pile up behind it. We should only drop events when the batch is at fault. On a 400 error, drop the bad events Amplitude lists and resend the rest. On a 413 error, try a smaller batch. For other errors, keep the events and retry. That includes 401, 403, 404, 429, server errors, and network errors. Those aren't the batch's fault. The queue has no size limit. We'll throw away events older than 30 days. That still lets busy sites build up a backlog within 30 days, so sending more than one batch per cron run may help. We also found that events don't include a timestamp. Amplitude uses the upload time instead. Events that wait in the queue get the wrong time. Each event should record its own time when it's logged. ### Cleanup There's a leftover `console.log()` in `telemetry.js`. It should be removed. **Fixed in [d18ad1ef](https://git.drupalcode.org/project/drupal_cms/-/commit/d18ad1ef68ccc1ad41c0a4ed1e96870cab3f6017).** A comment in `sendAll()` says it checks for opt-in. It actually checks for test, preview, and CI environments. The comment should say that. A line in `onModuleInstall()` uses XOR to work out whether a config sync is happening. It's hard to read. A plain `if` that sets `$is_syncing` to `FALSE` while a recipe is applying would be clearer. **Fixed in [d18ad1ef](https://git.drupalcode.org/project/drupal_cms/-/commit/d18ad1ef68ccc1ad41c0a4ed1e96870cab3f6017).** ### No change needed `status()` acts as both a getter and a setter. That's on purpose, and the doc comment explains it. The device ID is the same across dev, stage, and prod. That's on purpose. It identifies the site, not the environment. The `is_production` flag tells environments apart. Note that changing the hash salt makes a site look new. The opt-in form reuses code from the hook class. That's a style choice and causes no problems.