menu_get_item() attempts to load placeholders when called from Layout::save().

[jlfranklin]

Describe your issue or idea

The core bug here is a menu item placeholder % is used as a SQL string %. (But not as a wildcard.)

The Layout::save() function calls menu_get_item() with a path of node/%. get_menu_item() parses the path, matches it to the router item for node/% and eventually calls node_load_multiple('%') The node subsystem dutifully builds and executes a query with WHERE (base.nid IN ('%')).

Steps to reproduce (if reporting a bug)

Run the tests with the PostgreSQL driver, possibly with MySQL's Strict SQL mode enabled, or by adding some debugging code in node_load_multiple()

Actual behavior (if reporting a bug)

I discovered this bug while trying to get the tests to all pass while using the PostgreSQL driver imported back from Drupal 7. I know PostgreSQL is not supported here, but this bug is not in the database or driver, it is in the path handling of the menu subsystem.

The clue I had was the NodeBlockFunctionalTest::testRecentNodeBlock() test failed with an exception:

Exception PDOExcepti entity.controller  199 DefaultEntityController->load()
    SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax
    for integer: "%"
    LINE 5: WHERE  (base.nid IN  ('%'))

MySQL executes this query without an error, returning no rows. Enabling MySQL's Strict SQL mode may trigger a warning.

Expected behavior (if reporting a bug)

PostgreSQL tests pass, MySQL doesn't bother with an obviously bad query.

Relevant version/system information (if applicable)

[jlfranklin]

One line patch in PR fixes the immediate issue.

[jlfranklin]

This fixes a number of tests all for the same reason:

• FieldBlockTestCase
• LayoutInterfaceTest (x9)
• NodeBlockFunctionalTest (x2)

[jlfranklin]

I may have a better patch for this.

[quicksketch]

Thanks @jlfranklin! Even if PostGres is not supported, if we're making an invalid query at all it's still a bug. I'm marking this needs work to see what your better patch is, please move to needs review when ready.

[jlfranklin]

Re-rolled the PR.

[bugfolder]

Given the amount of time this has been around, I first checked to see if it's still a problem, and it is.

Checked by creating a layout with path node/% and then adding dpm($nids) to the beginning of function node_load_multiple($nids, $conditions, $reset). Saving the layout resulted in $nids being reported as the string % per the OP, which is doubly wrong because the docblock for that function says that $nids should be An array of node IDs or FALSE to load all the nodes..

Applying the patch of the PR fixes the issue; no bogus call (no call at all) to node_load_multiple(). WFM.

Code reviewed, LGTM.

RTBC.

[bugfolder]

@JFranklin, could you edit the first comment in your PR backdrop/backdrop#1961 to change "Addresses" to "Fixes" (which is a magic word that will connect that PR to this issue.)

[quicksketch]

Thanks @bugfolder for reviewing this old PR. I merged in 1.x into the PR and we'll see if it's passing tests still.

[bugfolder]

Oh look, testLayoutUpgrade() is failing one of the PHP versions. Who would have guessed? 😉

[quicksketch]

All other tests are passing. We have a separate issue for testLayoutUpgrade() now at #6293.

I merged backdrop/backdrop#1961 into 1.x and 1.26.x. Thanks @jlfranklin and @bugfolder!