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

CiviCRM Standalone: base classes #22227

Merged
merged 7 commits into from
Jul 11, 2022
Merged

Conversation

mlutfy
Copy link
Member

@mlutfy mlutfy commented Dec 8, 2021

Overview

As CiviCRM has evolved, and so has content management systems, and how organizations manage their CRM, it seems like a good time to bring CiviCRM Standalone back.

  • A lot of organizations do not expose their CRM, or they might only expose specific forms, on a sub-domain.
  • Managing a website and a CRM coupled together can be challenging.
  • CMSes have become more and more complex, with increasing conflicts between CiviCRM and the CRM. Having to install a CMS before installing a CRM is extra friction. Many people view Drupal9 as too complicated, and WordPress as risky.

More information:
https://lab.civicrm.org/dev/core/-/wikis/standalone

Technical Details

  • A lot more work required, obviously
  • Current implementation is very insecure. It copies some classes from the UnitTests, to bypass authentication, for example.

@civibot
Copy link

civibot bot commented Dec 8, 2021

(Standard links)

@civibot civibot bot added the master label Dec 8, 2021
@mlutfy mlutfy marked this pull request as draft December 8, 2021 20:01
@mlutfy
Copy link
Member Author

mlutfy commented Dec 9, 2021

I created a repo here to help get started, and also has the web/index.php to bootstrap CiviCRM.

Edit: repo link: https://github.com/mlutfy/civicrm-standalone

Doc updated here: https://lab.civicrm.org/dev/core/-/wikis/standalone

@mlutfy mlutfy force-pushed the standalone branch 5 times, most recently from 88176a3 to 6fdd01f Compare December 9, 2021 16:40
@mlutfy mlutfy changed the title WIP: CiviCRM Standalone: base classes CiviCRM Standalone: base classes Dec 9, 2021
@mlutfy mlutfy marked this pull request as ready for review December 9, 2021 17:08
@mlutfy
Copy link
Member Author

mlutfy commented Dec 9, 2021

odd, I don't think the test fail relates?

CRM_Core_BAO_CustomFieldTest::testFileDisplayValueNoDescription
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'              <a href="/index.php?q=civicrm/file&amp;reset=1&amp;id=1&amp;eid=3&amp;fcs=7ca559b89d9e6c1b88000a0028e7172732a600476a3670aafc4c3ce8325b31a0_1639081466_168" title="test_file.txt">\n
+'              <a href="/index.php?q=civicrm/file&amp;reset=1&amp;id=1&amp;eid=3&amp;fcs=7ca559b89d9e6c1b88000a0028e7172732a600476a3670aafc4c3ce8325b31a0_1639081467_168" title="test_file.txt">\n
                 <i class="crm-i fa-paperclip" aria-hidden="true"></i>\n
               </a>

@demeritcowboy
Copy link
Contributor

It's a timing rollover - known issue - note 1639081466_168 vs 1639081467_168. It's a little hard to fix because it would touch on a previous security fix.

@demeritcowboy
Copy link
Contributor

By the way where is the "repo" you mention above?

@mlutfy
Copy link
Member Author

mlutfy commented Dec 9, 2021

Here's the repo: https://github.com/mlutfy/civicrm-standalone

@mlutfy
Copy link
Member Author

mlutfy commented Dec 10, 2021

jenkins, test this please

1 similar comment
@mlutfy
Copy link
Member Author

mlutfy commented Dec 24, 2021

jenkins, test this please

*/
public function getLoginURL($destination = '') {
$query = $destination ? ['destination' => $destination] : [];
return \Drupal\Core\Url::fromRoute('user.login', [], ['query' => $query])->toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

This comes up as a fatal error on an entityref field if you choose e.g. the New Individual popup. Maybe for now it could be return CRM_Utils_System::url('civicrm/todo/user/login', 'reset=1'); or something like that, just so the popup works.

Copy link
Member Author

Choose a reason for hiding this comment

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

@demeritcowboy Thanks - I updated to civicrm/user/login, since I think that makes sense as a login URL.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@mlutfy
Copy link
Member Author

mlutfy commented Jun 24, 2022

Diving back into this is hard. I had forgotten that the installer now works, but I had not committed the file.

Example:

image

after entering DB credentials:

image

However, there are shenanigans after installation - some path is not correctly set, so the menu and CSS are not loading correctly:

image

ah - and because of my composer.lock on civicrm-standalone, it's building on 5.44. Need to re-test on latest.

@mlutfy
Copy link
Member Author

mlutfy commented Jun 29, 2022

Fixed the linting warning.

* Standalone specific stuff goes here.
*/
class CRM_Utils_System_Standalone extends CRM_Utils_System_Base {

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you've removed registerPathVars() in this latest update. It means that [cms.root] is now null, which kind of makes sense here, but we'll need to hunt down where it's used since it generates errors. Or perhaps it should point to web.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm I have to admit I don't really recall. I grepped a bit in the codebase about cms.root and it seems there more for convenience, for settings, but I'd be surprised if extensions used it? (there are better functions for generating URLs and getPaths)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I took notes but it might have been during install. And I seem to remember something about the system settings - directories page (which would make sense there).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I was using php 8 this time around, which is less forgiving. I didn't intend to, just it happened to be in my path from something else and then composer(👿) locked everything to php 8+ so it won't even run with a web server using 7.

@mlutfy
Copy link
Member Author

mlutfy commented Jul 7, 2022

jenkins, test this please

@eileenmcnaughton
Copy link
Contributor

Where is this at - to the extent that it only touches stuff within the standalone subsystem I think we can have a more 'work-in-progress' attitude - but if this is exposing it as available to install then I guess the concern is we might be offering something that we are not yet delivering.

For background I think this has concept-approved in terms of 'we do want standalone to work' - so it's a question of how to support work on it without creating confusion

@demeritcowboy
Copy link
Contributor

I see pluses and minuses to merging. There's the ones you've mentioned, and then

  • this one is unlikely to go stale in its current form
  • and also even if merged it will quickly get replaced by another one, although maybe smaller
  • the latest update here broke some things (in standalone only), but need to figure out what cms.root should mean in standalone anyway

After writing that out I see it leans a bit towards leaving here. But also for the concern about advertising it I don't see anything in here that makes it visible to users.

I'm 50/50 personally.

@mlutfy
Copy link
Member Author

mlutfy commented Jul 11, 2022

Personally, I'd favour merging, because the various decisions that will go into Standalone will be muddied if it's all behind a single PR. For example, cms.root could be a discussion in itself?

@demeritcowboy
Copy link
Contributor

🚀

@demeritcowboy demeritcowboy merged commit c36e958 into civicrm:master Jul 11, 2022
@eileenmcnaughton eileenmcnaughton deleted the standalone branch July 12, 2022 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants