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

CommentFileSizeAuthor
#28 3142821-28.patch2.44 KBmcdruid
#28 interdiff-3142821-26-28.txt3.17 KBmcdruid
#26 3142821-menu-weight-casting-with-tests-26.patch2.74 KBcraigra
#23 3142821-menu-weight-casting-with-tests-7.patch3.08 KBamjad1233
#20 3142821-menu-weight-casting-with-tests-6-final-final-final.patch3.13 KBamjad1233
#19 3142821-menu-weight-casting-with-tests-5-final-final.patch2.73 KBamjad1233
#16 3142821-menu-weight-casting-with-tests-4-final.patch2.88 KBamjad1233
#15 test-only-3-NULL-3142821-menu-weight-casting-with-tests.patch1.5 KBamjad1233
#13 3142821-menu-weight-casting-with-tests-3.patch2.09 KBamjad1233
#12 3142821-menu-weight-casting-with-tests-2.patch1.23 KBamjad1233
#11 test-only-2-NULL-3142821-menu-weight-casting-with-tests.patch655 bytesamjad1233
#10 test-only-2-3142821-menu-weight-casting-with-tests.patch653 bytesamjad1233
#9 test-only-3142821-menu-weight-casting-with-tests.patch1.42 KBamjad1233
#8 no-commit-3142821-menu-weight-casting-NEGETIVE-test.patch654 bytesamjad1233
#7 3142821-menu-weight-casting-with-tests.patch1.42 KBamjad1233
#2 3142821-menu-weight-casting.patch454 bytesamjad1233
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amjad1233 created an issue. See original summary.

amjad1233’s picture

amjad1233’s picture

Title: PDO Exception : menu_router, weight column is not being casted to integer » PDO Exception : Incorrect integer value: for column 'weight' (weight column is not being casted to integer)
larowlan’s picture

Status: Active » Needs work
Issue tags: +Needs tests
+++ b/includes/menu.inc
@@ -3894,7 +3894,7 @@ function _menu_router_save($menu, $masks) {
+      'weight' => (integer) $item['weight'],

I 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

larowlan’s picture

Re int vs integer it looks like core mostly does int but I see a few instances of integer in D7 contrib.

amjad1233’s picture

Assigned: Unassigned » amjad1233
Status: Needs work » Active

Awesome. Adding tests.

amjad1233’s picture

amjad1233’s picture

amjad1233’s picture

amjad1233’s picture

amjad1233’s picture

amjad1233’s picture

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

amjad1233’s picture

larowlan’s picture

Status: Active » Reviewed & tested by the community
Issue tags: -Needs tests

Assuming this comes back green, this looks RTBC to me.

amjad1233’s picture

amjad1233’s picture

Status: Reviewed & tested by the community » Needs work
amjad1233’s picture

Oops. Put the casting in the wrong place. Sorry.

But I promise I looked at the patch twice before submitting. ☹

amjad1233’s picture

amjad1233’s picture

Status: Needs work » Reviewed & tested by the community

RTBC based on @larwon's review.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/modules/simpletest/tests/menu.test
    @@ -1738,3 +1738,47 @@ class MenuTrailTestCase extends MenuWebTestCase {
    +  public static function getInfo() {
    ...
    +  function setUp() {
    

    Does D7 expect docblocks for these methods, or at the very least {@inheritdoc} ?

  2. +++ b/modules/simpletest/tests/menu.test
    @@ -1738,3 +1738,47 @@ class MenuTrailTestCase extends MenuWebTestCase {
    +    $modules = func_get_args();
    

    is this needed, seems premature to assume there's a child class

  3. +++ b/modules/simpletest/tests/menu.test
    @@ -1738,3 +1738,47 @@ class MenuTrailTestCase extends MenuWebTestCase {
    +  /**
    

    phpcs nit: needs an empty line between previous method

  4. +++ b/modules/simpletest/tests/menu.test
    @@ -1738,3 +1738,47 @@ class MenuTrailTestCase extends MenuWebTestCase {
    +      $this->assert(FALSE, "Menu weight is not being casted properly.");
    

    we can use $this->fail() here

  5. +++ b/modules/simpletest/tests/menu.test
    @@ -1738,3 +1738,47 @@ class MenuTrailTestCase extends MenuWebTestCase {
    +    $this->assert(TRUE, "Menu weight is being casted properly.");
    

    And $this->pass here

amjad1233’s picture

Added additional code improvements + phpcs as per @larowlan's feedback.

Thanks for that @larowlan.

amjad1233’s picture

Status: Needs work » Needs review
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

thanks @amjad1233, this looks good to me

craigra’s picture

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

mcdruid’s picture

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

        [message:protected] => SQLSTATE[23000]: Integrity constraint violation:
    1048 Column 'weight' cannot be null
        [string:Exception:private] => 

...snip...
    
                [3] => Array
                    (
                        [file] =>
    /path/to/drupal-7.x/includes/menu.inc
                        [line] => 3213
                        [function] => execute
                        [class] => InsertQuery_mysql
                        [type] => ->
                    )
    
                [4] => Array
                    (
                        [file] =>
    /path/to/drupal-7.x/modules/simpletest/tests/menu.test
                        [line] => 1768
                        [function] => menu_link_save
                    )

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.

  • mcdruid committed 51ae73c on 7.x
    Issue #3142821 by amjad1233, mcdruid, craigra, larowlan: PDO Exception...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed

Thank you!

Status: Fixed » Closed (fixed)

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