| Project: | New Relic RPM Integration |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
Attached is a draft patch to update New Relic RPM to Drupal 7. This was primed using the Coder Upgrade module after applying the patches referenced in the following issues:
* #1398104-2: warning: Invalid argument supplied for foreach() in new_relic_rpm.reports.inc on line 92 and 126
* #1487792-1: Follow best practice for t() placeholders
* #1487866: Cleanup of report overview table code
* #1487962: Cleanup of application Quick Links code
* #1488080: Follow best practice for updating SQL in hook_update_N
The most notable change after priming was the replacement of the form alter on the module page submit button with D7's hook_modules_enable() and hook_modules_disable(). Also included is a tweak to the curl code: CURLOPT_SSL_VERIFYPEER = FALSE (my Mac environment gets access denied on the API calls without it).
I have tested the reporting feature, but that is all so far; posting the draft patch as a starting point.
| Attachment | Size |
|---|---|
| new_relic_rpm-d7-update-draft.patch | 19.22 KB |
Comments
#1
The attached patch includes a full diff from current master (commit dff71f1) to the D7 port draft.
#2
Just a note that curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, FALSE); should be removed from the patch-- After further investigation I believe this is an issue limited to the libcurl implementation on the current version of Acquia Dev Desktop Control Panel for Mac OS.
#3
@Johathan Webb Great work. Maybe you should offer to co-maintain the project, so your work can get committed?
#4
The final result of this patch produces a lot of errors in Drush.
It might be totally irelevant to the patch, but I do not have a D6 site on the server that has new-relic to test it on D6 too.
drush st (emphasis mine)
#5
+++ b/new_relic_rpm.installundefined@@ -47,7 +54,13 @@ function new_relic_rpm_uninstall() {
*/
function new_relic_rpm_update_6001() {
- db_query("UPDATE {system} SET weight = -20 WHERE name = 'new_relic_rpm'");
+ $ret = array();
+ // TODO update_sql has been removed. Use the database API for any schema or data changes.
+ $ret[] = array() /* update_sql("UPDATE {system} SET weight = -20 WHERE name = 'new_relic_rpm'") */;
+ // hook_update_N() no longer returns a $ret array. Instead, return
+ // nothing or a translated string indicating the update ran successfully.
+ // See http://drupal.org/node/224333#update_sql.
+ return t('TODO Add a descriptive string here to show in the UI.') /* $ret */;
This can be dropped, not necessary to carry over 6.x update functions. instead, add a hook_update_last_removed() implementation.
+++ b/new_relic_rpm.moduleundefined@@ -16,7 +16,7 @@ function new_relic_rpm_menu() {
- $items['admin/settings/new-relic-rpm'] = array(
+ $items['admin/config/new-relic-rpm'] = array(
'title' => 'New Relic RPM Settings',
Path should be admin/config/$something/new-relic-rpm, $something could be services, system or development, not sure.
+++ b/new_relic_rpm.moduleundefined@@ -44,18 +44,27 @@ function new_relic_rpm_menu() {
+ 'administer new relic rpm' => array(
+ 'title' => t('administer new relic rpm'),
+ 'description' => t('TODO Add a description for \'administer new relic rpm\''),
+ ),
+ 'view new relic rpm reports' => array(
+ 'title' => t('view new relic rpm reports'),
+ 'description' => t('TODO Add a description for \'view new relic rpm reports\''),
+ ),
+ 'create new relic rpm deployments' => array(
+ 'title' => t('create new relic rpm deployments'),
+ 'description' => t('TODO Add a description for \'create new relic rpm deployments\''),
+ ),
Those pointless descriptions should be removed.
+++ b/new_relic_rpm.moduleundefined@@ -216,26 +163,32 @@ function new_relic_rpm_modules_enabled($form, &$form_state) {
- $m_inst . $m_remv);
+ _new_relic_rpm_deploy('Drupal Module Install/Uninstall',
+ 'Drupal modules were installed: ' . ($modules_installed ? 'YES' : 'NO') . ' and uninstalled: ' . ($modules_removed ? 'YES' : 'NO'),
Not sure if this should go through t().
#6
+++ b/new_relic_rpm.drush.incundefined@@ -5,11 +5,11 @@
- require_once dirname(__FILE__) . '/new_relic_rpm.module';
+ require_once DRUPAL_ROOT . '/' . dirname(__FILE__) . '/new_relic_rpm.module';
This change is wrong and causes the reported errors.
#7
#6 works with drush
any updates on 7.x branch? Has anybody talked to the maintainers?
#8
Just starting to kick the tires on integrating Newrelic into our processes, but this module looks REALLY awesome. Happy to help when the time comes.
@neclumdul and @soyarma are still active on d.o (even in the last few hours), but no commits in the last year. I'll tweet them about maintainership and maybe we can figure out what's up :)
https://twitter.com/patconnolly/status/282187532618305536
#9
I won't be able to review or commit this before the new year but it would help if the updates not related to the 7.x port could be put in other issues to ease review and application to 6.x. Especially because I don't currently maintain any 7.x sites so it will be easier for me to review the changes in 6.
#10
They all already have 6.x issues, see the issue summary.
I've applied them to a local repository separately, including the actual port and made some small changes to address the things I pointed out above.
Attached is an updated patch for everything and one that contains just the port without the other changes (although also some coding style changes).
Also pushed my local repository to https://github.com/Berdir/new_relic_rpm, so if you want, feel free to pull/merge from there.
Edit: Note that I haven't actually tested this yet, I've just prepared it for testing :)
#11
Uh, with patches now.
#12
@Berdir amazing. Let you know when I can test. You're awesome
@neclimdul thanks for dropping in an update! makes perfect sense
#13
Why not create a 7.x branch on d.o. for easier issue reporting?
#14
+1 for a 7.x branch
#15
+1
#16
Hi, I´m just learning about NewRelic.
Is the module here ready to install and test?
Thanks! :)