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] Alter new Setup process to use CiviCRM's Event Dispatcher #20717

Merged

Conversation

seamuslee001
Copy link
Contributor

Overview

This changes CiviSetup to use Civi Core's event dispatcher

Before

Uses the EventDispatcher directly

After

Uses the CiviCore Event dispatcher which currently extends off of the Symfony Event DIspatcher

ping @totten @demeritcowboy

@civibot
Copy link

civibot bot commented Jun 27, 2021

(Standard links)

@civibot civibot bot added the master label Jun 27, 2021
@seamuslee001
Copy link
Contributor Author

also ping @kcristiano

@demeritcowboy
Copy link
Contributor

My guess given that setup sometimes duplicates code found elsewhere is that there was a desire to minimize dependence on something that hasn't been installed/bootstrapped yet. But it does already call some civi things so I can just give it a quick test to see.

@demeritcowboy
Copy link
Contributor

And just to clarify the description this was in the context of drupal 9 using a newer symfony which annoyingly switches the order of parameters giving deprecation notices.

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Jun 28, 2021
@demeritcowboy
Copy link
Contributor

Runs ok for me.

@totten
Copy link
Member

totten commented Jun 29, 2021

Most of the added bits in CiviEventDispatcher should be safe in de-minimis/non-booted/foreign environments - e.g. the dispatch-policy stuff and the variations on adding-listerners/subscribers are fairly internal/clean.

I was slightly concerned about the class being available -- but the setup protocol specifies that Civi classes shall be loadable before we get here (ie before Step 3 - Civi\Setup::init()). So that's fine.

As currently written, the part to watch out for is delegateToUF() - that gets wrapped up with various globals/singletons/DB-content/file-loads. If delegateToUF() was called inadvertently or prematurely (ie before Civi is installed), then you could get Strange Things.

Fortunately, the setup-dispatcher doesn't actually call delegateToUF(). All its events fall under civi.setup.*. The risk is that Future Developer gets confused about the two dispatchers (setup-dispatcher vs runtime-dispatcher). This seems like a simple defense/mitigation:

    self::$instance->dispatcher = new  CiviEventDispatcher();
    self::$instance->dispatcher->setDispatchPolicy(['/^civi\.setup\./' => 'run', '/./' => 'fail']);

Apply patch from Tim to limit what events are dispatched
@seamuslee001 seamuslee001 force-pushed the use_civi_event_dispatcher_setup branch from 53ec7cf to c95fbf9 Compare June 30, 2021 21:51
@seamuslee001
Copy link
Contributor Author

ok I have applied that additional guard from Tim and I think this is right to be merged now

@demeritcowboy
Copy link
Contributor

Runs ok for me.
Also ran uninstall.

@demeritcowboy demeritcowboy merged commit 5621bd7 into civicrm:master Jul 1, 2021
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.

3 participants