Changes for Composer

[jackrabbithanna]

Overview

WIP PR working on https://lab.civicrm.org/dev/wordpress/-/issues/155

Changes to make the plugin work with composer installation.

Assuming removing the ./civicrm directory from the plugin directory, so Core is in vendor.

Technical Details

If the $civicrm_root global variable in the civicrm.settings.php is changed to point to the ./vendor/civicrm/civicrm-core/ directory

[jackrabbithanna]

@JSProffitt

[JoeMurray]

@kcristiano ^

[totten]

I'm just giving some quick reaction (without re-tracing paths) - in no particular order:

• The kind of things to look out for when updating bootstrap steps would be...

  • The different file-layouts (eg zip-layout vs composer-layout)
  • Support for different styles of startup/entry (web-based, wp-cli, cv, legacy-extern)
  • Different parts of lifecycle (installation; regular request)

• For reference, in Civi-Drupal, the equivalent operation involves a small search:

  https://github.com/civicrm/civicrm-drupal/blob/eee574762ae75e3e46d9aa8beff1c28b713d70cf/civicrm.module#L329-L349

• This patch may actually work for all of these, so don't go making any big change based on my speculation. (But if some these edges don't work, then the fix maybe something adaptive - like on Civi-Drupal.)

[christianwach]

So I'm trying to get my head round this (I'm not familiar with Composer file structures) but if you're going to avoid assumptions about where CivICRM Core is located, then you'll also need to modify this line too:

civicrm-wordpress/wp-rest/Controller/Open.php

Line 71 in 66f1bb8

$file = CIVICRM_PLUGIN_DIR . 'civicrm/i/tracker.gif';

[christianwach]

the fix maybe something adaptive - like on Civi-Drupal

@totten Perhaps you recall that we had wide-ranging discussions (perhaps here before the issue queue was closed?) of why directory traversal won't work reliably in a WordPress environment. FWIW it was the primary reason for building the WP-REST routes to replace extern/* entry points.

tl;dr Only WordPress has canonical knowledge of:

• where it's located, i.e. ABSPATH
• which site (in multisite) is being requested (could be by subdomain, path or domain mapping)

@jackrabbithanna My guess is that ease of configurability on a per site basis is likely to be the most reliable path forward - because generalising about WordPress from a plugin is not the recommended thing to do.

[kcristiano]

civicrm/civicrm-core#32945 (comment) related to above comment as well.

[totten]

generalising about WordPress from a plugin

I don't think this PR suggests doing that.

The aim is to add support for some composer layout. The composer layout may differ from existing layouts, and it appears negotiable, but it is only one, and it does not require a general comprehension of all possible WordPress layouts.

I'm not familiar with Composer file structures

Composer has some canonical file-structures. For example, if one said:

mkdir myapp
cd myapp
composer init
composer require civicrm/civicrm-{core,packages,wordpress}

Then it would download some archives (like https://github.com/civicrm/civicrm-core/archive/refs/tags/6.3.1.zip -- but for civicrm-core, civicrm-packages, and civicrm-wordpress), and extract them to corresponding folders:

myapp/vendor/civicrm/civicrm-core
myapp/vendor/civicrm/civicrm-packages
myapp/vendor/civicrm/civicrm-wordpress

composer has some options to rearrange these folders, but doing it haphazardly is asking for trouble, so people exercise restraint. Typically, an entire ecosystem might have a handful of rules for how to move packages. (Ex: D10's recommended-project).

Additionally, there's a constraint about overlapping folders -- we can move packages, but we shouldn't let them overlap. Alas, the basic/default layout for Civi-WP has three overlapping folders -- what you might call Matryoshka dependencies, e.g.

myapp/web/wp-content/plugins/civicrm                            ## Comes from civicrm-wordpress
myapp/web/wp-content/plugins/civicrm/civicrm                    ## Comes from civicrm-core
myapp/web/wp-content/plugins/civicrm/civicrm/packages           ## Comes from civicrm-packages

The Matryoshaka layout is kinda handy for zipball/tarball distribution, but it's functionally problematic for composer-based distribution. (What happens when composer upgrades or removes one subpackage -- how does it impact the others? Humans can work out solutions, but the logic is particular, and composer hasn't been specifically taught to handle such quirky questions.)

In Mark's wordpress-civicrm-composer-proto (and in the typical layout for Civi-D8/9/10/11), one breaks apart the Matryoshka layout -- using cleanly separated folders for each subpackage. This makes composer much happier.

myapp/vendor/civicrm/civicrm-core                       ## Comes from civicrm-core
myapp/vendor/civicrm/civicrm-packages                   ## Comes from civicrm-packages
myapp/web/wp-content/plugins/civicrm                    ## Comes from civicrm-wordpress

Which brings us to the current issue. If you have this layout, then you stumble upon a few hardcoded Matryoshka references in PHP. Thus the PR proposes:

-require_once CIVICRM_PLUGIN_DIR . 'civicrm/CRM/Core/ClassLoader.php';
+require_once $civicrm_root . 'CRM/Core/ClassLoader.php';

If I understand correctly, the theory is that $civicrm_root can adapt better (i.e. it should work with Matryoshka and non-Matryoshka layouts).

[christianwach]

@totten Thanks for this explanation - I appreciate it 👍

FWIW, by "generalising about WordPress from a plugin" I meant traversing directories in order to find stuff. It's not generally reliable when done from an entry point that is a plugin file, e.g. extern/*.

It's good to see that Composer has relatively predictable file structures and I'm sure that this can be accommodated for include and require - though URLs are likely to be more problematic. For example, the following all point to the same resource on my local multisite:

https://example.org/wp-content/plugins/civicrm/civicrm/bower_components/crossfilter2/crossfilter.min.js
https://example.org/subsite-one/wp-content/plugins/civicrm/civicrm/bower_components/crossfilter2/crossfilter.min.js
https://example.org/subsite-two/wp-content/plugins/civicrm/civicrm/bower_components/crossfilter2/crossfilter.min.js
https://example.net/wp-content/plugins/civicrm/civicrm/bower_components/crossfilter2/crossfilter.min.js

This is probably more relevant to the civicrm-core PR, however. My suggestion for the additional change to this PR is still relevant here.

[jackrabbithanna]

wrt css/js resources, or other static files, I don't think changes need to be made for paths. Those can be put in the correct location, for whatever directory location via the CiviCRM Asset Plugin.
That plugin already supports placing files in an arbitrary location
For example, I put in my POC composer.json

"civicrm-asset": {
            "path": "web/wp-content/plugins/contrib/civicrm/civicrm",
            "url": "/wp-content/plugins/contrib/civicrm/civicrm"
        },

I put WP plugins in a "contrib" directory, but this is optional, a choice, not a requirement.

The developer of the composer.json can set whatever paths they choose.

@christianwach
For the civicrm/i/tracker.gif file, I think the same principle can apply.
The vendor directory is usually outside of the webroot, so you don't want, or can't request them directly via url.

It is also possible to specify file extensions to copy, so perhaps .gif files are included (untested)

    // Customize default list of assets
    "assets:*": {
      "include": ["**.js", "**.css", "**.gif"],
      "exclude-dir": [".git"]
    },

See the options: https://lab.civicrm.org/dev/civicrm-asset-plugin#options

In this way it is possible for the .gif file to be in the plugin directory in the expected location.

[christianwach]

@jackrabbithanna Just wondering... perhaps this code could be removed completely? AFAICT the same code is run at the bottom of civicrm.settings.php so I'm not entirely sure why it's being run again.

[jackrabbithanna]

I was a little hesitant to remove completely, as its been there for so long. This simply changes the order it happens

[jackrabbithanna]

Any objections to this? I'm marking it as ready for review. Just a simple change of the order that would do no other use cases any harm.
What do you think @totten ?

[christianwach]

I don't have any strong exceptions, but I would prefer to see this code removed rather than relocated. Are you able to test that alternative @jackrabbithanna ? Less code is always preferable :)

[jackrabbithanna]

@christianwach .. I tested without that code. it does seem to work on both installation and normal page loads. I am hesitant to remove it, not knowing all possible cases. The purpose of this PR is just to make it composer can work, and the changes in it do make that happen. I trying it out to see how the tests run.

[kcristiano]

@jackrabbithanna I do agree that we need more testing to remove the code referenced #348 (review)

@christianwach I think moving to a separate PR would be a good way to do this?

I'll set up some time to test this and #349 . As I don't expect these changes to interfere with the 'typical' existing install methods I suspect we can merge after that.

[jackrabbithanna]

@kcristiano yeah I put the require Classloader.php, in its new spot.
To get composer based installations to work, it only needs be moved, not removed. So if we want to do this, and then in a second PR remove it .. that's safe an good.
This PR
Then in Core: civicrm/civicrm-core#32945
And #349

And composer based installation + page loads will work.