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

dev/backdrop#8 - Move session start into CMS-specific classes. #23620

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

bugfolder
Copy link
Contributor

Overview

Fixes https://lab.civicrm.org/dev/backdrop/-/issues/8 (or rather, a part of it; the rest is fixed by civicrm/civicrm-backdrop#159, which is a PR against a different repository).

Before

Previously, when CRM_Utils_System::authenticate() was called, it started a session. This creates warning messages in Backdrop CMS (though apparently not other CMSes). The solution is to move the session-starting code into CMS-specific classes and leave it out of the Backdrop-specific class.

After

The session-starting code is now in the CMS-specific classes but not Backdrop. There was a suggestion in the comments (suggesting this exact change) that suggested it would not be necessary in the UnitTest class, so I did not put a copy of the code in that class.

@civibot civibot bot added the master label May 28, 2022
@civibot
Copy link

civibot bot commented May 28, 2022

(Standard links)

@demeritcowboy demeritcowboy changed the title Issue 8: move session start into CMS-specific classes. dev/backdrop#8 - Move session start into CMS-specific classes. Jun 2, 2022
@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • Runs ok for me with e.g. something like php bin/cli.php -u admin -p XXX -e Job -a fetch_activities which hits this code.
      • But I don't have a backdrop to test, which is what this PR is about. So have only put merge-ready.
  • Developer standards
    • [r-tech] PASS
    • [r-code] Non-blocking comment
      • I might reduce the copy/paste a bit by adding a function in Base.php, i.e.
        protected function civicrmInitSession() {
          /* Before we do any loading, let's start the session and write to it.
           * We typically call authenticate only when we need to bootstrap the CMS
           * directly via Civi and hence bypass the normal CMS auth and bootstrap
           * process typically done in CLI and cron scripts. See: CRM-12648
           */
          CRM_Core_Session::singleton()->set('civicrmInitSession', TRUE);
        }
        so that instead of copying the whole block you only need to add one line $this->civicrmInitSession(); in each of the appropriate subclasses.
    • [r-maint] N/A
    • [r-test] PASS

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Jun 2, 2022
@demeritcowboy demeritcowboy merged commit 4bb583a into civicrm:master Jun 7, 2022
@bugfolder bugfolder deleted the 8_move_session_start branch June 7, 2022 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants