As a followup to #1280792: Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25, we discovered that changes in hooks have resulted in missing enabled triggers during the upgrade from 6.x to 7.x. An example of this was with the node module where hook_nodeapi($op) was changed to hook_node_op<operation>. However, there may be other core modules which implement triggers that also run into the same issue (hook_comment($op) was changed into hook_comment_op). So we need to:

  • Create additional tests. We are currently doing the testing for node operations. We should test for all other core triggers and make sure they are there when enabled. See the patch at http://drupal.org/node/1280792#comment-5801758 for how is should be done
  • Implement whatever fixes are necessary to convert the old trigger hooks from 6.x into the new variant for 7.x
CommentFileSizeAuthor
#24 1515212-14-trigger-path-24.patch18.39 KBvijaycs85
#22 1515212-22-reroll.patch1.14 KBwelly
#15 interdiff.txt1.01 KBxjm
#14 1515212-14-complete.patch18.36 KBxjm
#14 interdiff.txt1.01 KBxjm
#9 1515212-9-tests.patch17.32 KBxjm
#9 1515212-9-complete.patch18.34 KBxjm
#7 1515212.d7.patch18.34 KBBTMash
#6 1515212.d7.patch18.33 KBBTMash
#4 1515212.d7.patch7.64 KBBTMash
#2 1515212.testonly.patch6.63 KBBTMash
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BTMash’s picture

BTMash CreditAttribution: BTMash commented
Issue tags: +Needs tests

Looking through core, these are the following hooks/operations we should try and test on (this is based on a quick search for hook_action_info():

  • nodeapi(view, insert, update, delete)
  • comment(view, insert, update, delete)
  • user(view, insert, update, delete, login)
  • taxonomy(insert, update, delete)
BTMash’s picture

BTMash CreditAttribution: BTMash commented
Category: bug » task
Status: Active » Needs review
FileSize
1515212.testonly.patch6.63 KB

Alright, I'm adding a patch that consists solely of the tests. As an FYI, it fails on taxonomy. So something changed there. I don't know if there are other things I have missed, however. But this seemed like the easiest path to take and if someone could take a read through the tests to see if they make sense, that would be awesome.

BTMash’s picture

BTMash CreditAttribution: BTMash commented

And here is a patch that should fix the trigger issue. Note: anyone else that is good with str_replace and the like is welcome to change it up. We just have to account for both 6.x and 7.x (so we need to also update the trigger update tests from 7.0 to 7.x)

BTMash’s picture

BTMash CreditAttribution: BTMash commented
FileSize
1515212.d7.patch7.64 KB

Ack...forgot patch.

BTMash’s picture

BTMash CreditAttribution: BTMash commented
Priority: Normal » Major

And bumping this to major since trigger upgrade path for taxonomy triggers is currently broken.

BTMash’s picture

BTMash CreditAttribution: BTMash commented
FileSize
1515212.d7.patch18.33 KB

Ok, I ended up finding some more hooks, and I fleshed out the update path tests for trigger from 7.0 -> 7.x as well. However, I can't figure out why the cron task does not update. Any help would be greatly appreciated.

BTMash’s picture

BTMash CreditAttribution: BTMash commented
FileSize
1515212.d7.patch18.34 KB

Finally figured out that the reasons for the fail were because I misspelled 'system'. Latest patch should pass.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Category: task » bug

I think this is a bug since the upgrade is destroying data.

At first glance the test coverage looks great. I'd like to see the latest results of the test by themselves exposed on QA, so I'll try to upload a patch for that shortly.

Couple minor notes:

+++ b/modules/simpletest/tests/upgrade/update.trigger.testundefined
@@ -1,23 +1,23 @@
-      'group' => 'Upgrade path',
+      'group' => 'update path',

I think "Update path" should be capitalized here.

+++ b/modules/trigger/trigger.installundefined
@@ -105,3 +105,33 @@ function trigger_update_7002() {
+ * Renames taxonomy to taxonomy_term.

This is user-facing text, so I'd say something a little clearer. (Also update hooks get imperative verbs for the same reason; see http://drupal.org/node/1354#hookimpl.)

Perhaps "Update taxonomy hooks to new names." Something along those lines?

Leaving at NR since I haven't looked at the tests closely yet.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
1515212-9-complete.patch18.34 KB
1515212-9-tests.patch17.32 KB

Attached just cleans up the minor points above and includes the latest tests separately to expose the taxonomy failures.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Oh, for the next reroll, the inline comments also need to end in periods. :)

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented 
+
+/**
+ * Update taxonomy hook records to new names.
+ */
+function trigger_update_7003() {

Taxonomy vs trigger

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Taxonomy vs trigger

No, actually. This is the trigger module updating the names of the taxonomy hooks in its database records to their Drupal 7 names. So the doc is correct.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +D7 upgrade path

Tagging.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
interdiff.txt1.01 KB
1515212-14-complete.patch18.36 KB

Attached just tweaks the update hook doc slightly to avoid confusion like @aspilicious's in #11.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
interdiff.txt1.01 KB

Not sure what happened to the interdiff upload; one more time.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/modules/trigger/trigger.installundefined
@@ -105,3 +105,33 @@ function trigger_update_7002() {
+    $pattern = array(
...
+      'cron_run'
...
+    $replacement = array(
...
+      'cron'

Hrm. I was about to update the doc here to mention cron too, but there is no hook_cron_run() in D6 or D7, nor can I find this grepping D6. Can you clarify where the value 'cron_run' would be coming from?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

That's not a Drupal hook, it's a trigger hook, see http://api.drupal.org/api/drupal/modules%21trigger%21trigger.module/func... and http://api.drupal.org/api/drupal/modules%21system%21system.module/functi....

Yes, the hook is defined in system.module but called in trigger.module ;)

BTMash’s picture

BTMash CreditAttribution: BTMash commented

@xjm, This is actually initiated via trigger_cron (see http://api.drupal.org/api/drupal/modules!trigger!trigger.module/function... and http://api.drupal.org/api/drupal/modules!trigger!trigger.module/function...).

As far as the update hook names...even though it is coming from the trigger module (hence the updates would be groups with trigger updates), it made sense to say what hook names in the db were being updated.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Major » Normal

Downgrading this.

While some data gets lost during the upgrade path it's easy to just configure the triggers again - there's nothing irreversible here.

Also it's fairly minor compared to the configuration changes that usually need to be made when doing a major version upgrade.

readyman’s picture

readyman CreditAttribution: readyman commented
Assigned: Unassigned » readyman

Drupalcon Sydney

readyman’s picture

readyman CreditAttribution: readyman commented
Assigned: readyman » Unassigned

Too adventurous for our first attempt. Needs re-roll due to PSR-0

welly’s picture

welly CreditAttribution: welly commented
FileSize
1515212-22-reroll.patch1.14 KB

Have re-rolled the patch. Please review!

ZenDoodles’s picture

ZenDoodles CreditAttribution: ZenDoodles commented
Status: Needs review » Needs work
Issue tags: +sydney2013

Thank you @welly

Thanks for your patch, but it looks like there are some missing changes in other files?

We need the complete patch including the changes made in:

modules/simpletest/tests/upgrade/drupal-6.trigger.database.php
modules/simpletest/tests/upgrade/drupal-7.trigger.database.php
modules/simpletest/tests/upgrade/update.trigger.test
modules/simpletest/tests/upgrade/upgrade.trigger.test
modules/trigger/trigger.install b/modules/trigger/trigger.install

(Note: credit no_angel and damienwhaley too? http://core.drupalofficehours.org/task/1471)

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
1515212-14-trigger-path-24.patch18.39 KB

Another re-roll...

Deciphered’s picture

Deciphered CreditAttribution: Deciphered commented

@vijaycs84,

If possible, could you add an interdiff.txt (http://drupal.org/node/1488712, http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...) so the patch can be reviewed a little easier?