Add system check to ensure WP base page exists

[agh1]

Overview

The WordPress base page is important for rendering content in WordPress, but someone might delete the page, or the configured path might be changed so that it doesn't match the page.

This adds a system check to make sure that the page configured on the WordPress Integration settings page exists.

Before

If you delete the page at /civicrm, or if you accidentally change the setting to something different without having a page there, you'll have problems and won't know why.

After

If there is no page with the slug specified by the WordPress Base Page setting, you get one of the following ERROR-level messages:

• Base page is the default, but no page has that slug:
  

• Non-standard base page, but the default /civicrm exists:
  

• Non-standard base page, and the default /civicrm doesn't exist either:
  

If is_multisite() it will skip the whole check per comments below.

Technical Details

I added a new CRM_Utils_Check_Component_Cms that could potentially include other CMS-specific checks.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[agh1]

@kcristiano @christianwach are there any non-standard WP configs that could potentially get tripped up in this?

[agh1]

Jenkins retest this please

[kcristiano]

@christianwach what do you think about Multi-Site? That could be an issue.

@agh1 I am a bit concerned about this, we already have the problem of:

This appears on most installs, it's marked critical and all testing I've done shows it's a false positive, but I can't figure out a reliable fix for that and I just have to tell people to ignore. I'd like to avoid that here.

[seamuslee001]

Jenkins re test this please

[agh1]

@kcristiano the difference here is that it looks up via get_posts() if a page exists, only querying by the slug that's set in the CiviCRM WordPress integration settings. In other words, the test doesn't try to use anything besides the native WP function plus the civicrm or whatever that CiviCRM expects:

civicrm-core/CRM/Utils/Check/Component/Cms.php

Lines 32 to 39 in 2586fbb

	$slug = $config->wpBasePage;
	$pageArgs = [
	'name' => $slug,
	'post_type' => 'page',
	'post_status' => 'publish',
	'numberposts' => 1,
	];
	$basePage = get_posts($pageArgs);

Potentially the handy link could be wrong in some circumstances, but I can't think of when.

I do think that it would make sense to revisit that resource URL check, because it seems to trip up on the placeholders on nearly everyone, but that's outside the scope of this PR.

Separately, the test failures seem really weirdly unrelated unless I'm missing something.

[kcristiano]

@agh1 Good points and forgive my rants. I am revisiting the other issue.

I'll test this, but I do think we should restrict to single site to start. I see too many issues with WP Multi-Site. Is CiviCRM Network Activated? Is it activated just on a single site? Are we sharing Databases? I can go on, but this all affects what base page gets used and how we'd use it.

[demeritcowboy]

For the resource url see https://lab.civicrm.org/dev/core/-/issues/1647 and https://lab.civicrm.org/dev/drupal/-/issues/114 which are the same problem.

[agh1]

@kcristiano if you're accessing CiviCRM within a WP multi-site, would it make a difference how to call get_posts()? It seems that it calls the site that you're on when you run the check (whether via API or the system check page). Similarly, I think $config->wpBasePage would be specific to the CiviCRM domain you're on. If it's possible for those two to get out of whack, I think that would be a more existential problem than just this system check.

[christianwach]

I agree with @kcristiano that this check should only apply to single site instances of WordPress. There are just too many possible configurations when using WordPress Multisite and WordPress Multi-Network for this to be reliable.

[christianwach]

@agh1 Can I please please ask that there be no notice in multisite? Anyone using multisite should be able to get their luggage off the carousel. This seems like unnecessary (and, most likely, misleading) clutter.

[agh1]

Ok due to popular demand I've toned it down to a NOTICE if it's multisite (see updated screenshot in the description above) and added an invitation for anyone to let us know if the check thinks there's a problem even though everything's fine. What do you think @kcristiano @christianwach ? Again, I would be shocked if WordPress' own get_posts function failed in a multisite context, but I'm happy to avoid false alarms for more advanced environments.

[agh1]

@christianwach I was just writing my most recent comment when yours came in--are you saying please please not even a NOTICE or please please don't make it a severe notification that pops up? What I switched it to is parallel to what everyone with PHP 7.2 sees.

[christianwach]

are you saying please please not even a NOTICE

@agh1 Yes. It's not that the get_posts() check may fail, it's that it may not (and likely will not) reflect the way that multisite is set up. I run several multisite (and a couple of multi-network) instances and none of them are "standard" with a Base Page per site. Why tell me what I already know?

[kcristiano]

@agh1 The issue is we have so many possibilities:

• CiviCRM Network Activated on Mult-Site
• CiviCRM activated just on a sub site and not on the main site
• CiviCRM activated on multiple sub sites sharing a database
• CiviCRM activated per site and having unique databases
• CiviCRM installed on A WP Multi-Network
• MS with Sub directory
• MS with Sub Domains
• MS with sunrise.php to do domain mapping
  .... I can keep going.

In short WP Multi-Site is as standard as a standard CiviEvent setup. Everyone does it differently.

And yes get_posts() will look to the site you are logged into -- BUT CiviCRM may be set to only show on site id 1.

I have tested and I had a number of false positives - in my case all 'Good" when no base pages exist.

I do think that this check may be helpful - but more to us - Developers and Partners. I'd limit to Single site only and not even run on Multi-Site.

If this goes in, it's another system status I have to unhook in our repos and I really don't want to do that. I see the value in Single Site I see too many potential 'unexpected outcomes' in Multi-Site.

[agh1]

@christianwach @kcristiano sounds fine with me. I've updated it to completely skip the check for multisite.

I do worry that this system check may be the least concern in the contexts where the check would have a false positive. In other words, if you're accessing the backend of CiviCRM via site 2 and CiviCRM is only expected to work on site 1, with no base page on site 2, all of the frontend links from the backend of CiviCRM should be bad, right?

[kcristiano]

In other words, if you're accessing the backend of CiviCRM via site 2 and CiviCRM is only expected to work on site 1, with no base page on site 2, all of the frontend links from the backend of CiviCRM should be bad, right?

In my testing it will take all front end pages to the main site - at least native CiviCRM links. Any site that tries to get a native CiviCRM page on site 2 would fail and in this case it'd be correct.

I agree with @christianwach that if someone is installing MS it's on them. I am just concerned that the noise generated by system checks has passed the point where they mean anything. I know I have been adamant here, but I don't see the value on this check on MS.

I am still trying to work through civicrm/civicrm-wordpress#205 But the issue here is that the way I set up MS it failed. If I set it up a different way it works - yet all it should do is create a page on the blog_id I am logged into with CiviCRM - yet my standard config was different.

Once we get past that PR(and I think we will but we must update the docs as to the install procedure as well) then could we add a MS check? _ Maybe_

[agh1]

Your last comment @kcristiano got me wondering how CRM_Utils_System_WordPress::url() handles things for frontend links, and it turns out that it builds on the [wp.frontend] placeholder

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

Line 398 in 1611013

return Civi::paths()->getUrl('[wp.frontend]/.', $absolute ? 'absolute' : 'relative');

which is populated by looking up the page on the current site corresponding to the $config->wpBasePage:

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

Lines 77 to 83 in 1611013

	Civi::paths()->register('wp.frontend', function () {
	$config = CRM_Core_Config::singleton();
	$basepage = get_page_by_path($config->wpBasePage);
	return [
	'url' => get_permalink($basepage->ID),
	];
	});

If nothing else, it's better to use a method for the check that's consistent with the method for building the URLs, so I changed how to do the lookup here. It also lets me provide better information in the situation of someone unpublishing the page.

[eileenmcnaughton]

I've added OK-without-test as we don't have a way to test wp-specific stuff. @kcristiano @christianwach I assume one of you will merge when you are happy with this

[christianwach]

@eileenmcnaughton @agh1 We will meet to discuss this and take action tomorrow.

[eileenmcnaughton]

Thanks @christianwach - as always you & @kcristiano are on top of things!

[christianwach]

Here are a few points that emerged from our discussion:

1. Removing checks from multisite/multi-network simplifies matters substantially. However...
2. What happens in situations where CiviCRM is used without any front-end forms, webhooks or IPN return URLs? Perhaps people are just using it as an address book, for example? These installs would not need a Base Page and the warnings here would be redundant.
3. The wording of the warnings isn't comprehensive: for example, if a plugin implements this hook then (for that install) the "default" slug will not be civicrm, so the hard-coded wording here will be misleading at best.
4. The appearance of warnings and notices already gives the impression that CiviCRM is broken software. Adding yet another check would not alleviate this problem.
5. Fresh installs of CiviCRM should never cause any of these warning to be shown... so why have it?

tl;dr We're still ambivalent about the value of this check.

[agh1]

@christianwach I can see how point 3 might be relevant and possible to implement. In principle it seems like an edge case, but it would certainly be easy to add that one line letting plugins modify the value before looking it up.

However, points 2, 4, and 5 seem to argue against offering a system status page at all. We check for all kinds of things that don't affect some users. Refusing to add a check that is sensitive and specific won't stop the system from yelling at people that their database is unprepared for the chance that some future version of CiviCRM will allow emoji.

Point 5 gets to the real purpose of this check and the real problem with a lot of checks. Most system checks should not show on a fresh install of CiviCRM. If CiviCRM can find a problem, it should be able to prevent it or at least tell you during install.

This system check is for the specific, shockingly common situation of someone deciding to clean out their site and delete the "CiviCRM" base page. (I know the page itself says not to delete it, but I suppose people think it's just programmed to say that as a fail-safe measure.) It's also helpful if someone moves CiviCRM from one site to another and the destination either lacks the page or doesn't have the custom base page that the source site did.

Unlike checks that warn about theoretical future problems or newly-introduced requirements, this watches out for something that actually breaks sites when it happens and has been an issue ever since the base page feature was introduced.

Incidentally, if you're interested in ways to avoid "the impression that CiviCRM is broken software", I think @seamuslee001 and @eileenmcnaughton were at least discussing a way to display system checks for a more specific permission than Adminster CiviCRM.

[agh1]

Actually, I take that back about point 3. The civicrm_basepage_slug filter is only used in creating the base page. Once it's created, the path is set in the wpBasePage setting:
https://github.com/civicrm/civicrm-wordpress/blob/77159b696307275f7850f1819e2ef8d5c6c6a734/includes/civicrm.basepage.php#L221-L227

Since this check looks at that setting, the situation of a plugin modifying the default base page slug is already handled. I can see arguments both ways as to whether the "default" is CiviCRM's default or whatever the plugins have modified it to be.

[kcristiano]

@agh1 I just did a test where I changed the base page on install using an mu-plugin

function tc_change_slug( ) {
        $tc_slug = 'crmbasepage';
    return $tc_slug;
}
add_filter( 'civicrm_basepage_slug', 'tc_change_slug' );

This does change the base page on install and store it in civicrm_settings. @christianwach is there another use of the filter I am missing that should be looked at?

Unlike checks that warn about theoretical future problems or newly-introduced requirements, this watches out for something that actually breaks sites when it happens and has been an issue ever since the base page feature was introduced.

I do get this. I've had times where odd things broke (Mosaico) due to lack of a base page. At the very least I think the severity needs to be dampened down and we need to improve the worjding for users.

Let me add this to my 5.28 RC testing sites and see if I encounter any issues.

[christianwach]

@agh1 I just did a test where I changed the base page on install using an mu-plugin

function tc_change_slug( ) {
        $tc_slug = 'crmbasepage';
    return $tc_slug;
}
add_filter( 'civicrm_basepage_slug', 'tc_change_slug' );

This does change the base page on install and store it in civicrm_settings. @christianwach is there another use of the filter I am missing that should be looked at?

@kcristiano Nope, that's the one. The result was presumably that the check reported the wrong value for the default base page slug on this install (i.e. civicrm instead of crmbasepage) because the check doesn't implement the filter.

[kcristiano]

Thanks @agh1 I see the latest changes should clean up language. Do we want to discuss the level? I really don't think it's a critical error. I will acknowledge that this could be mitigated if #16482 was adopted.

How would you feel about a Notice instead of critical?

[agh1]

That makes sense that in an environment where the default base page has been modified via civicrm_basepage_slug the check should suggest that instead of civicrm. I've just made a commit that does just that. It gives precedence for the modified default if it's present or if neither that nor civicrm is present. However, it does suggest civicrm if that is present and the modified default is not (which might happen if you enabled a plugin belatedly and then modified the CiviCRM settings to something that doesn't exist).

[agh1]

Sorry I'm getting backed up on comments--I was addressing the language suggestions. I'll respond to levels @kcristiano in just a sec.

[agh1]

@kcristiano in determining levels for the system check we've been trying to stick to how they're described in PSR-3: https://www.php-fig.org/psr/psr-3/#3-psrlogloggerinterface

Level	Description	Examples
emergency	System is unusable.
alert	Action must be taken immediately.	Entire website down, database unavailable, etc. This should trigger the SMS alerts and wake you up.
critical	Critical conditions.	Application component unavailable, unexpected exception.
error	Runtime errors that do not require immediate action but should typically be logged and monitored.
warning	Exceptional occurrences that are not errors.	Use of deprecated APIs, poor use of an API, undesirable things that are not necessarily wrong.
notice	Normal but significant events.
info	Interesting events.	User logs in, SQL logs.
debug	Detailed debug information.

By this measure, the basepage being missing is certainly more severe than "normal but significant events", and probably more severe than "undesirable things that are not necessarily wrong". It's short of "action must be taken immediately", and while this will make an "application component unavailable"--namely all contribution pages, events, Mosaico, and more--I wanted to lean against alarmism, so I chose "error".

Moving away from the framework to more practical considerations, a "notice" will not appear to the admin like anything is wrong. To me, all of these warnings about upcoming PHP versions and UTF8MB4 should be classic notices: nothing is wrong right now, but you ought to be aware. In contrast, a missing basepage is going to cause serious problems for most people, and more specifically, those problems won't be easy for someone to trace back to the missing page.

There's no way to promise that all messages will be applicable to everyone, but that is why you can hush the message either through the UI or the API. My feeling is that someone who has gone out of their way to delete the base page or modify the setting to a page that doesn't exist is someone who can go out of their way to click the button to hide the message. In contrast, if someone less sophisticated has a standard site and sees that message, it's almost always going to be caused by accident, and it will probably point to a real problem they'll face.

Finally, I 100% agree that the system messages are too noisy. Admins see them and don't know what to do about them. There are loud ones about theoretical future problems, not issues to be addressed now. However, by that measure this check will do practically nothing to make them noisier.

[kcristiano]

but that is why you can hush the message either through the UI or the API

User/Site Admins just don't do that

I don't believe this is a critical error (I think ERROR and above as critical). Perhaps warning at best.

I know I am being rigid on this. I just don't see this rising to a that severity.

[agh1]

@kcristiano has already found his way there, but for everyone else's sake I've opened an issue dev/core#1863 to discuss severities of system checks in general. I do believe that this message should be an "error" in the rare occasions when it would appear--it represents a real, present problem for the majority of sites--but I also think that a lot of messages that appear far more often should be ratcheted down.

[kcristiano]

-it represents a real, present problem for the majority of sites

Can you detail how this will rise to a Red Level Alert -Emergency Action Needed? That's how it appears to a user. I understand PSR-3, but Zero of my clients do and expect they are a pretty good measure of users. I am not comfortable with this check at an the "Error" level.

[agh1]

Yeah, when an organization's donation page won't work, that's a 🚨 Red Alert 🚨 for pretty much anyone. The same goes for event registrations and Mosaico for organizations who use them.

The only reason I bring up PSR-3 is that I totally understand there could be a variety of opinions on severities, but that's the standard CiviCRM uses. We have a problem that different system check writers have different ideas as to what's severe, so I'm trying to refer back to the standard so we have some level of parity among them.

Put another way, I'd say the base page going missing is at least as severe as a case type referring to a relationship type that doesn't exist, not being able to write to the upload directory, or cron having stopped for over 24 hours. Those are fairly typical of the "error" level for other system checks.

[kcristiano]

Ok you make a very valid point.

This looks merge ready to me

[kcristiano]

I have done r-run on this with the base page filtered and with the default. All testing shows it works as intended.

[christianwach]

Looks good to me now too.

[eileenmcnaughton]

I'm merging this - I understand @kcristiano giving it merge-ready as 'looks OK to me but what does @christianwach think' - we have that answer now so merging