Add system check for clean URL configuration #24477

elisseck · 2022-09-07T18:48:18Z

Overview

https://lab.civicrm.org/dev/core/-/issues/3839

Before

Clean URL settings are often not updated by unsuspecting admins as older sites are upgraded to versions of CiviCRM where they should be required.

After

System check warning appears for Wordpress users if civicrm.settings.php does not include up-to-date clean URL settings.

Technical Details

[WIP] because I am having trouble with both $civicrm_settings['CIVICRM_CLEANURL'] not updating properly each time in my testing when swapping between define('CIVICRM_CLEANURL', 0); and define('CIVICRM_CLEANURL', 1);... and because I cannot manage to cURL or GuzzleHttpClient() without breaking the system status page, and i'm really not quite sure why yet... but I need to stop here and spend some time on other things today.

Any thoughts welcome!

civibot · 2022-09-07T18:48:21Z

(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).

demeritcowboy · 2022-09-07T19:11:50Z

CRM/Utils/Check/Component/Cleanurls.php

+  public static function checkWpCleanurls() {
+    $config = CRM_Core_Config::singleton();
+    //Right now this config issue largely affects Wordpress only. This check could be applied to other CMS' but for now it's just WP
+    if ($config->userFramework != 'WordPress') {


The test fail is a style issue where it doesn't like the indentation on line 96, but regarding cms-specific, a preferred way would be:

Put all of the code below in this function here instead into CRM_Utils_System_Wordpress in a function called checkCleanurls()

In CRM_Utils_System_Base, make a function checkCleanurls() that just returns []

And then here this becomes a one-line function which will automatically do the right thing for all cmses, and is easy to update independently for other cmses by making overrides in their own CRM_Utils_System_XXX.

public static function checkCleanurls() { return CRM_Core_Config::singleton()->userSystem->checkCleanurls(); }

Thanks! I just followed CRM_Utils_Check_Component_Cms but this makes sense as a way to organize it.

Oh that one could probably be cleaned up too someday (grin).

demeritcowboy · 2022-09-07T19:17:34Z

CRM/Utils/Check/Component/Cleanurls.php

+  private static function checkCleanPage($slug, $id, $config) {
+    $client = new GuzzleHttp\Client();
+    $res = $client->head($config->userFrameworkBaseURL . $config->wpBasePage . $slug . $id);
+    $httpCode = $res->getStatusCode();


There might be some situations where guzzle throws an exception. Can wrap this in a try/catch just so that system checks don't fail in that case.

Thanks! Will do if this gets used. Not even sure if it's workable yet. Currently any implementation of guzzle here, or cURL, appears to break the whole system status page with no logs of a sort I can find. Not sure if that's because it's an angular page and I don't know angular or what yet :)

Ah it needs a backslash in front of GuzzleHttp, or add a use statement at the top of the file and then just put Client here.

Okay cool i'll try that when I get back to this later. I thought it was set properly because I can see it hitting the url in my access log but the system page then fails to load so obviously something was wrong.

elisseck · 2022-09-19T17:37:05Z

@demeritcowboy thanks for all the feedback! I believe this is working now and has integrated all of your feedback and been rebased/squashed so I am removing the [WIP].

demeritcowboy · 2022-09-20T02:06:13Z

Thanks. Overall it looks ok to me but some missing ts's, and some other comments.

General standards
- [r-explain] PASS
- [r-user] PASS
- [r-doc] Maybe could update
  - At the very bottom of https://docs.civicrm.org/sysadmin/en/latest/integration/wordpress/clean-urls/ it says "Old style URLs will still work so you won't break any existing links to your website", but I guess something has changed and that's why this status check is now needed? If so the docs could be updated.
- [r-run] PASS with comment
  - I can physically go through the motions but I don't know wordpress, so all I can do is say yes the message appears if I comment out the part in civicrm.settings.php that sets CIVICRM_CLEANURL for wordpress.
Developer standards
- [r-tech] Issue
  - Missing ts() in several places. Text that end-users are likely to see should have ts(), but e.g. the call to Civi::log() is correct that it shouldn't be ts'd.
    - Note for line 1556 you would do it like says for hyperlinks in large blocks at https://docs.civicrm.org/dev/en/latest/translation/#avoid-tags-inside-strings, but there's also an extra complication because the docs url in that text is hardcoded to the en link, so:
      $warning = ts('Clean URLs...and review <a %1>the documentation</a> for more information.', [1 => 'href="' . CRM_Utils_System::docURL2('sysadmin/integration/wordpress/clean-urls/', TRUE) . '"']);`
    - As it turns out the sysadmin pages have no non-english translations, and docURL2 always returns en at the moment anyway! But I think that's the best-practice way.
- [r-code] Minor
  - Line 1583 seems like it would be the wrong message?
  - Is there ever a situation where CIVICRM_CLEANURL would not get defined at all, like in a very old civicrm.settings.php? This line would fail in php 8+, but maybe that's ok if it's expected/required for it to be defined, just because this is an ajax page, all the user would see is a blank page.
  - When all the code was in one file it maybe made sense for it to be in its own check file, but it feels weird now. The check function could just be added to Env.php.
  - Similarly at the time having Wp in the function name maybe made sense, but now it could just be called checkCleanurls.
- [r-maint] PASS
- [r-test] PASS

elisseck · 2022-09-20T13:06:26Z

Re: [r-doc] This is technically still true, the old URLs will work... but nowadays you'll start running into session issues i.e. "cannot find valid value for id" which is confusing and frustrating to end-users trying to pay for an event etc. An admin would also need to remember to update any hard-coded URLs on the front-end of their site, which is perhaps what could be added to the docs but might be out of scope for that page.

Re: [r-tech] Thanks for the comments. Never heard of docurl2 before that's quite useful although the name is quite confusing and I keep getting it mixed up xD. Added some ts().

Re: [r-code]

removed "Wp" from function name
"1583 messaging": I'm not sure what else to say here. "Refreshed" word could be "Flushed" which would be more in line with Wordpress nomenclature, but I don't find it tends to make as much sense to people who aren't used to cli cache commands. Suggestions welcome but this makes sense to me.
"CIVICRM_CLEANURL not defined": Added a defined() check for this although agreed not particularly worried about it in the real world.
"Env.php": Generally see what you mean, the change I made was to move this to "Cms.php" which contains another application-level config check. Since this isn't really a server-level environment check I don't think it makes sense for Env.php.

demeritcowboy · 2022-09-20T13:17:34Z

Thanks for the explanations.
Regarding 1583 messaging what I meant was that in the case of a guzzle exception, it doesn't seem like it would have anything to do with the wordpress permalinks cache? It would be something more like "network error".

elisseck · 2022-09-20T13:41:06Z

The catch there is just if Guzzle fails for whatever reason we "couldn't load a page". Perhaps it should be more explicit about what's going on.

I think the second check there under if ($httpCode == 404) { is correct. This is covering the case where CleanUrls ARE configured correctly in CiviCRM, but they're not being reflected in Wordpress and users are still going to run into trouble with civiwp links (i.e. we get a 404 from what should be a valid clean url from a contribution or event page). This happens when the permalinks cache hasn't been flushed recently. AFAIK this cache pretty much doesn't flush unless you do it manually via wp-cli or by re-saving those settings so this situation is more common than it might seem.

Edit: Sorry - I see where the "refresh cache" text is duplicated now and I am making a change.

demeritcowboy · 2022-09-20T14:43:06Z

Ok I think we're almost there. Sorry I'm being so picky but it's What I Do(™).

Extra space on line 1559, after "Clean URLs".
I simulated the 404 by just hardcoding httpCode to 404 and then it gave the right message but when I clicked the action button it took me to http://example.org/wp-admin/admin.php?page=CiviCRM&q=civicrmhttp%3A%2F%2Fexample.org%2Fwp-admin%2Foptions-permalink.php. That could be my setup but things seem to be working in general. If it's working for you then let's ignore it.
Trailing commas in function args doesn't work in php 7.2 which technically civi still supports. Lines 1601 and 1608.

elisseck · 2022-09-20T16:51:23Z

@demeritcowboy no worries at all! It's good to get me back into the swing of things, making a lot of small mistakes.

I actually added more whitespace so it's consistent since this is before a return statement. I try to add whitespace before a return statement because I find it readable and I don't think it's against civicrm or drupal coding standards but maybe it's an implicit rule. I'm happy to keep them all or remove them all but now at least it's consistent here for a change.
Thank you for checking the button. I swapped the button for a link. Turns out system check action buttons use CRM.url and are not compatible with non-civicrm routes.
Removed these commas. I was getting some trailing comma style errors from jenkins earlier but that must have been something different so i'm a bit confused by that but this seems to have passed :)

kcristiano · 2022-09-20T20:25:04Z

I've also given this an r-run and it works as expected.

I do think we should update the docs and at least say that as of 5.55 we strongly recommend that CleanURLs be enabled as we have seen session errors when they are not enabled. I am struggling with how to word this, but I do think we need to clarify and push people to Clean URLs.

CRM/Utils/System/WordPress.php

elisseck · 2022-09-20T22:35:44Z

If you're feeling good at this point I can squash this down again. Thanks for all the review time!

Move per demerit's feedback Add action button, link to WordPress Permalinks page. resolve style warnings

civibot bot added the master label Sep 7, 2022

demeritcowboy reviewed Sep 7, 2022

View reviewed changes

elisseck force-pushed the issues/3839 branch from 04ecc66 to 5fddb9a Compare September 19, 2022 17:34

elisseck changed the title ~~[WIP] Add system check for clean URL configuration~~ Add system check for clean URL configuration Sep 19, 2022

elisseck force-pushed the issues/3839 branch from 5fddb9a to d04cf40 Compare September 19, 2022 18:26

demeritcowboy reviewed Sep 20, 2022

View reviewed changes

CRM/Utils/System/WordPress.php Outdated Show resolved Hide resolved

Add system check for clean URL configuration

cbcea42

Move per demerit's feedback Add action button, link to WordPress Permalinks page. resolve style warnings

elisseck force-pushed the issues/3839 branch from 70290f8 to cbcea42 Compare September 20, 2022 22:38

demeritcowboy added the merge on pass label Sep 20, 2022

demeritcowboy merged commit b082b2b into civicrm:master Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add system check for clean URL configuration #24477

Add system check for clean URL configuration #24477

elisseck commented Sep 7, 2022

civibot bot commented Sep 7, 2022

demeritcowboy Sep 7, 2022

elisseck Sep 7, 2022

demeritcowboy Sep 7, 2022

demeritcowboy Sep 7, 2022

elisseck Sep 7, 2022

demeritcowboy Sep 7, 2022

elisseck Sep 7, 2022

elisseck commented Sep 19, 2022

demeritcowboy commented Sep 20, 2022

elisseck commented Sep 20, 2022

demeritcowboy commented Sep 20, 2022

elisseck commented Sep 20, 2022 •

edited

Loading

demeritcowboy commented Sep 20, 2022

elisseck commented Sep 20, 2022

kcristiano commented Sep 20, 2022

elisseck commented Sep 20, 2022

Add system check for clean URL configuration #24477

Add system check for clean URL configuration #24477

Conversation

elisseck commented Sep 7, 2022

Overview

Before

After

Technical Details

civibot bot commented Sep 7, 2022

demeritcowboy Sep 7, 2022

Choose a reason for hiding this comment

elisseck Sep 7, 2022

Choose a reason for hiding this comment

demeritcowboy Sep 7, 2022

Choose a reason for hiding this comment

demeritcowboy Sep 7, 2022

Choose a reason for hiding this comment

elisseck Sep 7, 2022

Choose a reason for hiding this comment

demeritcowboy Sep 7, 2022

Choose a reason for hiding this comment

elisseck Sep 7, 2022

Choose a reason for hiding this comment

elisseck commented Sep 19, 2022

demeritcowboy commented Sep 20, 2022

elisseck commented Sep 20, 2022

demeritcowboy commented Sep 20, 2022

elisseck commented Sep 20, 2022 • edited Loading

demeritcowboy commented Sep 20, 2022

elisseck commented Sep 20, 2022

kcristiano commented Sep 20, 2022

elisseck commented Sep 20, 2022

elisseck commented Sep 20, 2022 •

edited

Loading