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] Ship Flexmailer extension with Core #17669

Merged
merged 1 commit into from Jul 1, 2020

Conversation

seamuslee001
Copy link
Contributor

Overview

This ships flexmailer with core whilst still allowing sysadmins to enable it.

Before

Flexmailer not shipped with core

After

Flexmailer shipped with core

ping @totten @eileenmcnaughton this is as I believe we discussed

@civibot
Copy link

civibot bot commented Jun 20, 2020

(Standard links)

@civibot civibot bot added the master label Jun 20, 2020
@eileenmcnaughton
Copy link
Contributor

@seamuslee001 I think we should enable on install for new installs - but I'm happy for that to come later. Possibly even a release later since we need to do some checks about the path caching issue @jusfreeman mentioned

@mattwire
Copy link
Contributor

@seamuslee001 I thought we were going to merge civicrm/org.civicrm.flexmailer#53 and resolve civicrm/org.civicrm.flexmailer#59 per https://lab.civicrm.org/dev/core/-/issues/1825 before moving this into core?

@mattwire
Copy link
Contributor

Also ping @mlutfy ^

@seamuslee001
Copy link
Contributor Author

@mattwire in the discussions I had had with Tim and Eileen they didn't raise those PRs I'm just trying to move this project along, either way I would be fine

@mlutfy
Copy link
Member

mlutfy commented Jun 20, 2020

Seems good to merge to core now, and change the setting in a subsequent release? (to do one big change at the time)

@mattwire
Copy link
Contributor

@mlutfy Makes sense to change the setting later - but can we get the refactor PR merged before pulling into core? It reduces the code required by flexmailer and will need significant re-work to re-do it against core

@mlutfy
Copy link
Member

mlutfy commented Jun 20, 2020

Hmm true, I had assumed we would continue tracking PRs separately (in the flexmailer project), but there isn't any advantage to that..

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 style warnings... looks like you brought in pull 53 & we should ideally get this done before tomorrow's rc

@mlutfy
Copy link
Member

mlutfy commented Jul 1, 2020

We should test that this issue does not affect upgrades:
civicrm/org.civicrm.flexmailer#62

If we are renaming the extension, we might get away with it, since the old extension will be removed, cache flushed, then new one enabled.

@agileware-justin
Copy link
Contributor

Mosaico has a dependency on Flexmailer, so how would the rename impact on Mosaico?

I would really like to test this change before it ships, recent issues with ExportUI and Flexmailer need to be avoided as much as possible.

For reference:

  1. Just noting that when the last extension moved into Core, exportui caused a lot of "CiviCRM down" issues due to the location change.
  2. Regression: "Uncaught Error: Class 'Civi\FlexMailer\Services' not found" - Caused CiviCRM outages
  3. Fixes #61: partial revert the old autoloader to avoid cache problems

@eileenmcnaughton
Copy link
Contributor

I'm going to merge this as merging it now will put it in the core tarball and we can discontinue the other repo. I note this has no code to enable the extension, either on install or on upgrade. I'm open to trying to leaving that until after the rc or fixing before the rc but merging this just means we have the code in the new place but are not (yet) determining which is enabled

@eileenmcnaughton eileenmcnaughton merged commit 0732f87 into civicrm:master Jul 1, 2020
@eileenmcnaughton eileenmcnaughton deleted the flexmailer_core branch July 1, 2020 22:43
@seamuslee001
Copy link
Contributor Author

I would also add that my understanding is the following

  1. the extension isn't getting renamed as part of this just the folder which has absolute no implications
  2. If you have flexmailer already downloaded in your ext directory it should continue to use that version not the one that is shipped with core

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants