Skip to content

[4.0] Fix deleting user groups #28687

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

Merged
merged 1 commit into from
Apr 17, 2020
Merged

[4.0] Fix deleting user groups #28687

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
14 changes: 6 additions & 8 deletions libraries/src/Table/Usergroup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,10 +248,10 @@ public function delete($oid = null)

foreach ($ids as $id)
{
$replace[] = ',' . $db->quote("[$id,") . ',' . $db->quote('[') . ')';
$replace[] = ',' . $db->quote(",$id,") . ',' . $db->quote(',') . ')';
$replace[] = ',' . $db->quote(",$id]") . ',' . $db->quote(']') . ')';
$replace[] = ',' . $db->quote("[$id]") . ',' . $db->quote('[]') . ')';
$replace[] = ',' . $db->quote("[$id,") . ',' . $db->quote('[');
$replace[] = ',' . $db->quote(",$id,") . ',' . $db->quote(',');
$replace[] = ',' . $db->quote(",$id]") . ',' . $db->quote(']');
$replace[] = ',' . $db->quote("[$id]") . ',' . $db->quote('[]');
}

$query->clear()
Expand Down Expand Up @@ -280,12 +280,10 @@ public function delete($oid = null)

if (!empty($matchIds))
{
$rules = str_repeat('replace(', 4 * \count($ids)) . 'rules' . implode('', $replace);
$query->clear()
->update($db->quoteName('#__viewlevels'))
->set($db->quoteName('rules') . ' = :rules')
->whereIn($db->quoteName('id'), $matchIds)
->bind(':rules', $rules);
->set($db->quoteName('rules') . ' = ' . str_repeat('REPLACE(', 4 * \count($ids)) . $db->quoteName('rules') . implode(')', $replace) . ')')
Copy link
Contributor

@alikon alikon Apr 15, 2020

Choose a reason for hiding this comment

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

we are loosing the prepared bind in this way

Copy link
Member

@richard67 richard67 Apr 15, 2020

Choose a reason for hiding this comment

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

It is safe, there is no user input involved, and it makes it work. So I am ok with it. Just my 5 cent.

Copy link
Contributor

@chmst chmst Apr 15, 2020

Choose a reason for hiding this comment

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

The only other way I found is a foreach over all records with updating every item. Ir does no harm, because there are not many viewlevels, but as @richard67 says, this is safe.

Copy link
Member

@richard67 richard67 Apr 15, 2020

Choose a reason for hiding this comment

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

@alikon Are you convinced? May I set it RTC? Or do you want to discuss that with @SharkyKZ ?

Copy link
Contributor

@alikon alikon Apr 15, 2020

Choose a reason for hiding this comment

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

no i'm not, but that don't prevent to set this as RTC cause there are 2 tests
imho despite there are no clear user input, but there are variables then they should be validated on the server side like any other variable parameters

Copy link
Member

@richard67 richard67 Apr 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@SharkyKZ Was the problem in the wrong ')' at the end of each of these $replace[] = statements? Or was it in the prepared statement below? Or both? Maybe it can be changed bacl to prepared statement?

Copy link
Contributor Author

@SharkyKZ SharkyKZ Apr 15, 2020

Choose a reason for hiding this comment

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

Because of prepared statement. It's possible to keep prepared statement but there's not much point as you've already noticed.

Copy link
Member

@richard67 richard67 Apr 15, 2020

Choose a reason for hiding this comment

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

I set it to RTC, we can later optimize that again if we want. Now it fixes a really ugly bug.

->whereIn($db->quoteName('id'), $matchIds);
$db->setQuery($query);
$db->execute();
}
Expand Down