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.
Comment | File | Size | Author |
---|---|---|---|
#111 | 1280792.testonly.patch | 1.76 KB | BTMash |
#111 | 1280792.patch | 2.75 KB | BTMash |
#100 | drupal-1280792-100.patch | 5.05 KB | tim.plunkett |
#89 | 1280792.testonly.patch | 3.49 KB | BTMash |
#89 | 1280792.88.patch | 4.84 KB | BTMash |
Comments
Comment #1
seanrSame here. Can;t complete database updates because of this. Why was this field made so short?
Comment #2
julien CreditAttribution: julien commentedMaybe 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.
Comment #3
xjmSo, 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.
Comment #4
xjmAlright, 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.Comment #5
xjmLet'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_alter
and confirm that it works.Comment #9
julien CreditAttribution: julien commented@xjm made a patch for 1-3.
Comment #10
julien CreditAttribution: julien commentedComment #11
julien CreditAttribution: julien commentedOopsie, removed unrelated debug...
Comment #12
xjmThanks @julien! #11 does not appear to include the change to
trigger_schema()
, though, so we need to add that too.Comment #13
julien CreditAttribution: julien commented@xjm, indeed, this one should be ok now.
Comment #14
julien CreditAttribution: julien commentedComment #15
xjmGuess I'll work on the test for this.
Comment #16
xjmIt 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.
Comment #17
xjmAlright, 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.
Comment #18
xjmcafuego suggested probably a better solution in IRC:
Comment #21
xjmEdit: 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.
Comment #22
xjmSo, 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.
Comment #23
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTest 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!
Comment #24
catchIf this needs to go into D7, we shouldn't have a D8 update (since as yet we still don't support head2head upgrades).
Comment #25
xjmWasn'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?
Comment #26
xjmFor 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:hook
field is properly altered to 128 characters.Comment #27
pingers CreditAttribution: pingers commentedTesting 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;
* The data is otherwise unaffected.
mysql> select * from trigger_assignments;
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;
4. Apply the patch and run update.php
Check the {trigger_assignments} table again and ensure that:
5. The hook field is properly altered to 128 characters.
mysql> describe trigger_assignments;
6. The data is otherwise unaffected.
mysql> select * from trigger_assignments;
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 :)
Comment #28
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedSee, xjm? ;) Thanks for the exhaustive testing.
Comment #29
xjmAwesome job, thanks @pingers!
Comment #30
xjmUntagging.
Comment #31
xjmPostponing this on #764558: Remove Trigger module from core.
Comment #32
webchickI'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?
Comment #33
xjmRenamed for the bot.
Comment #35
xjmAhem.
Comment #36
oriol_e9g#33: 1280792.patch queued for re-testing.
Comment #37
webchickCommitted and pushed to 7.x. Thanks! And thanks for the extensive testing, pingers!!
Comment #38
andypostUpdate fails on a site upgraded from D6 - mysql 5.1 myisam
Just pulled git and this update failed
Comment #39
catchComment #40
andypostTested on innodb - upgrade works but myisam #fail
actual data
Comment #41
webchickWell! 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.
Comment #42
andyposti've tested under centos with a same results
maximum length is 78
code to test
EDIT related http://bugs.mysql.com/bug.php?id=4541
Comment #43
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMhh ... not sure what we should do now. Just go with that maximum?
Comment #44
andypostThis means summary key size should not be greater then 333 as pointed at http://bugs.mysql.com/bug.php?id=4541
Comment #45
catchWe 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.
Comment #46
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMhh ... 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.
Comment #47
andypostTests 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
Comment #48
pingers CreditAttribution: pingers commentedAh, now I see why it's 78 :)
Primary key is aid and hook columns.
So 255 + 78 = 333
...bit slow today :P
Comment #49
xjmRegarding #41: #1414640: No documentation standard for hook_update_N() summaries [policy, no patch]
Comment #50
xjmwebchick 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.
Comment #52
xjmIt might help if I actually updated the test to reflect the shorter length, eh?
Comment #53
xjmGá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.
Comment #54
xjmHere'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).
Comment #55
xjmOops. This one.
Comment #57
catchDoes aid really need to be 255 characters? Wondering if we could get away with reducing that (I've not checked).
Comment #58
xjm@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.Comment #59
xjm#55 failed because I neglected to put a slash between the path and database dump filename. Oops. Fixed here.
Comment #61
pingers CreditAttribution: pingers commenteddrush 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 :) )
Comment #62
xjmMaybe?
Comment #64
xjmAnother 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.
Comment #66
xjmSo 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.
Comment #67
xjmComment #68
xjmComment #69
BTMash CreditAttribution: BTMash commentedAs per what @xjm said, 7.0 -> 7.x tests use UpdatePathTestCase while 6.x -> 7.x use UpgradePathTestCase. Attaching patch which should hopefully pass.
Comment #70
BTMash CreditAttribution: BTMash commentedAck...setting to needs review.
Comment #71
xjmAnd, sounds like our InnoDB and MyISAM testing is covered in #61!
Comment #72
pingers CreditAttribution: pingers commentedRe-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.
Comment #73
xjmEdit: Never mind, I'm crazy.
A comment seems reasonable. I'll reroll to add that (and remove the newline).
Comment #74
xjmComment #75
oriol_e9gTestbot happy, code looks good and tested manually.
Comment #76
oriol_e9gComment #77
pingers CreditAttribution: pingers commented@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 :)
Comment #78
webchickGreat, thanks a huge bunch of the additional testing on this sucker.
Committed and pushed to 7.x. Hooray!!
Comment #80
jantoine CreditAttribution: jantoine commentedForgive 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?
Comment #81
jantoine CreditAttribution: jantoine commentedAttached 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.
Comment #82
BTMash CreditAttribution: BTMash commentedan 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.
Comment #83
BTMash CreditAttribution: BTMash commented@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.
Comment #84
jantoine CreditAttribution: jantoine commented@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.
Comment #85
BTMash CreditAttribution: BTMash commented@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.
Comment #86
xjmThe 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).
Comment #87
BTMash CreditAttribution: BTMash commentedAssigning to self so I remember to tackle the issue.
Comment #88
BTMash CreditAttribution: BTMash commentedWow, 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 :(
Comment #89
BTMash CreditAttribution: BTMash commentedBased 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.
Comment #90
oriol_e9gWhy not add a dependency with hook_update_dependencies?
http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...
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.
Comment #91
BTMash CreditAttribution: BTMash commented@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.
Comment #92
BTMash CreditAttribution: BTMash commentedNow that it passes, I'm unassigning it. Whoever gets a chance to can test out the update dependencies stuff (along with the test).
Comment #93
BTMash CreditAttribution: BTMash commentedack. Forgot to unassign.
Comment #94
catchI 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.
Comment #95
BTMash CreditAttribution: BTMash commentedHmm, 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.
Comment #96
catchOh just a sec I missed this bit entirely:
That's probably enough then...
Comment #97
xjmAwesome, thanks BTMash.
Should be:
Tests trigger upgrades.
(not taxonomy)
Should start with a verb; how about:
Tests a basic upgrade of the Trigger module.
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.
Comment #98
xjmAnd it has tests. :)
Comment #99
BTMash CreditAttribution: BTMash commentedMarking as novice since this seems pretty easy to change up :)
Comment #100
tim.plunkettI 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.Comment #101
BTMash CreditAttribution: BTMash commentedI'm marking this as RTBC because this should satisfy the comments by @xjm.
Comment #102
webchickAwesome 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!!
Comment #103
irunflower CreditAttribution: irunflower commentedNode 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.
Comment #104
xjmWow, 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.
Comment #105
BTMash CreditAttribution: BTMash commented@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.
Comment #106
BTMash CreditAttribution: BTMash commentedAck, I had this window open for way too long. Sorry!
Comment #107
catch@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.
Comment #108
BTMash CreditAttribution: BTMash commentedI **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?
Comment #109
webchickOooh. 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.
Comment #110
webchickTo answer your question, though, no. http://api.drupal.org/api/drupal/modules%21trigger%21trigger.install/7 Only upgrade functions related to this issue.
Comment #111
BTMash CreditAttribution: BTMash commentedAttaching a patch that is only the tests and one that is test plus patch.
Comment #112
catchPatch looks great, test fails fine, RTBC.
Comment #113
xjmWow, 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!
Comment #114
Zgear CreditAttribution: Zgear commentedJust 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 :)
Comment #115
Zgear CreditAttribution: Zgear commentedCompleted 6.25 upgrade with same results, moving on to 7.12 to dev.
Comment #116
Zgear CreditAttribution: Zgear commented7.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?
Comment #117
Zgear CreditAttribution: Zgear commentedupdate, 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.
Comment #118
webchickCommitted 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.
Comment #119
BTMash CreditAttribution: BTMash commentedYep, 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
Comment #120
irunflower CreditAttribution: irunflower commentedI didn't check up on this until now... totally didn't think it would snowball O________O;; Sorry!!
Comment #121.0
(not verified) CreditAttribution: commentedlong hookname here, instead of testing value