search

backdrop-contrib / project

Projects associate a code-based project with releases and power the update server of BackdropCMS.org
2 stars 10 forks source link

Introduce Project Telemetry module. See core issue #285. #50

Closed quicksketch closed 3 years ago

quicksketch commented 3 years ago

Work in progress PR to introduce Telemetry support to Project module.

docwilmot commented 3 years ago

I was going to do a PR but thought I'd discuss first:

  1. on my local, I needed to add a project_telemetry_node_presave() to initialize $node->project['telemetry'] otherwise it was empty on creating new nodes. Not sure why it worked for others so maybe not needed?
  2. I use WAMP so project-telemetry-post.php couldnt find my base URL until I changed to: if (($pos = strpos($cwd, '/sites/')) || ($pos = strpos($cwd, '/modules/')) || ($pos = strpos($cwd, '\\modules\\')) || ($pos = strpos($cwd, '\\modules\\'))) but this will be on Backdrop CMS not on Windows so also maybe not needed?
  3. Line 79 in project_telemetry.module says 'allowed_options' => array() but I thought it should be 'allowed_values' => array(). It didnt work well until I did that but again, not sure why it worked for others so maybe not needed?
quicksketch commented 3 years ago
  1. on my local, I needed to add a project_telemetry_node_presave() to initialize $node->project['telemetry'] otherwise it was empty on creating new nodes. Not sure why it worked for others so maybe not needed?

I think that hook_node_prepare() is sufficient to initialize the telemetry options, but I'll double-check. Perhaps it is not fired in all situations.

  1. I use WAMP so project-telemetry-post.php couldnt find my base URL until I changed to: if (($pos = strpos($cwd, '/sites/')) || ($pos = strpos($cwd, '/modules/')) || ($pos = strpos($cwd, '\\modules\\')) || ($pos = strpos($cwd, '\\modules\\'))) but this will be on Backdrop CMS not on Windows so also maybe not needed?

Good catch. This use of getcwd() is the same in the other two Project stand-alone PHP files: project-release-serve-history.php and project-usage-process.php. Maybe we make that a follow-up, but as you say it's not much of a problem because backdropcms.org doesn't run Windows.

  1. Line 79 in project_telemetry.module says 'allowed_options' => array() but I thought it should be 'allowed_values' => array(). It didnt work well until I did that but again, not sure why it worked for others so maybe not needed?

I think you're right about this one. I'll correct.

quicksketch commented 3 years ago

Pushed up a new commit that fixes the setting of default values on hook_node_insert() and sets the proper key of "allowed_values" rather than "allowed_options".