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

dev/joomla#26 - Fix path derivation when CMS is rooted in a subdir #16887

Merged
merged 2 commits into from
Mar 24, 2020

Conversation

totten
Copy link
Member

@totten totten commented Mar 24, 2020

Overview

CiviCRM is deployed inside a CMS. The CMS is usually deployed at the HTTP root (http://example.org), but it is sometimes deployed in a subdirectory (http://example.org/my-cms).

Some asset URLs are computed using the variables [civicrm.bower], [civicrm.packages], and [civicrm.vendor], which are derived from the value of [civicrm.root]. However, if the site is deployed in a subdirectory, then the computation of [civicrm.bower] (etc) can misbehave.

(The problem has been reported in 5.23. It may or may not have misbehaved in prior versions, but it was more obscure/less important in prior versions - in 5.23, it matters more.)

See: https://lab.civicrm.org/dev/joomla/issues/26#note_33465

Before

When the URL for [civicrm.bower] (or similar) is derived, and if the environment uses HTTP on the default port (80/443), then the URL goes through multiple filters - first, from absolute to relative, and then later from relative back to absolute. In the process, the base may be inadvertently changed.

In dev/joomla#26, this was observed to produce redundant URL elements.

After

When the URL for [civicrm.bower] (or similar) is derived, it is computed in absolute format - and simply kept that way.

Comments

Regarding test coverage, there are two relevant unit-tests. This PR only updates one.

  • E2E\Core\PathUrlTest: This is a more concrete smoke test which demonstrates functional problems with variables like [civicrm.bower]. It should already catch problems like dev/joomla#26... but only if you run the E2E suite on a site where the CMS was deployed to a subdirectory. Currently, the list of build-types available in CI doesn't have such a site.
  • Civi\Core\PathsTest: This is a more abstract, headless test to examine edge-cases in Civi\Core\Paths. It does not specifically check for [civicrm.bower] or similar variables (b/c the URL routing is ill-defined in a headless context). However, I've updated it to compare/contrast the working and non-working ways to derive variables.

My local dev environment doesn't support low-number ports (eg port 80), so I cannot reproduce or validate this issue precisely. It's particularly important to do some r-run during PR review.

Overview
--------

CiviCRM is deployed inside a CMS. The CMS is usually deployed at the HTTP root (`http://example.org`),
but it is sometimes deployed in a subdirectory (`http://example.org/my-cms`).

Some asset URLs are computed using the variables `[civicrm.bower]`, `[civicrm.packages]`, and `[civicrm.vendor]`, which
are derived from the value of `[civicrm.root]`.  However, if the site is deployed in a subdirectory, and if using v5.23,
then the computation of `[civicrm.bower]` (etc) can misbehave.

Before
------

When the URL for `[civicrm.bower]` (or similar) is derived, it goes through multiple filters - first, from absolute to
relative, and then later from relative back to absolute.  In the process, the base is inadvertently changed.

After
-----

When the URL is derived, it is computed in absolute format - and simply kept that way.

Comments
--------

Regarding test coverage, there are two relevant unit-tests. This PR only updates one.

* `E2E\Core\PathUrlTest`: This is a more concrete smoke test which demonstrates functional problems with variables like
  `[civicrm.bower]`.  It should already catch problems like dev/joomla#26...  but only if you run the E2E suite on a system
  where the CMS was deployed to a subdirectory.  `civibuild` doesn't currently include such a build-type.
* `Civi\Core\PathsTest`: This is a more abstract, headless test to examine edge-cases in `Civi\Core\Paths`. It does not
  specifically check for `[civicrm.bower]` or similar variables (b/c the URL routing is ill-defined in a headless context).
  However, I've updated it to compare/contrast the working and non-working ways to derive variables.
@civibot
Copy link

civibot bot commented Mar 24, 2020

(Standard links)

@civibot civibot bot added the 5.24 label Mar 24, 2020
@andrewpthompson
Copy link
Contributor

I've tested the PR (but not PathsTest.php) on Joomla on a site at top-level and in a subdirectory. No problems seen.
cli.php also works still.

As a bonus I think also this longstanding issue https://lab.civicrm.org/dev/joomla/-/issues/5 is resolved between this PR and the recent work elsewhere (maybe #16761 and/or 1143a78), which is a little bit exciting!

@kcristiano
Copy link
Member

I've also given this an r-run on a WP Multi-site subdirectory installl and it solves the issue.

r-run on standard WP install works properly

I also added to my test site that has:

  • wp in it's won directory
  • wp-content moved

I did an r-run and it succeeded.

@totten
Copy link
Member Author

totten commented Mar 24, 2020

Great! Thanks @andrewpthompson @kcristiano.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001 seamuslee001 merged commit b82673c into civicrm:5.24 Mar 24, 2020
@totten totten deleted the 5.24-dbl-path branch March 25, 2020 00:12
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.

4 participants