Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] [com_modules] convert to prepared statement #25386

Merged
merged 54 commits into from
Oct 21, 2019
Merged

[4.0] [com_modules] convert to prepared statement #25386

merged 54 commits into from
Oct 21, 2019

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Jul 1, 2019

Summary of Changes

use prepared statement for SQL

Testing Instructions

test com_modules

Expected result

should work as before

Actual result

N/A

@@ -1074,7 +1088,8 @@ public function save($data)
$db->quoteName('#__modules', 'm') . ' ON ' . $db->quoteName('e.client_id') . ' = ' . (int) $table->client_id .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

' ON ' => ,

I would not use leftJoin any longer because it's an unneeded function call and should be removed in my opinion see joomla-framework/database#174

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i cannot see any leftJoin

Copy link
Member

@HLeithner HLeithner Jul 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starts two line above

@Quy
Copy link
Contributor

Quy commented Jul 6, 2019

Error 500
Go to Content > Site Modules > Filter Options > Select Menu Item > Create a Post

PHP Warning: mysqli_stmt::bind_param(): Number of variables doesn't match number of parameters in prepared statement in \libraries\vendor\joomla\database\src\Mysqli\MysqliStatement.php on line 430

$db->setQuery($query);
$menus = $db->loadColumn();

// Insert the new records into the table
foreach ($menus as $menu)
foreach ($menus as $i => $menu)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could rewrite this query to one execute call and add only ->values() in the loop something like:

$query->insert($db->quoteName('#__modules_menu'))
  ->columns($db->quoteName(['moduleid', 'menuid']))
foreach ($menus as $i => $menu)
{
        $query->values(implode(', ', [':newid' . $i, ':menu' . $i]))
	->bind(':newid' . $i, $newId, ParameterType::INTEGER)
	->bind(':menu' . $i, $menu, ParameterType::INTEGER);
}
$db->setQuery($query);
$db->execute();

->values((int) $table->id . ', 0');
->insert($db->quoteName('#__modules_menu'))
->columns($db->quoteName(['moduleid', 'menuid']))
->values(implode(', ', [':moduleid', 0]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implode is unneeded, I would only concatenate a string.

@@ -1074,7 +1088,8 @@ public function save($data)
$db->quoteName('#__modules', 'm') . ' ON ' . $db->quoteName('e.client_id') . ' = ' . (int) $table->client_id .
Copy link
Member

@HLeithner HLeithner Jul 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starts two line above

@alikon
Copy link
Contributor Author

alikon commented Jul 21, 2019

@Quy

Error 500
Go to Content > Site Modules > Filter Options > Select Menu Item > Create a Post

fixed now, can you confirm ?
there was an issue with bind and subqueries

@wilsonge wilsonge added this to Component in [4.0] Prepared Statements Oct 10, 2019
@chmst
Copy link
Contributor

chmst commented Oct 19, 2019

I have tested this item ✅ successfully on fd9f342


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25386.

1 similar comment
@waader
Copy link
Contributor

waader commented Oct 19, 2019

I have tested this item ✅ successfully on fd9f342


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25386.

@alikon
Copy link
Contributor Author

alikon commented Oct 20, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25386.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 20, 2019
@wilsonge wilsonge merged commit cace7f7 into joomla:4.0-dev Oct 21, 2019
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 21, 2019
@wilsonge wilsonge added this to the Joomla 4.0 milestone Oct 21, 2019
@alikon alikon deleted the patch-113 branch October 21, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants