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] Fix deleting user groups #28687

Merged
merged 1 commit into from
Apr 17, 2020
Merged

[4.0] Fix deleting user groups #28687

merged 1 commit into from
Apr 17, 2020

Conversation

SharkyKZ
Copy link
Contributor

Fixes #28522.

Summary of Changes

Fixes query when deleting user groups.

Testing Instructions

Delete Manager user group.

Expected result

Group deleted and site continues to work.

Actual result

Group deleted but site is broken, modules are missing;

screen shot 2020-03-31 at 08 49 09

Documentation Changes Required

No.

@richard67
Copy link
Member

I have tested this item ✅ successfully on baa3012

Tested with MySQL 8.0.19 (both MySQLi and PDO) and PosgreSQL 11. All fine.


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

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

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

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

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

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

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

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

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

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.

@chmst
Copy link
Contributor

chmst commented Apr 15, 2020

I have tested this item ✅ successfully on baa3012


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 15, 2020
@adj9
Copy link

adj9 commented Apr 15, 2020

I have tested this item ✅ successfully on baa3012

I confirm, the site still works ... it has no interruptions. Even with a logout and subsequent login.


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

@wilsonge wilsonge merged commit 08c8653 into joomla:4.0-dev Apr 17, 2020
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 17, 2020
@zero-24 zero-24 added this to the Joomla 4.0 milestone Apr 17, 2020
@SharkyKZ SharkyKZ deleted the j4/query/usergroups branch April 17, 2020 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants