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

[REF] Standalone System - graduate some functions from Civi\Standalone\Security to CRM_Utils_System_Standalone and Civi\Authx\Standalone #31127

Merged
merged 12 commits into from
Oct 10, 2024

Conversation

ufundo
Copy link
Contributor

@ufundo ufundo commented Sep 17, 2024

Overview

Attempt at consolidating some of the functions in CRM_Utils_System_Standalone , Civi\Standalone\Security and Civi\Authx\Standalone .

Before

  • Many of the functions for CRM_Utils_System_Standalone are implemented in Civi\Standalone\Security
  • Piecemeal handling in the CRM_Utils_System_Standalone for when standaloneusers extension is unavailable

After

  • System functions that it seems unlikely the user extension would want to change moved to the System class
  • \Civi\Standalone\Security keeps the juicy password algorithm logic
  • a reusable check for whether standaloneusers and authx are available, and more failsafes when not

Comments

I've edited to break out some of the more functional changes.

Copy link

civibot bot commented Sep 17, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Sep 17, 2024
@ufundo ufundo force-pushed the standalone-system-cleanup branch 6 times, most recently from 9629e8e to 5a2d2d4 Compare September 18, 2024 09:07
@ufundo ufundo added the run-standalone Civibot should setup demos+tests for Standalone label Sep 18, 2024
@ufundo ufundo marked this pull request as ready for review September 20, 2024 00:53
@ufundo
Copy link
Contributor Author

ufundo commented Sep 20, 2024

Have narrowed this PR to take out:

@ufundo ufundo changed the title Standalone System cleanup - graduate some functions from Civi\Standalone\Security to CRM_Utils_System_Standalone and Civi\Authx\Standalone [REF] Standalone System - graduate some functions from Civi\Standalone\Security to CRM_Utils_System_Standalone and Civi\Authx\Standalone Sep 21, 2024
@ufundo ufundo force-pushed the standalone-system-cleanup branch 2 times, most recently from dd7e4a3 to 4520316 Compare October 8, 2024 15:19
@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Oct 8, 2024
@colemanw
Copy link
Member

colemanw commented Oct 8, 2024

Code looks good. IMO better to merge it now while Standalone is still in beta.

@ufundo
Copy link
Contributor Author

ufundo commented Oct 8, 2024

Thanks @colemanw

There's a couple new merge conflicts that need working through, so switching to nearly-merge-ready just for the minute 🙂

@ufundo ufundo removed the merge ready PR will be merged after a few days if there are no objections label Oct 8, 2024
@ufundo
Copy link
Contributor Author

ufundo commented Oct 8, 2024

Hmm - I think this is now failing as a symptom of https://lab.civicrm.org/dev/core/-/issues/5493 - ActionObjectProvider::getEntities is getting a bit tied in knots during install as well as upgrade.

@ufundo ufundo force-pushed the standalone-system-cleanup branch 2 times, most recently from 6fcfd4c to 0af7f37 Compare October 9, 2024 09:18
@ufundo
Copy link
Contributor Author

ufundo commented Oct 10, 2024

@colemanw this is rebased and passing again - I think it's good to go 👍

@colemanw colemanw merged commit 820a556 into civicrm:master Oct 10, 2024
1 of 2 checks passed
@colemanw
Copy link
Member

Nice work @ufundo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master run-standalone Civibot should setup demos+tests for Standalone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants