dev/core#4993 block Sync CMS Users form and functionality on Standalone #29351
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/4993 |
ef3c3dd to
662ec86
Compare
| $config = CRM_Core_Config::singleton(); | ||
| if ($config->userFramework === 'Standalone') { | ||
| throw new \Exception('No CMS to sync users to when using CiviCRM Standalone'); | ||
| } |
There was a problem hiding this comment.
I know this is a draft but just noting two things:
- A year or two ago we tried to remove most of the
if Drupalfrom various files to make it more OO-ey. I know in standalone sometimes it hasn't booted yet and so there's no choice, but here I think it would be better to do something likeif !$config->userSystem->allowsUserSync()and then CRM_Utils_System_Base::allowsUserSync would return true, and CRM_Utils_System_Standalone::allowsUserSync() would return false. In theory this also allows mocking both variations in backend unit tests (although it's not currently set up for more than one unittest mock). - In forms we usually use CRM_Core_Error::statusBounce instead of throwing exceptions. Even if it were to throw an exception it's preferred to use CRM_Core_Exception.
There was a problem hiding this comment.
thanks for these pointers 👍
There was a problem hiding this comment.
This got me thinking -- it's specifically standaloneusers that the syncing is incompatible with. So I've made it conditional on that, leaving open the door for alternatives where syncing is a thing (external auth providers maybe?)
There was a problem hiding this comment.
@ufundo 🤯 I think external auth providers might be dealt with in a way other than replacing the whole of standaloneusers. But ok!
dab1344 to
7da08f0
Compare
7da08f0 to
6e8e15f
Compare
6e8e15f to
7d8c738
Compare
|
Tested, this does hide the menu item and prevent access to the page. Merging, thanks @ufundo |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Block CMS user sync form when using Standaloneusers extension, because it doesn't do anything. And the most secure code is no code.
There was a note about the case when the contact of a user was deleted -- but deleting a contact with a user attached is prevented with #29026