Random Drupal core hook that is longer: hook_field_available_languages_alter().

Obviously, if trying to assign an action anyway:

PDOException: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'hook' at row 1: INSERT INTO {trigger_assignments} (hook, aid, weight) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => LONG HOOKNAME HERE [:db_insert_placeholder_1] => 160 [:db_insert_placeholder_2] => 1 ) in trigger_assign_form_submit() (line 233 of /var/www/drupal/modules/trigger/trigger.admin.inc).

I think the field shouldn't be long enough to fit all possible PHP functions (that would be damn big), but a bit longer.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanr’s picture

Priority: Normal » Major

Same here. Can;t complete database updates because of this. Why was this field made so short?

julien’s picture

Issue tags: -Needs tests, -Novice

Maybe it has been defined short because it's a primary key, plus the string length of the base drupal hooks doesn't exceed 32 chars.
This column have this length also on drupal6, long keys are no doubt awful for performance and increase size of everything. On D6 this table had 3 primary key, now only two, with the same sizes.
Maybe it has been defined like this on D6 because of this bug http://bugs.mysql.com/bug.php?id=4541.

xjm’s picture

So, this size for the field dates to forever, as far as I can tell. It was 32 in #150245: Move hook_schema into .install files. I didn't bother to try to look before that. :)

I think we can increase the field size to 64 at least.

xjm’s picture

Alright, I skimmed and the longest hook name in core is hook_field_widget_properties_ENTITY_TYPE_alter at 46ish characters. So 64 seems to be sufficient. We could consider increasing it to 128 to be safe, but the con is, of course, the overhead.

xjm’s picture

Issue tags: +Needs tests, +Novice

Let's start with increasing it to 64 chars. Should be a simple enough patch.

  1. Change the schema to have a length of 64 for this field.
  2. Add a hook_update_N() in trigger to update the schema to the new field size.
  3. Add a maxlength of 64 to the hook field in trigger_assign_form().
  4. Add a test that tries to assign an action to (say) hook_openid_normalization_method_info_alter and confirm that it works.
  5. Add another test that tries to insert some really long hook name and ensure that it fails validation rather than a PDO exception.
julien’s picture

Status: Needs review » Active
FileSize
5.01 KB

@xjm made a patch for 1-3.

julien’s picture

Status: Active » Needs review
Issue tags: +Needs tests, +Novice
julien’s picture

Status: Active » Needs review
FileSize
1.23 KB

Oopsie, removed unrelated debug...

xjm’s picture

Status: Needs review » Needs work

Thanks @julien! #11 does not appear to include the change to trigger_schema(), though, so we need to add that too.

julien’s picture

@xjm, indeed, this one should be ok now.

julien’s picture

Status: Needs work » Needs review
xjm’s picture

Assigned: Unassigned » xjm

Guess I'll work on the test for this.

xjm’s picture

It looks like #764558: Remove Trigger module from core will probably go in, at which point we can move this issue to D7. It's still a major for D7, though.

xjm’s picture

Alright, I have a test for this, which brought me to realize failing validation isn't going to work as the trigger hook is defined in an info hook in the module that provides it, and it's a hidden element, so #maxlength is nonsense. Duh. (In case it is not obvious, I have never actually used trigger until today.)

So I'm trying to decide the proper way to respond to hook names that are too long. It doesn't make sense to fail validation for the user; rather, no form should be rendered at all if the trigger should not be inserted. Is there some way to disallow modules from trying to register triggers that are too long?

I'm also leaning toward bumping the length to 128 for that reason.

xjm’s picture

cafuego suggested probably a better solution in IRC:

  1. Add an autoincrement field as the primary key
  2. Change the hook field to a unique index
  3. Increase the field size for the hook field to something really big.
xjm’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Edit: I decided it's probably not worth the effort to fix trigger's queries for a serial key. Let's just bandaid the bug with a longer field for now.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
3.65 KB
2.58 KB

So, with 128 characters. Presumably we'll just reroll this patch for D7 once #764558: Remove Trigger module from core is in and rename the update hook, but leaving against D8 for now just in case.

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Test fails for the right reason. The fix solves the problem (and the combined patch passes). Found no coding style issues. Schema update function given. Yay!

catch’s picture

Status: Reviewed & tested by the community » Needs work

If this needs to go into D7, we shouldn't have a D8 update (since as yet we still don't support head2head upgrades).

xjm’s picture

Wasn't actually expecting this to get RTBCed... perhaps I should have more faith. :) Maybe we should just postpone it until #764558: Remove Trigger module from core goes in so that we don't force additional rerolls there?

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
Issue tags: +Novice, +Needs manual testing
FileSize
3.55 KB
3.1 KB

For academism, here's a D8 patch without the update hook and a D7 version with it named properly. Hopefully we can wait on #764558: Remove Trigger module from core (needs reviews!).

We should have folks test the D7 upgrade path and confirm it works properly. Tagging novice for that task:

  1. Install D7 from git without the patch applied.
  2. Enable the trigger module and assign some actions to some triggers.
  3. Inspect your {trigger_assigments} database table.
  4. Apply the patch and run update.php
  5. Check the {trigger_assignments} table again and ensure that:
    • The hook field is properly altered to 128 characters.
    • The data is otherwise unaffected.
  6. Manually add an additional trigger and confirm that configuration is stored as well.
  7. Report the results here.
pingers’s picture

Status: Needs review » Reviewed & tested by the community

Testing results:

Fresh D8

1. Installed D8 from git with the patch applied.
2. Enabled the trigger module and assign some actions to some triggers.
3. Inspected the {trigger_assigments} database table.
* Check the {trigger_assignments} table again and ensure that:
* The hook field is properly altered to 128 characters.
mysql> describe trigger_assignments;

+--------+--------------+------+-----+---------+-------+
| Field  | Type         | Null | Key | Default | Extra |
+--------+--------------+------+-----+---------+-------+
| hook   | varchar(128) | NO   | PRI |         |       |
| aid    | varchar(255) | NO   | PRI |         |       |
| weight | int(11)      | NO   |     | 0       |       |
+--------+--------------+------+-----+---------+-------+

* The data is otherwise unaffected.
mysql> select * from trigger_assignments;

+--------------+-----+--------+
| hook         | aid | weight |
+--------------+-----+--------+
| node_presave | 2   |      1 |
| node_view    | 2   |      1 |
+--------------+-----+--------+

D7

1. Install D7 from git without the patch applied.
2. Enable the trigger module and assign some actions to some triggers.
3. Inspect your {trigger_assigments} database table.
mysql> describe trigger_assignments;

+--------+--------------+------+-----+---------+-------+
| Field  | Type         | Null | Key | Default | Extra |
+--------+--------------+------+-----+---------+-------+
| hook   | varchar(32)  | NO   | PRI |         |       |
| aid    | varchar(255) | NO   | PRI |         |       |
| weight | int(11)      | NO   |     | 0       |       |
+--------+--------------+------+-----+---------+-------+

4. Apply the patch and run update.php

$ drush updb
The following updates are pending:

trigger module : 
  7001 -   Implements hook_update_N(). 

Do you wish to run all pending updates? (y/n): y
Finished performing updates.                                         [ok]

Check the {trigger_assignments} table again and ensure that:
5. The hook field is properly altered to 128 characters.
mysql> describe trigger_assignments;

+--------+--------------+------+-----+---------+-------+
| Field  | Type         | Null | Key | Default | Extra |
+--------+--------------+------+-----+---------+-------+
| hook   | varchar(128) | NO   | PRI |         |       |
| aid    | varchar(255) | NO   | PRI |         |       |
| weight | int(11)      | NO   |     | 0       |       |
+--------+--------------+------+-----+---------+-------+

6. The data is otherwise unaffected.
mysql> select * from trigger_assignments;

+--------------+-----+--------+
| hook         | aid | weight |
+--------------+-----+--------+
| node_presave | 2   |      1 |
| node_view    | 2   |      1 |
+--------------+-----+--------+

7. Manually add an additional trigger and confirm that configuration is stored as well.
All good :)

Also upgraded this D7 database to D8 from git, with the D8 patch applied. Added new trigger and existing ones worked fine. Additionally, the schema was not affected, so it was fine too.

So... RTBC in my view :)

Niklas Fiekas’s picture

See, xjm? ;) Thanks for the exhaustive testing.

xjm’s picture

Awesome job, thanks @pingers!

xjm’s picture

Issue tags: -Novice, -Needs manual testing

Untagging.

xjm’s picture

Status: Reviewed & tested by the community » Postponed
webchick’s picture

I'm not crazy about postponing major D7 bug fixes on a theoretical discussion. :( If anyone has to be inconvenienced, can't it be the people wanting to rip this module out of core, rather than people actively using it?

xjm’s picture

Status: Postponed » Reviewed & tested by the community
FileSize
3.55 KB

Renamed for the bot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1280792.patch, failed testing.

xjm’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs backport to D7

Ahem.

oriol_e9g’s picture

#33: 1280792.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks! And thanks for the extensive testing, pingers!!

andypost’s picture

Priority: Critical » Major
Status: Active » Needs review

Update fails on a site upgraded from D6 - mysql 5.1 myisam

The following updates returned messages
trigger module
Update #7001

    Failed: PDOException: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 1000 bytes: ALTER TABLE {trigger_assignments} CHANGE `hook` `hook` VARCHAR(128) NOT NULL DEFAULT '' COMMENT 'Primary Key: The name of the internal Drupal hook; for example, node_insert.', ADD PRIMARY KEY (`hook`, `aid`); Array ( ) in db_change_field() (line 2988 of /var/www/d7.git/includes/database/database.inc).

Just pulled git and this update failed

catch’s picture

Priority: Major » Critical
Status: Fixed » Active
andypost’s picture

Priority: Major » Critical
Status: Needs work » Active

Tested on innodb - upgrade works but myisam #fail

# mysql --version
mysql  Ver 14.14 Distrib 5.1.49, for debian-linux-gnu (x86_64) using readline 6.1

actual data

mysql> ALTER TABLE trigger_assignments CHANGE `hook` `hook` VARCHAR(128) NOT NULL DEFAULT '' COMMENT 'Primary Key: The name of the internal Drupal hook; for example, node_insert.';
Query OK, 3 rows affected (0.01 sec)
Records: 3  Duplicates: 0  Warnings: 0

mysql> select * from trigger_assignments;
+----------------+-----+--------+
| hook           | aid | weight |
+----------------+-----+--------+
| user_insert    | 2   |      1 |
| user_update    | 2   |      1 |
| comment_update | 2   |      1 |
+----------------+-----+--------+
3 rows in set (0.00 sec)

mysql> ALTER TABLE trigger_assignments  ADD PRIMARY KEY (`hook`, `aid`);
ERROR 1071 (42000): Specified key was too long; max key length is 1000 bytes

webchick’s picture

Status: Needs review » Needs work

Well! Guess 128 won't work then. :) Rolled back. Thanks so much for testing!!

While we're at it, let's change that "Implements hook_update_N()" to something more meaningful.

andypost’s picture

i've tested under centos with a same results
maximum length is 78
code to test

CREATE TABLE `atest`(
 `hook` VARCHAR(32) NOT NULL DEFAULT '',
 `aid` VARCHAR(255) NOT NULL DEFAULT '',
 PRIMARY KEY (`hook`, `aid`)
 ) ENGINE=MYISAM CHARSET=utf8 COLLATE=utf8_general_ci;
ALTER TABLE `atest` DROP PRIMARY KEY;
ALTER TABLE `atest` CHANGE `hook` `hook` VARCHAR(78) NOT NULL DEFAULT '';
ALTER TABLE `atest`  ADD PRIMARY KEY (`hook`, `aid`);
DROP TABLE `atest`;

EDIT related http://bugs.mysql.com/bug.php?id=4541

Niklas Fiekas’s picture

Priority: Critical » Major
Status: Active » Needs work

Mhh ... not sure what we should do now. Just go with that maximum?

andypost’s picture

This means summary key size should not be greater then 333 as pointed at http://bugs.mysql.com/bug.php?id=4541

catch’s picture

Issue tags: +Needs tests

We should add test coverage for this as well - this means there's no data in the trigger 7.x-7.x test database (not exactly surprising), just adding the data at all might be enough to catch the issue.

Niklas Fiekas’s picture

Issue tags: -Needs tests

Mhh ... I believe it's not about having data but instead about having InnoDB / MyISAM. So untagging, but I might be wrong. I can at least reproduce the problem without any data on MyISAM - I didn't catch it with InnoDB.

andypost’s picture

pingers’s picture

Ah, now I see why it's 78 :)
Primary key is aid and hook columns.
So 255 + 78 = 333

...bit slow today :P

xjm’s picture

xjm’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
3.58 KB

webchick suggested adding some generic tests to check for index lengths for myisam, since the issue has come up before, but I'm not sure exactly how one would go about that. Maybe enforce it in drupal_install_schema() or something? Seems like that would be a followup issue.

Attached uses 78 chars instead of 128, and also gives the update hook a meaningful summary. I tested using MyISAM instead of InnoDB, and confirmed that the update fails with #33 but passes with this version.

Tagging for manual testing because it would be good to have some additional confirmation.

Status: Needs review » Needs work

The last submitted patch, trigger-78-chars.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.56 KB

It might help if I actually updated the test to reflect the shorter length, eh?

xjm’s picture

Assigned: Unassigned » xjm

Gábor has added a handbook doc on writing upgrade path tests, so this issue seems like a good opportunity to review that doc. I'll give it a shot this afternoon.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
3.56 KB

Here's a stab at adding a 7.0 -> 7.x upgrade test. I'm not bothering to upload that test by itself or with the old patch because, theoretically, it should pass except on MyISAM (as described in #46).

xjm’s picture

Oops. This one.

Status: Needs review » Needs work

The last submitted patch, 1280792-with-upgrade-path-tests.patch, failed testing.

catch’s picture

Does aid really need to be 255 characters? Wondering if we could get away with reducing that (I've not checked).

xjm’s picture

@catch: Yeah, that occurred to me too. The aid isn't actually an ID at all, though; it's a machine name like (e.g.) comment_publish_action. So I think it would be a bad idea to shorten it at this point.

xjm’s picture

Status: Needs work » Needs review
FileSize
5.91 KB

#55 failed because I neglected to put a slash between the path and database dump filename. Oops. Fixed here.

Status: Needs review » Needs work

The last submitted patch, 1280792-59.patch, failed testing.

pingers’s picture

drush test-run Trigger --methods="testLongTrigger" --uri=localhost/drupal
Trigger assignment form 14 passes, 0 fails, 0 exceptions, and 4 debug

Hmm... not sure why it's failing. Working locally for me (myisam & innodb :) )

xjm’s picture

Status: Needs work » Needs review
FileSize
4.69 KB

Maybe?

Status: Needs review » Needs work

The last submitted patch, 1280792-put-upgrade-tests-in-simpletest.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.7 KB

Another stupid mistake... Apologies for running the tests on-issue, but I don't have the option of running them locally ATM because of the stupid gzip library.

Status: Needs review » Needs work

The last submitted patch, 1280792-grr.patch, failed testing.

xjm’s picture

So the fatal is:
DatabaseSchemaObjectExistsException: Cannot add field <em class="placeholder">sessions</em>.<em class="placeholder">ssid</em>: field already exists. in DatabaseSchema_mysql->addField() (line 323 of /Applications/MAMP/htdocs/d7git/includes/database/mysql/schema.inc).

BTMash tested and got the same thing. Maybe a bug in the particular dump? I'll try a 6 -> 7 version instead.

xjm’s picture

btmash: xjm: you should use UpdatePathTestCase instead of UpgradePathTestCase
[12:11PM] btmash: Upgrade = 6 -7. Update = 7 - 7

xjm’s picture

Status: Needs work » Needs review
FileSize
4.7 KB
BTMash’s picture

Status: Needs review » Needs work
FileSize
4.7 KB

As per what @xjm said, 7.0 -> 7.x tests use UpdatePathTestCase while 6.x -> 7.x use UpgradePathTestCase. Attaching patch which should hopefully pass.

BTMash’s picture

Status: Needs work » Needs review

Ack...setting to needs review.

xjm’s picture

Issue tags: -Needs manual testing

And, sounds like our InnoDB and MyISAM testing is covered in #61!

pingers’s picture

FileSize
4.69 KB

Re-rolled to remove double new line at the end of update.trigger.test
...this didn't show in the diff... just when I actually applied the patch, the two empty lines were there :S

Was also thinking maybe the 78 chars needs explanation in the comments... but figure git blame is gooood and will point to this issue.

xjm’s picture

Edit: Never mind, I'm crazy.

A comment seems reasonable. I'll reroll to add that (and remove the newline).

xjm’s picture

FileSize
585 bytes
5.02 KB
oriol_e9g’s picture

FileSize
31.75 KB

Testbot happy, code looks good and tested manually.

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community
pingers’s picture

@xjm comment looks good - technically it's "less than or equal to 333 characters", but really it's 1000 bytes... in any case, if you need more info, I'm sure you'd find your way to this issue. Thanks for your persistence :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks a huge bunch of the additional testing on this sucker.

Committed and pushed to 7.x. Hooray!!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jantoine’s picture

Status: Closed (fixed) » Needs work

Forgive me if I am missing something obvious. I am upgrading a site from D6 to D7 and I am receiving the same error found in the issue summary. The error is being reported from trigger_update_7000() which is running before trigger_update_7001() where the size of the hook field is increased. Should the size of the hook field be increased in trigger_update_7000() before trying to update the hook field value?

jantoine’s picture

Status: Needs work » Needs review
FileSize
1.36 KB

Attached is a new patch that adds the functionality of trigger_update_7001() into trigger_update_7000(), but keeps trigger_update_7001() around for those who have run trigger_update_7000() prior to this issue being fixed. Not sure if this is the right approach, but it works for a D6 to D7 upgrade and I can't see how it would break any of the prior tests.

BTMash’s picture

an older version of hook_update_7000? Has hook_update_7000 changed? My understanding was that unless there was something absolutely horrible happening in a hook_update (white screen of death, red errors), the code in there could not be updated due to the kind of issue you are describing. hook_update_7000 is supposed to run first and if the issue is coming from there, then it *seems* like there might be some other kind of issue going on.

BTMash’s picture

@AntoineSolutions, the last stable version of Drupal came out on Feb 02. And the patch outlined above was committed on Feb. 14. I'm unsure of when the next stable version is coming out, but the patch from #74 will be in there when that happens.

jantoine’s picture

@BTMash, if I understand the problem in it's entirety, the patch from #74 misses a critical error, that is the D6 to D7 upgrade process.

The original trigger_update_7000() function attempted to consolidate the values of the "hook" and "op" fields of the "trigger_assignments" table into the "hook" field without considering that the combined data might be too large for the field. In comes trigger_update_7001() to increase the size of the field.

With the current fix from #74, in a D6 to D7 upgrade, if the values of the "hook" and "op" fields exceed 32 characters, trigger_update_7000() will fail and prevent trigger_update_7001() from running. Furthermore, it will never successfully run without requiring manual database updates. I would call that a pretty serious, if not a critical issue.

The patch in #81 adds the code from trigger_update_7001() that increases the size of the "hook" field into trigger_update_7000() and keeps trigger_update_7001() for sites that have already run trigger_update_7000().

Perhaps you could call this a separate issue since it deals specifically with the D6 to D7 upgrade process, but I would say it is simply an element of the issue that was overlooked. I'll leave it to you more experienced core devs to decide.

BTMash’s picture

Issue tags: +Needs tests

@AntoineSolutions, thank you very much for the clarification. I think I have a better understanding of what you are saying (hook, op become one, string may be too long to fit in the initial 32 chars provided, site admin is now stuck). In that case, let's keep it in this issue then and try to bring it to the attention of one of the core devs on what the best resolution would be. I'll try and mimic the issue in testing with a db dump so the core devs can see exactly how this gets affected or if you can provide a series of steps to take from a d6 install where this fails, it'll simplify my task in creating the particular test case.

xjm’s picture

The approach in #81 is interesting; that could work. We should make the update hook descriptions less than 80 chars, though (one line).

And yeah, we clearly need some additional test coverage for 6 -> 7 (I think I only added 7.0 -> 7.x).

BTMash’s picture

Assigned: Unassigned » BTMash

Assigning to self so I remember to tackle the issue.

BTMash’s picture

Wow, this is a shame...trigger is not tested in any way from Drupal 6 to Drupal 7 (it is not installed). So it was not tested to begin with :(

BTMash’s picture

FileSize
4.84 KB
3.49 KB

Based off what I see, I was able to replicate the issue @AntoineSolutions ran into. I've created a new upgrade simpletest around it. The first patch is just the test and the second patch is the test + @AntoineSolutions' patch. I don't know if we should actually include an install of trigger in the real install, however.

oriol_e9g’s picture

Why not add a dependency with hook_update_dependencies?

http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

$dependencies['trigger'][7000] = array(
    'trigger' => 7001,
 );

In this case, the new updates will run in this order: 7001 -> 7000.
The sites that runned in the past 7000 only will run 7001.

BTMash’s picture

@oriol_e9g, that is an interesting idea. Though I am curious to know what happens when a module self-references its dependency to a future update.

BTMash’s picture

Now that it passes, I'm unassigning it. Whoever gets a chance to can test out the update dependencies stuff (along with the test).

BTMash’s picture

Assigned: BTMash » Unassigned

ack. Forgot to unassign.

catch’s picture

I think it's fine to just run the update twice, it's a clever idea to use dependencies like that, but not sure it'd even work, and if it does I'm not sure it should...

We should probably add an installed trigger module to the upgrade path though as opposed to only creating the table.

BTMash’s picture

Hmm, alright :( I'll see if I can make time this week to create a new dump. We'll use the php file to introduce the new long trigger name for the test case.

catch’s picture

Oh just a sec I missed this bit entirely:

+db_update('system')->fields(array(
+  'schema_version' => '6000',
+  'status' => '1',
+))
+->condition('filename', 'modules/trigger/trigger.module')
+->execute();

That's probably enough then...

xjm’s picture

Status: Needs review » Needs work

Awesome, thanks BTMash.

+++ b/modules/simpletest/tests/upgrade/upgrade.trigger.testundefined
@@ -0,0 +1,30 @@
+ * Test taxonomy upgrades.

Should be:
Tests trigger upgrades.
(not taxonomy)

+++ b/modules/simpletest/tests/upgrade/upgrade.trigger.testundefined
@@ -0,0 +1,30 @@
+   * Basic tests for the trigger upgrade.

Should start with a verb; how about:
Tests a basic upgrade of the Trigger module.

+++ b/modules/trigger/trigger.installundefined
@@ -55,9 +55,13 @@ function trigger_install() {
+ * Increases the length of the "hook" field to 78 characters. Adds operation
+ * names to the hook names and drops the "op" field.

@@ -69,10 +73,13 @@ function trigger_update_7000() {
+ * Increases the length of the "hook" field to 78 characters for those who ran
+ * an older version of trigger_update_7000() that did not do this.

These should be one line. See also: #1414640: No documentation standard for hook_update_N() summaries [policy, no patch], http://drupal.org/node/1354#hookimpl

Regarding #94 - #96, I'd be comfortable with just installing the schema (as in the current patch) given that trigger is removed in D8.

xjm’s picture

Issue tags: -Needs tests

And it has tests. :)

BTMash’s picture

Issue tags: +Novice

Marking as novice since this seems pretty easy to change up :)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.05 KB

I was messing around with update/upgrade functions today, thought I'd try to help finish this off.
I went with Tests the Trigger 6 -> 7 upgrade path. for the UpgradePathTriggerTestCase docblock since it mirrors TriggerUpdatePathTestCase.

BTMash’s picture

Status: Needs review » Reviewed & tested by the community

I'm marking this as RTBC because this should satisfy the comments by @xjm.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.13 release notes

Awesome work on this folks. Tests++

I got very confused about why update_7000 was doing the same thing as update_7001, but both were declaring the primary key slightly differently. However, the code appears as though it's correct. I'm going to commit this fix and then try and get the word out for people to test it.

Committed and pushed to 7.x. Thanks!!

irunflower’s picture

Title: {trigger_assignments}.hook has only 32 characters, is too short » Node triggers removed when upgrading to 7-dev from 6.25
Version: 7.x-dev » 6.25
Assigned: Unassigned » irunflower
Status: Fixed » Active

Node triggers seem to disappear when upgrading to 7-dev from 6.25. This assignment was given to me by xjm in office hours (3/28/12). I did this test twice using a local environment, MAMP (Acquia Dev didn't seem to like the upgrading process).

Run 1:
1) Fresh install of 6.25
2) Enabled Trigger module
3) Assigned Node Trigger (Trigger: When either saving new content or updating existing content -> Make post sticky)
4) Upgraded to 7-dev (I did this by copying the files into the local directory in which 6.25 was saved).
5) In 7-dev, Trigger module was enabled, but previous trigger assignment was not saved.

Run 2:
1) Fresh install of 6.25
2) Enabled Trigger module
3) Assigned Node Trigger (Trigger: When either saving new content or updating existing content -> Make post sticky)
4) Assigned Comment Triggers (Trigger: After saving a new comment -> Publish comment, Trigger: After saving an updated comment -> Publish Comment, Trigger: After deleting a comment -> Make post unsticky, Save post).
5) Created 3 pages and commented on 1 page.
4) Upgraded to 7-dev
5) In 7-dev, Trigger was enabled, but Node Trigger was removed. Comment Triggers were all saved.
6) Tested one trigger: After saving a new comment; trigger action completed (published the comment like it was supposed to).

If anyone else can test this and add comments, that would be great.

xjm’s picture

Title: Node triggers removed when upgrading to 7-dev from 6.25 » Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25
Version: 6.25 » 7.x-dev
Issue tags: +Needs tests

Wow, I am very glad we did not push this out in 7.13 today.

This would be a bug in the 7.x code so moving back there. Zgear is also testing this to see if he can confirm what irunflower found and to see what happens for the 7.0 upgrade path.

And if this is the case, we still need even more test coverage that is clearly missing.

BTMash’s picture

Title: Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25 » Node triggers removed when upgrading to 7-dev from 6.25
Version: 7.x-dev » 6.25
Assigned: irunflower » Unassigned
Issue tags: -Needs tests

@irunflower, thank you for your finding. Its really appreciated :) I'm unassigning it from you as I'm not sure if you're working on a patch but it might give others a chance to chime in / offer a fix/whatnot as well if they are working on it. I'm going to try and find out once I have some time on what is going on if someone else doesn't get to it first.

BTMash’s picture

Title: Node triggers removed when upgrading to 7-dev from 6.25 » Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25
Version: 6.25 » 7.x-dev
Issue tags: +Needs tests

Ack, I had this window open for way too long. Sorry!

catch’s picture

@irunflower: could you try upgrading from 7.12? It surprises me that the update here would break this, but it wouldn't surprise me if it had been broken for a long time already.

BTMash’s picture

I **think** I have found the cause of the issue.

The prior hook for the node stuff was 'nodeapi' and we had the operation. The new hook is 'node_'.

So as part of the upgrade process, does that conversion occur anywhere?

webchick’s picture

Oooh. Faaaaascinating! I bet you anything there is not a conversion routine for that.

But that's good, because that means this is (probably) a general upgrade issue, and not specific to this patch.

webchick’s picture

To answer your question, though, no. http://api.drupal.org/api/drupal/modules%21trigger%21trigger.install/7 Only upgrade functions related to this issue.

BTMash’s picture

Status: Active » Needs review
FileSize
2.75 KB
1.76 KB

Attaching a patch that is only the tests and one that is test plus patch.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks great, test fails fine, RTBC.

xjm’s picture

Wow, that's a pretty interesting implication of an API change I never considered. I wonder if there are other hooks that might have similar issues?

Nice work @irunflower and @BTMash!

Zgear’s picture

Just followed irunflower's notes except I updated from 7.0 to 7.x dev and applied the patch. The patch was successful in retaining both the node triggers and the comment triggers. Moving on to updating from 6.25 :)

Zgear’s picture

Completed 6.25 upgrade with same results, moving on to 7.12 to dev.

Zgear’s picture

7.12 update works as well. Also when updating I updated to 7.13-dev just to clarify, is this an acceptable test to pass this through?

Zgear’s picture

update, apparently I have to clarify by same results I mean I got the same results as my previous post when I said that the patch retained the triggers.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x! Thanks a lot for all the helpful testing! :D core mentoring hours++

However, we're probably going to have this same problem with other hooks that got renamed, e.g. hook_comment, etc. BTMash said he opened a follow-up but I didn't catch the issue ID, so hopefully he'll report back.

BTMash’s picture

irunflower’s picture

I didn't check up on this until now... totally didn't think it would snowball O________O;; Sorry!!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

long hookname here, instead of testing value