Streamline and enhance the plugin load procedure

[christianwach]

Overview

This PR should solve a couple of issues on Lab:

• 404 Error when I use the Api v4...
• Base page is only created on main site...

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.

[kcristiano]

jenkins test this please

[kcristiano]

@seamuslee001 Can you help me a bit here? I can build with buildkit, but am unsure how to run the E2E tests locally.

While a very minor functional change, this has a great deal of cleanup and I'd like to find out what is triggering the test failures.

[kcristiano]

This is the error I get

PHP Fatal error:  Uncaught RuntimeException: Suite includes end-to-end tests (E2E\Api4\PermissionTest) which do not support CIVICRM_UF=UnitTests.

for:

env CIVICRM_UF=UnitTests phpunit6 tests/phpunit/E2E/AllTests.php

[christianwach]

Hmm, hopefully 5d8bef7 fixes the failure.

[christianwach]

Jenkins re test this please

[christianwach]

Hmm, looking at the console output, it seems like this step is throwing a fatal error:

wp eval '$c=[civi_wp(), "add_wpload_setting"]; if (is_callable($c)) $c();'

Which is odd because the currently-targeted add_wpload_setting method isn't even callable!

Ah, hold on... I think I see what's going on...

What's going is that wp plugin activate civicrm does indeed successfully activate the plugin - but because the plugins_loaded action has already fired, CiviCRM_For_WordPress::setup_instance() doesn't get called during that WP-CLI call. The subsequent call does fully load the plugin - then calls the eval statement, but the error had already happened in the original load. It just happens that the method in the eval was the same one throwing the error in the original load.

I think 1b0d4f0 should fix the above error.

@seamuslee001 Does the Jenkins script need updating perhaps? From what I can tell, the above statement should be replaced with something like:

wp eval '$c=[civi_wp()->admin, "add_wpload_setting"]; if (is_callable($c)) $c();'
wp eval '$c=[civi_wp()->basepage, "create_wp_basepage"]; if (is_callable($c)) $c();'

This would set both the wpLoadPhp and wpBasePage settings and create the Base Page.

[christianwach]

Jenkins re test this please

[christianwach]

@seamuslee001 The current error seems unrelated:

The scheduled date and time is in the past

Any ideas?

[seamuslee001]

@christianwach that is fine (that should happen in the Karma tests) but the problem seems to be about the insertion of a new setting group not having a default value or something. thoughts @totten

[christianwach]

@seamuslee001 @totten The key change in the code here is that I switch the way the wpLoadPhp setting is added from a call to CRM_Core_BAO_Setting::setItem() (because it was triggering deprecated warnings in the logs) to an API v3 civicrm_api3("Setting", "create", ...) call. I assume this to be the preferred method since it's done that way throughout Core.

[christianwach]

461b7e0 reverts the way in which the wpLoadPhp setting is created to the deprecated setItem() method because I didn't want the civicrm_api() call to hold up assessment of this PR. However it seems that the civicrm_api() call is not the cause, because the setItem() method produces the same test failure. Not sure how to proceed from here.

[kcristiano]

@totten Do you have any insights on hwy tests are failing on this PR? I cannot seem to figure it out.

[christianwach]

@kcristiano @totten I've made a couple of force-pushed changes to avoid what seem to be changes in the way that civilint now works - it was complaining about optional params before required params in method signatures, which it hasn't done before.

I'm still stumped why the tests are failing.

[seamuslee001]

@christianwach that thing about the optional before required is actually not Civilint but php8, the test boxes now have php8 on as the PR testing version of php

[christianwach]

Jenkins re test this please

[christianwach]

Now that there's a recent test success to compare against, diffing the console output of this and #264 shows the failure being 4.6.36-setupsh.sql.bz2 ERROR.

@kcristiano @seamuslee001 @totten Anyone know what this means and how I can debug what's going on?

[kcristiano]

That looks to like wpLoadPhp is failing to be set in the 4.6.36 upgrade test. Based on when that setting was introduced, it should work and I am unsure why this fails:

[code] => -1
    [message] => DB Error: unknown error
    [mode] => 16
    [debug_info] => INSERT INTO civicrm_setting (`component_id`,`contact_id`,`created_date`,`created_id`,`domain_id`,`id`,`is_domain`,`name`,`value`) VALUES
(NULL,NULL,"20211221104414",NULL,"1",NULL,"1","wpLoadPhp","s:58:\"/home/jenkins/bknix-dfl/build/wp-260-48og8/web/wp-load.php\";")
 [nativecode=1364 ** Field 'group_name' doesn't have a default value]
    [type] => DB_Error
    [user_info] => INSERT INTO civicrm_setting (`component_id`,`contact_id`,`created_date`,`created_id`,`domain_id`,`id`,`is_domain`,`name`,`value`) VALUES
(NULL,NULL,"20211221104414",NULL,"1",NULL,"1","wpLoadPhp","s:58:\"/home/jenkins/bknix-dfl/build/wp-260-48og8/web/wp-load.php\";")
 [nativecode=1364 ** Field 'group_name' doesn't have a default value]
    [to_string] => [db_error: message="DB Error: unknown error" code=-1 mode=callback callback=CRM_Core_Error::exceptionHandler prefix="" info="INSERT INTO civicrm_setting (`component_id`,`contact_id`,`created_date`,`created_id`,`domain_id`,`id`,`is_domain`,`name`,`value`) VALUES
(NULL,NULL,"20211221104414",NULL,"1",NULL,"1","wpLoadPhp","s:58:\"/home/jenkins/bknix-dfl/build/wp-260-48og8/web/wp-load.php\";")
 [nativecode=1364 ** Field 'group_name' doesn't have a default value]"]

The setting is defined https://github.com/civicrm/civicrm-core/blame/0d0eca748e1bf13765cc6ade8cf7bd9ad5800557/settings/Core.setting.php#L905 and gropu is set to core

I am hoping others can get into more detail.

[kcristiano]

related civicrm/civicrm-buildkit#664

[seamuslee001]

Jenkins re test this please

[kcristiano]

Looks like the buildkit fix didn't help here - any way we can see why the 4.6.36 upgrade is failing? I'd like to get past this as I had hoped we'd have this in 5.45 but we've missed that.

[seamuslee001]

What happens if we change this to be

Suggested change

	CRM_Core_BAO_Setting::setItem($path, 'CiviCRM Preferences', 'wpLoadPhp');
	Civi::settings()->set('wpLoadPhp', $path);

[totten]

(@christianwach) Now that there's a recent test success to compare against, diffing the console output of this and #264 shows the failure being 4.6.36-setupsh.sql.bz2 ERROR.

Anyone know what this means and how I can debug what's going on?

Not sure this was addressed directly; just to answer....

The test suite includes a series of database snapshots generated from historical releases (https://github.com/civicrm/civicrm-upgrade-test). IIRC, the PR selects snapshots from 4.6.12 through current; then, for each, it loads the DB and tries to run an upgrade.

(The kind of bug that it should call-out involves bootstrap-logic or upgrade-logic which relies on some new service or schema. For example, suppose v5.75.0 adds an entity FooBar and uses FooBar during bootstrap; it calls an API which calls a BAO which formulates a query SELECT foo, bar FROM civicrm_foobar. Now a user downloads 5.75.0 and tries to upgrade their DB from 4.6.12. Their 4.6 DB does not have civicrm_foobar - so bootstrap always fails - so the site becomes unusable, and it's not possible to run the upgrader. (EDIT) The general resolutions are either (a) keep the boot simple - don't add more complexity or (b) sprinkle conditionals around the new bits or (c) change the system-requirements for upgrading.)

To reproduce, I think you could manually load the snapshot DB and run wp civicrm upgrade-db. Similarly, civicrm-upgrade-test.git has a README/instructions for using the script. In civibuild environment, it's a bit more automated - any one of the following commands should reproduce (on a build named wpmaster):

## Run upgrade with a specific DB snapshot
civibuild upgrade-test wpmaster 4.6.36-setupsh.sql.bz2
civibuild upgrade-test wpmaster /path/to/buildkit/vendor/civicrm/upgrade-test/databases/4.6.36-setupsh.sql.bz2

## Run all DB snapshots between 4.6.12 and 4.7.0
civi-test-run -j /tmp/junit -b wpmaster upgrade@4.6.12..4.7.0

FWIW, I can confirm that I get a similar failure when running locally.

[seamuslee001]

@totten my finding was that because we are now trying to set the setting before we start the upgrade that this if fails and as such https://github.com/civicrm/civicrm-core/blob/master/Civi/Core/SettingsBag.php#L419 group_name doesn't get applied to the insert SQL

[kcristiano]

@totten Thanks for the anlaysis I'll try and look over the next few days.

I've also determined that we had put a work around in buildkit https://github.com/civicrm/civicrm-buildkit/blame/master/app/config/wp-demo/install.sh#L60

However, this PR fixes this and that line causes a fatal error in te tests as far as I can see. I was attempting to do a regex to add logic to only run if the version is less than or equal to 5.45, but so far my regex fails. I can get the logic to not run that line if $CIVI_VERSION -eq 'master' to not run and that avoids the fatal error locally. I'll try that with your above tests.

I'll also take a look at resolving the merge conflicts in wp-cli as I do want the clean up that was done in this PR.

Any guidance on the regex to get a string version the is le a value would be appreciated.

[totten]

For version comparisons in bash -- I've found it more effective to pass over to PHP's version_compare() which handles multiple digits, alphas, betas, etc. There are a couple examples in civibuild.lib.sh, eg this creates a setting if version is <4.7.0:

  if cv ev 'exit(version_compare(CRM_Utils_System::version(), "4.7.0", "<") ?0:1);' ; then
    cv api setting.create versionCheck=0 debug=1
  fi

But that example uses cv ev and CRM_Utils_System - which may not work early-on. You can swap cv ev with php -r, but swapping CRM_Utils_System::version() is a little more tricky. I'd use the civicrm_get_ver helper.

Perhaps something like this:

###############################################################################
## Compare the current Civi source-version to some reference amount
##
## usage: civicrm_compare_ver OPERATOR REFERENCE_VER
## ex:    if civicrm_compare_ver '<' '5.20' ; then
function civicrm_compare_ver() {
  env ME=$(civicrm_get_ver "$CIVI_ROOT") \
    OP="$1" \
    REF="$2" \
    php -r 'exit(version_compare(getenv("ME"),getenv("REF"),getenv("OP"))?0:1);'
  return $?
}

if civicrm_compare_ver '<' '5.20' ; then echo "Less than 5.20" ; else echo "More than 5.20" ; fi
if civicrm_compare_ver '>' '5.20' ; then echo "More than 5.20" ; else echo "Less than 5.20" ; fi
if civicrm_compare_ver '<' '5.50' ; then echo "Less than 5.50" ; else echo "More than 5.50" ; fi
if civicrm_compare_ver '>' '5.50' ; then echo "More than 5.50" ; else echo "Less than 5.50" ; fi

(NOTE: Edited civicrm_compare_ver() for improved escaping)

[kcristiano]

@christianwach christianwach#10 is my attempt to fix the merge conflicts - nver mind - I missed a section.

I'll come back to this later.

Is it possible to cut this as multiple PRs? The NFC changes and then the load changes? The error that I expected to be due to how we are building the site turned out to be a red herring so I think we need to attack in smaller pieces.

[totten]

@christianwach @kcristiano I took a stab at rebasing in #271.

[kcristiano]

@christianwach can we close this in favor of #271

[kcristiano]

Merged #271 which replaces this PR