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.

Sign up for GitHub

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

Clean up ModulesModelModule class #13380

Merged
merged 1 commit into from
Jan 14, 2017
Merged

Clean up ModulesModelModule class #13380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions administrator/components/com_modules/models/module.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,8 +426,8 @@ public function duplicate(&$pks)
->from($db->quoteName('#__modules_menu'))
->where($db->quoteName('moduleid') . ' = ' . (int) $pk);

$this->_db->setQuery($query);
$rows = $this->_db->loadColumn();
$db->setQuery($query);
$rows = $db->loadColumn();
Copy link
Contributor

Choose a reason for hiding this comment

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

for further cleanup i think you could use just

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

Copy link
Contributor Author

@joomdonation joomdonation Dec 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your suggestion @andrepereiradasilva . The suggested change will work OK, however, seems we don't have that rule in core code. You can see on the same file (and many other places in Joomla core), we usually call $db->setQuery method to set query object before running the next method

With that said, I will leave it as how it is for now unless maintainers request for change.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure was just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you reviewed the code, could you please help marking the test result, too?


foreach ($rows as $menuid)
{
Expand All @@ -448,11 +448,11 @@ public function duplicate(&$pks)
->columns($db->quoteName(array('moduleid', 'menuid')))
->values($tuples);

$this->_db->setQuery($query);
$db->setQuery($query);

try
{
$this->_db->execute();
$db->execute();
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

$db->setQuery($query)->execute();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same with my reply above. Plus I think we should only add the command to try catch block if that command could throw exception. In this case, $db->setQuery doesn't, so I will leave it outside the try catch block.

}
catch (RuntimeException $e)
{
Expand Down Expand Up @@ -686,8 +686,7 @@ public function getItem($pk = null)
}
else
{
$app = JFactory::getApplication();
$app->redirect(JRoute::_('index.php?option=com_modules&view=modules', false));
JFactory::getApplication()->redirect(JRoute::_('index.php?option=com_modules&view=modules', false));

return false;
}
Expand Down Expand Up @@ -1016,18 +1015,16 @@ public function save($data)
// Get the sign of the number.
$sign = $assignment < 0 ? -1 : 1;

// Preprocess the assigned array.
$tuples = array();
$query->clear()
->insert($db->quoteName('#__modules_menu'))
->columns($db->quoteName(array('moduleid', 'menuid')));

foreach ($data['assigned'] as &$pk)
{
$tuples[] = '(' . (int) $table->id . ',' . (int) $pk * $sign . ')';
$query->values((int) $table->id . ',' . (int) $pk * $sign);
}

$this->_db->setQuery(
'INSERT INTO #__modules_menu (moduleid, menuid) VALUES ' .
implode(',', $tuples)
);
$db->setQuery($query);

try
{
Expand All @@ -1046,7 +1043,7 @@ public function save($data)
$dispatcher->trigger($this->event_after_save, array($context, &$table, $isNew));

// Compute the extension id of this module in case the controller wants it.
$query = $db->getQuery(true)
$query->clear()
->select('extension_id')
->from('#__extensions AS e')
->join('LEFT', '#__modules AS m ON e.element = m.module')
Expand Down