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.

Files: 
CommentFileSizeAuthor
#111 1280792.testonly.patch1.76 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 38,636 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#111 1280792.patch2.75 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 38,626 pass(es).
[ View ]
#100 drupal-1280792-100.patch5.05 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 38,576 pass(es).
[ View ]
#89 1280792.testonly.patch3.49 KBBTMash
FAILED: [[SimpleTest]]: [MySQL] 38,523 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#89 1280792.88.patch4.84 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 38,517 pass(es).
[ View ]
#81 key-field-length-1280792-81.patch1.36 KBjantoine
PASSED: [[SimpleTest]]: [MySQL] 38,302 pass(es).
[ View ]
#75 testing-trigger.png31.75 KBoriol_e9g
#74 1280792-74.patch5.02 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,837 pass(es).
[ View ]
#74 interdiff.txt585 bytesxjm
#72 1280792-73.patch4.69 KBpingers
PASSED: [[SimpleTest]]: [MySQL] 37,863 pass(es).
[ View ]
#69 1280792.patch4.7 KBBTMash
PASSED: [[SimpleTest]]: [MySQL] 37,874 pass(es).
[ View ]
#68 1280792-updategradebraid.patch4.7 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,841 pass(es).
[ View ]
#64 1280792-grr.patch4.7 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 37,836 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#62 1280792-put-upgrade-tests-in-simpletest.patch4.69 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 37,821 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#59 1280792-59.patch5.91 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 37,853 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#55 1280792-with-upgrade-path-tests.patch5.91 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 37,863 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#54 trigger-78-chars-no-really.patch3.56 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,862 pass(es).
[ View ]
#52 trigger-78-chars-no-really.patch3.56 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,816 pass(es).
[ View ]
#50 trigger-78-chars.patch3.58 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 37,833 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#33 1280792.patch3.55 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 36,995 pass(es).
[ View ]
#26 d8-1280792-26.patch3.1 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,133 pass(es).
[ View ]
#26 1280792-26-d7.patch3.55 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1280792-26-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 1280792-22-test-only.patch2.58 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 34,133 pass(es), 1 fail(s), and 1 exception(es).
[ View ]
#22 1280792-22-combined.patch3.65 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,131 pass(es).
[ View ]
#13 hook_column_size-1280792-5367226.patch1.55 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 34,154 pass(es).
[ View ]
#11 hook_column_size-1280792-5362500.patch1.23 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es).
[ View ]
#9 hook_column_size-1280792-5362500.patch5.01 KBjulien
PASSED: [[SimpleTest]]: [MySQL] 34,172 pass(es).
[ View ]

Comments

Priority:Normal» Major

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

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.

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.

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.

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.

Status:Needs review» Active
StatusFileSize
new5.01 KB
PASSED: [[SimpleTest]]: [MySQL] 34,172 pass(es).
[ View ]

@xjm made a patch for 1-3.

Status:Active» Needs review
Issue tags:+Needs tests, +Novice

Status:Active» Needs review
StatusFileSize
new1.23 KB
PASSED: [[SimpleTest]]: [MySQL] 34,168 pass(es).
[ View ]

Oopsie, removed unrelated debug...

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.

StatusFileSize
new1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 34,154 pass(es).
[ View ]

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

Status:Needs work» Needs review

Assigned:Unassigned» xjm

Guess I'll work on the test for this.

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.

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.

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.

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.

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new3.65 KB
PASSED: [[SimpleTest]]: [MySQL] 34,131 pass(es).
[ View ]
new2.58 KB
FAILED: [[SimpleTest]]: [MySQL] 34,133 pass(es), 1 fail(s), and 1 exception(es).
[ View ]

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.

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!

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).

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?

Assigned:xjm» Unassigned
Status:Needs work» Needs review
Issue tags:+Novice, +Needs manual testing
StatusFileSize
new3.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1280792-26-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.1 KB
PASSED: [[SimpleTest]]: [MySQL] 34,133 pass(es).
[ View ]

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.

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 :)

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

Awesome job, thanks @pingers!

Untagging.

Status:Reviewed & tested by the community» Postponed

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?

Status:Postponed» Reviewed & tested by the community
StatusFileSize
new3.55 KB
PASSED: [[SimpleTest]]: [MySQL] 36,995 pass(es).
[ View ]

Renamed for the bot.

Status:Reviewed & tested by the community» Needs work

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

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

Ahem.

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

Status:Reviewed & tested by the community» Fixed

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

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

Priority:Major» Critical
Status:Fixed» Active

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

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.

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

Priority:Critical» Major
Status:Active» Needs work

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

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

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.

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.

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

...bit slow today :P

Status:Needs work» Needs review
Issue tags:+Needs manual testing
StatusFileSize
new3.58 KB
FAILED: [[SimpleTest]]: [MySQL] 37,833 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new3.56 KB
PASSED: [[SimpleTest]]: [MySQL] 37,816 pass(es).
[ View ]

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

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.

Assigned:xjm» Unassigned
StatusFileSize
new3.56 KB
PASSED: [[SimpleTest]]: [MySQL] 37,862 pass(es).
[ View ]

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).

StatusFileSize
new5.91 KB
FAILED: [[SimpleTest]]: [MySQL] 37,863 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Oops. This one.

Status:Needs review» Needs work

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

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

@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.

Status:Needs work» Needs review
StatusFileSize
new5.91 KB
FAILED: [[SimpleTest]]: [MySQL] 37,853 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

#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.

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 :) )

Status:Needs work» Needs review
StatusFileSize
new4.69 KB
FAILED: [[SimpleTest]]: [MySQL] 37,821 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Maybe?

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new4.7 KB
FAILED: [[SimpleTest]]: [MySQL] 37,836 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 37,841 pass(es).
[ View ]

Status:Needs review» Needs work
StatusFileSize
new4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 37,874 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Ack...setting to needs review.

Issue tags:-Needs manual testing

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

StatusFileSize
new4.69 KB
PASSED: [[SimpleTest]]: [MySQL] 37,863 pass(es).
[ View ]

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.

Edit: Never mind, I'm crazy.

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

StatusFileSize
new585 bytes
new5.02 KB
PASSED: [[SimpleTest]]: [MySQL] 37,837 pass(es).
[ View ]

StatusFileSize
new31.75 KB

Testbot happy, code looks good and tested manually.

Status:Needs review» Reviewed & tested by the community

@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 :)

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.

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?

Status:Needs work» Needs review
StatusFileSize
new1.36 KB
PASSED: [[SimpleTest]]: [MySQL] 38,302 pass(es).
[ View ]

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.

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.

@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.

@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.

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.

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).

Assigned:Unassigned» BTMash

Assigning to self so I remember to tackle the issue.

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 :(

StatusFileSize
new4.84 KB
PASSED: [[SimpleTest]]: [MySQL] 38,517 pass(es).
[ View ]
new3.49 KB
FAILED: [[SimpleTest]]: [MySQL] 38,523 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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.

@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.

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

Assigned:BTMash» Unassigned

ack. Forgot to unassign.

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.

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.

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...

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.

Issue tags:-Needs tests

And it has tests. :)

Issue tags:+Novice

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

Status:Needs work» Needs review
StatusFileSize
new5.05 KB
PASSED: [[SimpleTest]]: [MySQL] 38,576 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

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

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!!

Title:{trigger_assignments}.hook has only 32 characters, is too shortNode 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.

Title:Node triggers removed when upgrading to 7-dev from 6.25Trigger 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.

Title:Trigger upgrade path: Node triggers removed when upgrading to 7-dev from 6.25Node 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.

Title:Node triggers removed when upgrading to 7-dev from 6.25Trigger 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!

@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.

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?

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.

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

Status:Active» Needs review
StatusFileSize
new2.75 KB
PASSED: [[SimpleTest]]: [MySQL] 38,626 pass(es).
[ View ]
new1.76 KB
FAILED: [[SimpleTest]]: [MySQL] 38,636 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Patch looks great, test fails fine, RTBC.

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!

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 :)

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

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?

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.

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.

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.

Issue summary:View changes

long hookname here, instead of testing value