Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Some modules like ctools(custom pages) send menu array which contains "weight" column as "string" and thus a PDO is thrown as below :
1366 Incorrect integer value: '' for column 'weight' at row 19: INSERT INTO {menu_router}
Proposed resolution
Cast the value to an integer as per attached patch.
Remaining tasks
Cast other items as needed i.e., checking for NULL values before executing insert statement.
User interface changes
Nonde
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#28 | 3142821-28.patch | 2.44 KB | mcdruid |
#28 | interdiff-3142821-26-28.txt | 3.17 KB | mcdruid |
Comments
Comment #2
amjad1233Comment #3
amjad1233Comment #4
larowlanI think we use the alias int rather than integer for consistency with static type-hinting in PHP7+ (although both are valid).
I suspect because this is a bug, it will likely need a test to demonstrate the issue and fix.
Demonstrating both should be as simple as adding a string weight entry into
menu_test_menu()
Are you able to do that, and upload a 'test only' patch (without the fix, with just the change to
menu_test_menu
).Thanks
Comment #5
larowlanRe
int
vsinteger
it looks like core mostly doesint
but I see a few instances of integer in D7 contrib.Comment #6
amjad1233Awesome. Adding tests.
Comment #7
amjad1233Here is the patch with test added.
Comment #8
amjad1233Please do not commit this patch. It's negative testing.
Comment #9
amjad1233Another negative test :-(
Comment #10
amjad1233Just the test.
Comment #11
amjad1233Null value.
Comment #12
amjad1233The above negative tests prove that if Null is passed the PDO is being thrown.
casting to int will take care of either scenario.
I have included a new patch with improvement on menu_link_save() function.
Comment #13
amjad1233Apologies not attaching test in the previous patch.
Comment #14
larowlanAssuming this comes back green, this looks RTBC to me.
Comment #15
amjad1233Added Proper Test. And left opportunity for future values to be tested etc.
Comment #16
amjad1233The same patchwith new tests.Comment #19
amjad1233Oops. Put the casting in the wrong place. Sorry.
But I promise I looked at the patch twice before submitting. ☹
Comment #20
amjad1233I can't believe myself. Feel like patching myself.
Comment #21
amjad1233RTBC based on @larwon's review.
Comment #22
larowlanDoes D7 expect docblocks for these methods, or at the very least {@inheritdoc} ?
is this needed, seems premature to assume there's a child class
phpcs nit: needs an empty line between previous method
we can use $this->fail() here
And $this->pass here
Comment #23
amjad1233Added additional code improvements + phpcs as per @larowlan's feedback.
Thanks for that @larowlan.
Comment #24
amjad1233Comment #25
larowlanthanks @amjad1233, this looks good to me
Comment #26
craigra CreditAttribution: craigra commentedReroll of 3142821-menu-weight-casting-with-tests-7.patch in #23.
No changes - rebase only.
We've been using 3142821-menu-weight-casting-with-tests-7.patch for many months in live sites without issue.
Comment #27
mcdruidI double-checked that the test is still failing without the accompanying changes in menu.inc
Looking at the PDOException that's thrown if the NULL is not cast to an int:
So, yup I think this looks good.
We've just committed several similar tweaks to cast variables to the correct type in order to avoid PHP 8.1 complaining:
https://git.drupalcode.org/project/drupal/-/commit/71626aac0465960774bfc...
...and that included a few casts to
(int)
(as opposed to(integer)
) so this patch is consistent with that.I'm going to make one or two tweaks to the comments in the test, but nothing major.
Comment #28
mcdruidComment #30
mcdruidThank you!