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

Streamline and enhance the plugin load procedure (rebased) #271

Merged
merged 5 commits into from
Jan 30, 2022

Conversation

totten
Copy link
Member

@totten totten commented Jan 14, 2022

(This is a rebased version of @christianwach's #260. I've simply copied/pasted his original description. At the bottom, I'll add some comments about changes/conflict-resolution.)

Overview

This PR should solve a couple of issues on Lab:

It replaces #205 for the above issues.

It also streamlines the plugin load procedure(s):

  • Adds the anonymous_user role and default capabilities on plugin activation.
  • wp civicrm install now sets wpLoadPhp and creates the Base Page.

It should be noted that there is still work to be done when CiviCRM is configured in "Multi-Domain" mode. The procedures to set up new Domains are more complex than CiviCRM in "WordPress Multisite with multiple separate databases" mode. The Base Page, for example, cannot be identified until the new Domain has been properly set up - because the new CiviCRM Domain reads the "root" Domain's settings until that point.

Having said that, this PR improves the experience for most developers most of the time and should help future progress on streamlining "Multi-Domain" mode.

Comments (Rebase/Conflict-Resolution)

There were two PRs going through contemporaneous review, #264 (which just changed the wp civicrm install) and #260 (which did code-cleanups and functional-updates across many files+functions... including wp civicrm install). Most files/functions/changes from #260 appear to merge cleanly with #264 - except for very harry conflicts around wp civicrm install.

So to make this PR, I did the following:

But there is one notable section that I did not reproduce:

Screen Shot 2022-01-14 at 1 45 32 PM

  • This looks more functional.
  • I don't sufficiently understand the motivation/problem-scenario to reproduce/evaluate.
  • For r-running wp civicrm install, I followed the same test procedure that I used in (dev/core#1615) wp-cli - Perform CLI installations using Civi\Setup #264, and I don't see any obvious problems. The wpLoadPhp setting appears reasonable in my environment:
    $ cv ev 'return Civi::settings()->get("wpLoadPhp");'
    "/Users/totten/bknix/build/wpmaster/web/wp-load.php"
    
  • It's possible that this last part is not needed any more -- ie it was drafted on top of the old installer; but the new installer is a separate implementation (and it's shared with the web-based install). eg Perhaps a prior fix aimed at web-based users is now sufficient for the CLI installer too.

christianwach and others added 4 commits January 14, 2022 12:53
…d::install)

This is a rebased/cherry-picked version of 921af8e. However,
it skips the changes in`wp-cli/civicrm.php`'s `CiviCRM_Command::install` because these
have conflicts.

I'll try to reproduce those changes as a separate/smaller commit.
This reproduces some of the cleanups from
921af8e - specifically, the parts dealing with
the method `CiviCRM_Command::install()`
@totten
Copy link
Member Author

totten commented Jan 15, 2022

I pushed up a patch which gets the 4.6.x upgrade-test to pass (1bdb8e9).

I spent some time running upgrades in different permutations (eg wrt to web-vs-cli as medium; wrt starting version; wrt ending version). With 1bdb8e9, it seems to be as good as the status quo -- ie

  • cli; 4.6.x => current master: completes without error
  • cli; 4.6.x => current master plus this PR: completes without error
  • web ui; 4.6.x => current master: completes without error, but dashboard is weird (JS errors; doesn't show everything)
  • web ui; 4.6.x => current master plus this PR: completes without error, but dashboard is weird (JS errors; doesn't show everything)

I haven't tracked where the issue with upgrading in the web ui originated -- but I can say these upgrades looked OK: 4.6=>5.13; 4.6=>5.27; 4.6=>5.39.

@kcristiano
Copy link
Member

@totten Thank you for reviewing this. I'll do a new r-run and should be able to get it merged soon.

@kcristiano
Copy link
Member

I've done another review and r-run work as expected.

Merging.

@kcristiano kcristiano merged commit cce8f82 into civicrm:master Jan 30, 2022
@totten
Copy link
Member Author

totten commented Feb 1, 2022

Yay! Thanks @kcristiano

@totten totten deleted the cw-init branch February 1, 2022 22:24
@pboling
Copy link

pboling commented Feb 2, 2022

For end users, what impact does this change have? Does this mean we'll be able to upgrade the CiviCRM plugin via the normal plugin update feature of WordPress?

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

Successfully merging this pull request may close these issues.

4 participants