| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | trigger.module |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | 7.13 release notes, Needs tests, Novice |
Issue Summary
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.
Comments
#1
Same here. Can;t complete database updates because of this. Why was this field made so short?
#2
Maybe it has been defined short because it's a primary key, plus the string length of the base drupal hooks doesn't exceed
32chars.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.
#3
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.
#4
Alright, I skimmed and the longest hook name in core is
hook_field_widget_properties_ENTITY_TYPE_alterat 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.#5
Let's start with increasing it to 64 chars. Should be a simple enough patch.
hook_update_N()in trigger to update the schema to the new field size.trigger_assign_form().hook_openid_normalization_method_info_alterand confirm that it works.#9
@xjm made a patch for 1-3.
#10
#11
Oopsie, removed unrelated debug...
#12
Thanks @julien! #11 does not appear to include the change to
trigger_schema(), though, so we need to add that too.#13
@xjm, indeed, this one should be ok now.
#14
#15
Guess I'll work on the test for this.
#16
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.
#17
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
#maxlengthis 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.
#18
cafuego suggested probably a better solution in IRC:
#21
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.
#22
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.
#23
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!
#24
If this needs to go into D7, we shouldn't have a D8 update (since as yet we still don't support head2head upgrades).
#25
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?
#26
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:
{trigger_assigments}database table.{trigger_assignments}table again and ensure that:hookfield is properly altered to 128 characters.#27
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 :)
#28
See, xjm? ;) Thanks for the exhaustive testing.
#29
Awesome job, thanks @pingers!
#30
Untagging.
#31
Postponing this on #764558: Remove Trigger module from core.
#32
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?
#33
Renamed for the bot.
#34
The last submitted patch, 1280792.patch, failed testing.
#35
Ahem.
#36
#33: 1280792.patch queued for re-testing.
#37
Committed and pushed to 7.x. Thanks! And thanks for the extensive testing, pingers!!
#38
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
#39
#40
Tested on innodb - upgrade works but myisam #fail
# mysql --versionmysql 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
#41
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.
#42
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
#43
Mhh ... not sure what we should do now. Just go with that maximum?
#44
This means summary key size should not be greater then 333 as pointed at http://bugs.mysql.com/bug.php?id=4541
#45
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.
#46
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.
#47
Tests for trigger has a lot of problems
#244093: Node and comment actions are (still) completely broken and have broken tests too
#420124: DX: Remove module actions on uninstall
#48
Ah, now I see why it's 78 :)
Primary key is aid and hook columns.
So 255 + 78 = 333
...bit slow today :P
#49
Regarding #41: #1414640: No documentation standard for hook_update_N() summaries [policy, no patch]
#50
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.
#51
The last submitted patch, trigger-78-chars.patch, failed testing.
#52
It might help if I actually updated the test to reflect the shorter length, eh?
#53
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.
#54
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).
#55
Oops. This one.
#56
The last submitted patch, 1280792-with-upgrade-path-tests.patch, failed testing.
#57
Does aid really need to be 255 characters? Wondering if we could get away with reducing that (I've not checked).
#58
@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.#59
#55 failed because I neglected to put a slash between the path and database dump filename. Oops. Fixed here.
#60
The last submitted patch, 1280792-59.patch, failed testing.
#61
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 :) )
#62
Maybe?
#63
The last submitted patch, 1280792-put-upgrade-tests-in-simpletest.patch, failed testing.
#64
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.
#65
The last submitted patch, 1280792-grr.patch, failed testing.
#66
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.
#67
#68
#69
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.
#70
Ack...setting to needs review.
#71
And, sounds like our InnoDB and MyISAM testing is covered in #61!
#72
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.
#73
Edit: Never mind, I'm crazy.
A comment seems reasonable. I'll reroll to add that (and remove the newline).
#74
#75
Testbot happy, code looks good and tested manually.
#76
#77
@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 :)
#78
Great, thanks a huge bunch of the additional testing on this sucker.
Committed and pushed to 7.x. Hooray!!
#79
Automatically closed -- issue fixed for 2 weeks with no activity.
#80
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?
#81
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.
#82
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.
#83
@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.
#84
@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.
#85
@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.
#86
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).
#87
Assigning to self so I remember to tackle the issue.
#88
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 :(
#89
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.
#90
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.
#91
@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.
#92
Now that it passes, I'm unassigning it. Whoever gets a chance to can test out the update dependencies stuff (along with the test).
#93
ack. Forgot to unassign.
#94
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.
#95
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.
#96
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...
#97
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.
#98
And it has tests. :)
#99
Marking as novice since this seems pretty easy to change up :)
#100
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.#101
I'm marking this as RTBC because this should satisfy the comments by @xjm.
#102
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!!
#103
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.
#104
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.
#105
@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.
#106
Ack, I had this window open for way too long. Sorry!
#107
@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.
#108
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?
#109
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.
#110
To answer your question, though, no. http://api.drupal.org/api/drupal/modules%21trigger%21trigger.install/7 Only upgrade functions related to this issue.
#111
Attaching a patch that is only the tests and one that is test plus patch.
#112
Patch looks great, test fails fine, RTBC.
#113
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!
#114
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 :)
#115
Completed 6.25 upgrade with same results, moving on to 7.12 to dev.
#116
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?
#117
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.
#118
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.
#119
Yep, I've opened up a followup issue at #1515212: Trigger upgrade path: enabled triggers removed due to hook changes from 6.x to 7.x
#120
I didn't check up on this until now... totally didn't think it would snowball O________O;; Sorry!!
#121
Automatically closed -- issue fixed for 2 weeks with no activity.