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

WP - Change definitions of cms.root, civicrm.root #17360

Merged
merged 2 commits into from
Jun 7, 2020

Conversation

totten
Copy link
Member

@totten totten commented May 19, 2020

Overview

This is an alternative to #17353.

Before

The logic for computing path/URL defaults is in Civi/Core/Paths.php.

On WP, normal Civi pages and standalone extern scripts use the same logic for computing paths/URLs.

After

The base logic for computing path/URL defaults is in Civi/Core/Paths.php.

For Civi-WP, certain variables (which are easier to compute via WP APIs) are overridden in CRM/Utils/System/WordPress.php.

On WP, the path/URL logic for has been split:

  • One codepath for normal Civi pages - based on WP APIs
  • Another codepath for older, standalone extern scripts - based on the same logic as before

Technical Details

Some key differentiators vs #17353:

  • Uses the existing Civi::paths()->register() interface
  • Doesn't add a long list of getFooStorage() functions
  • If you use any normal page or any newer extern-like end-points, then it uses the prettier callpath.
  • If you have external references to the standalone extern/* scripts, and if those scripts are currently working, then they should continue working in the same way as before.
  • Moves the wp.frontend, wp.backend, etc variables (which are really WP-specific) out from Civi/Core/Paths.php to CRM/Utils/System/WordPress.php.

@civibot
Copy link

civibot bot commented May 19, 2020

(Standard links)

@mattwire
Copy link
Contributor

Ping @christianwach

@christianwach
Copy link
Member

Thanks for this @totten - it looks like this is functionally identical to #17353 with the bonus that the legacy routes (which I ignored) are still accommodated 🥇

Do later declarations of Civi::paths()->register('foo') replace existing callbacks or add to them? (I assume the former but would appreciate confirmation)

@totten
Copy link
Member Author

totten commented May 19, 2020

Later declarations replace earlier ones ✅

public function register($name, $factory) {
$this->variableFactory[$name] = $factory;
return $this;
}

There's a small element of luck in how this patch worked out -- given that register() can replace the callback, one still has to re-register() at the right moment (i.e. after Civi\Core\Paths is instantiated but before anyone tries to read the given path/url/var). It worked out that the CRM_Utils_System_*::initialize() is demonstrably opportune:

$bootServices['paths'] = new \Civi\Core\Paths();
$class = $runtime->userFrameworkClass;
$bootServices['userSystem'] = $userSystem = new $class();
$userSystem->initialize();

Firing a hook in getVariable() would also be fairly opportune (at least... it works out in my mental model...), but admittedly there are caveats/risks with hooks during bootstrap, and this is a smaller patch.

@christianwach
Copy link
Member

@totten Thanks for the clarification - it's all clear to me now. I like this approach and it would certainly be a major step forward from where we currently are. I'll close #17353 in favour of this. Happily so :-)

@mattwire
Copy link
Contributor

mattwire commented Jun 6, 2020

@christianwach Have you specifically tested this PR and are you happy that it should be merged?

@kcristiano
Copy link
Member

I'd like to see this in 5.27 if I had my way, but lost track of it.

I have done r-run locally and have tested basic functionality and it looks good. I'd like it in 5.27 so we can get a full RC cycle. Then I can test all extern points which have always been the most problematic.

@totten any thoughts on having this in 5.27 as the RC just dropped?

@seamuslee001 seamuslee001 changed the base branch from master to 5.27 June 6, 2020 22:52
@civibot civibot bot added 5.27 and removed master labels Jun 6, 2020
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001
Copy link
Contributor

Test fails unrelated

@seamuslee001 seamuslee001 merged commit 8889004 into civicrm:5.27 Jun 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants