CRM-21277: Suppress warnings while finding install directory.

[agileware-fj]

Prevents log spam when e.g. open_basedir restrictions are in effect.

---

• CRM-21277: CRM_Utils_System_WordPress::validInstallDir spams log with warnings when open_basedir restriction is in effect
  https://issues.civicrm.org/jira/browse/CRM-21277

Overview

Suppress warnings while finding install directory.

Before

Error log can be spammed during Wordpress CiviCRM bootstrap by PHP warnings about non-fatal reasons the directories can't be opened

After

These warning are now suppresed by the @ operator

[civicrm-builder]

Can one of the admins verify this patch?

[eileenmcnaughton]

@totten what do you think of this? Other CMS are not warning-quashed when calling cmsRootPath .... so it feels a bit inconsistent ...

[agileware-fj]

Are other systems hunting all the way up like this? If so, I'd argue that the warning should probably be quashed on those CMSen also as this is a valid mode of failure for this strategy.

We just don't restrict open_basedir on our own hosting system so we've only seen these issues on a foreign WordPress site.

[agileware-fj]

Looking at it, the inconsistency is the direction searched (e.g. Drupal does bottom to top, Wordpress is doing top to bottom):

civicrm-core/CRM/Utils/System/Drupal.php

Lines 615 to 625 in 163494a

	do {
	$cmsRoot = $firstVar . '/' . implode('/', $pathVars);
	$cmsIncludePath = "$cmsRoot/includes";
	// Stop if we find bootstrap.
	if (file_exists("$cmsIncludePath/bootstrap.inc")) {
	$valid = TRUE;
	break;
	}
	//remove one directory level.
	array_pop($pathVars);
	} while (count($pathVars));

vs

civicrm-core/CRM/Utils/System/WordPress.php

Lines 502 to 509 in 163494a

	foreach ($pathVars as $var) {
	$cmsRoot .= "/$var";
	if ($this->validInstallDir($cmsRoot)) {
	//stop as we found bootstrap.
	$valid = TRUE;
	break;
	}
	}

[eileenmcnaughton]

OK that makes sense as a point of difference. @kcristiano do you have an opinion on this?

[colemanw]

@civicrm-builder add to whitelist

[highfalutin]

Re: the difference in cmsRootPath() between Drupal and WP -- Drupal searches bottom-up and WP searches top-down -- this has caused problems for me before and I've thought it would be better to make the WP version bottom-up as well.

The problem I've found with the WP top-down approach is that it gets the wrong result when I have a WordPress Civi site installed inside the root directory of another WordPress site:

WP root directory	cmsRootPath() result
/path/to/wp1	/path/to/wp1
/path/to/wp1/path/to/wp2	/path/to/wp1 -- incorrect!

(It's not my choice to have this directory structure but sometimes my clients have set things up this way.)

Making the WP version consistent with the Drupal version (bottom-up) would solve this problem. And if I'm reading the discussion on this PR correctly, it would also solve the problem that this PR is meant to address.

I'd be happy to submit a separate issue/PR that makes the WP version of cmsRootPath() consistent with the Drupal version. Thoughts?

[kcristiano]

@agileware-fj Thanks for working on this. The open_basedir has been an issue for a long time. @highfalutin I like consistency so matching the Drupal Logic makes sense to me, but whether we go top down or bottom up I have seen way too many "nested WP installs" . In that case if we start at the bottom we can still find the wrong install.

@christianwach I know you've looked at this as well many times - do you think we should look at a DB setting to bypass the searching? We still have the issue when there is an external call such as an ipn.

[christianwach]

This directory traversal code has been unnecessary for about 2.5 years now. There is already a setting for the path to 'wp-load.php' from which ABSPATH can be derived - just strip off 'wp-load.php' :-) See the add_wpload_setting method in class CiviCRM_For_WordPress for full details.

[highfalutin]

Thanks @christianwach for noting that. Are you suggesting that the directory traversal code be replaced entirely?

In the meantime, @kcristiano, I agree that nested WP installs are a headache. But try as I might, I can't think of any case where a bottom-up traversal would lead to an incorrect result. Could you give an example?

[christianwach]

Are you suggesting that the directory traversal code be replaced entirely?

Yes, absolutely. There is no reliable way of discovering the whereabouts of a WordPress install based on the location of a plugin or theme file. I have been trying to get this across for years now.

I can't think of any case where a bottom-up traversal would lead to an incorrect result

Please look at "Editing wp-config.php" for the almost endless ways in which the filesystem can be structured - and moving the 'wp-content' folder in particular. This is not only possible but quite common and is precisely what has been causing @kcristiano so many headaches.

My preference in this messy (and ultimately unresolvable) situation would be to route everything through WordPress - CiviCRM is the plugin, after all. And as @totten pointed out, it is only "extern/open.php" and "extern/url.php" which do not call loadBootstrap() (though IPN returns may also be included in this as @kcristiano suggests) so the impact of doing so would be pretty minimal.

[kcristiano]

@christianwach I agree 100% - In reviewing further, we did patch the PayPal ipn code a while back to boot wp - so we are "only" talking about 'extern/open.php' and 'extern/url.php' that do not call loadBootstrap()

Full history of the WP in its own directory issues are here: https://wiki.civicrm.org/confluence/display/CRM/WordPress+installed+in+its+own+directory+issues

[christianwach]

@highfalutin I should have added "Giving WordPress Its Own Directory" to the links I provided. In combination with what I wrote in my last comment, I hope you can see the issue being faced.

[eileenmcnaughton]

Should we be tackling getting rid of 'extern/open.php' and 'extern/url.php' in favour of a routed path as a pre-requisite to this?

[christianwach]

Should we be tackling getting rid of 'extern/open.php' and 'extern/url.php'

In my view, yes. Caveat: I'm not 100% sure those are the only direct calls to CiviCRM - @eileenmcnaughton can you think of any others?

It's also important to create those routes because many WordPress installs have been "hardened" by adding an '.htaccess' file inside 'wp-content' which disallows the execution of PHP within that directory. The (quite reasonable) assumption is that anything inside 'wp-content' is content for WordPress - although I concede that it's a loose definition of "content" in that it includes themes, plugins and language files in addition to uploads.

[eileenmcnaughton]

I guess anything in the extern folder.... the ipn files might still need to 'age out' in that 'already initiated' recurring ipns might still be hitting them

[christianwach]

I guess some of the other services might need the same - in case people click links in old emails, for example.

[eileenmcnaughton]

Yep - although they can't start aging out until there is an alternative path

[highfalutin]

thanks @christianwach, I was unfamiliar with the flexibility in WP directory structure.

[agileware-fj]

So.... While I appreciate that getting rid of legacy code is a good idea, I think it's a little out of scope for this PR; Can we determine if we're merging this and possibly talk about the Wordpress-installation-finding issue somewhere with better visibility? (I'm okay with my issue being hijacked for it, even)

[kcristiano]

@agileware-fj Sorry for the hijacking - looking at the patch what we want to do here is suppress the errors for open_basedir. While I'd like to fix the underlying issue, I do understand the desire to clean up the erro log of nonsensical entries.

Let me see if I can get a setup that creates the error messages and I'll test.

[christianwach]

I'm okay with my issue being hijacked

Yeah, sorry about that. Useful to have had this discussion though - thanks for "hosting"!

[jusfreeman]

This directory traversal code has been unnecessary for about 2.5 years now

@christianwach is there a PR or JIRA issue around to change this behaviour in CiviCRM? If not, would you mind creating one. Open to discussing sponsoring funding this development too - reach out on MM.

[christianwach]

is there a PR or JIRA issue around to change this behaviour in CiviCRM?

@jusfreeman I don't think so. Let me think about the scope of this before opening an issue - it has an impact on many aspects of CiviCRM. Agreed - let's take further discussion to MM.

[jusfreeman]

Is this PR still valid and required? If not, can be closed.

[kcristiano]

@jusfreeman I do think CRM-21277 is valid. The question is to further patch directory scanning as this PR does or tackle removing directory scanning.

While I would advocate removing directory scanning that will take a much greater effort. The question is do we want to add another patch to suppress the warnings for now. I am not seeing the downside to this. @christianwach what do you think?

[christianwach]

I see no harm in it as such - just that it inevitably punts a proper solution down the road.

[eileenmcnaughton]

This only affects Wordpress & our resident WP gurus have given this a soft +1.

@totten @colemanw @mlutfy if none of you have a concern I think I'm going to go with better-in-than-out on this (next time I happen to look at it)

[eileenmcnaughton]

merging per previous comment