Rebuild the Drupal route cache.

[homotechsual]

Overview

This is linked to issue 158 and issue 37.

It takes an alternative approach to #19888, adding a Utils/System function to rebuild the routes which we can then invoke from multiple places if required with an invocation added to the rebuildMenuandCaches function in invoke.php.

Before

Enabling a CiviCRM extension that adds a route on Drupal 8 or 9 results in a 404 until the routes are rebuilt in Drupal.

After

Enabling/disabling an extension rebuilds Drupal's routes and routes added in CiviCRM work as expected..

Technical Details

There might be a more performant option for this but I can't find anything...

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

[homotechsual]

Alternative approach for #19888

[homotechsual]

Test sites:

D8: https://d8pr19906.bkonaz.mjco.uk/
D9: https://d9pr19906.bkonaz.mjco.uk/

[demeritcowboy]

I think what @seamuslee001 meant on the other PR by CRM_Utils_System_x classes is that instead of hardcoding Drupal8, here in Invoke you would call $config->userSystem->invalidateRouteCache() and in CRM_Utils_System_Base you would put a do-nothing function invalidateRouteCache() { } and in CRM_Utils_System_Drupal8 you would override that function.

[homotechsual]

I think what @seamuslee001 meant on the other PR by CRM_Utils_System_x classes is that instead of hardcoding Drupal8, here in Invoke you would call $config->userSystem->invalidateRouteCache() and in CRM_Utils_System_Base you would put a do-nothing function invalidateRouteCache() { } and in CRM_Utils_System_Drupal8 you would override that function.

That makes sense.

[homotechsual]

Refactored with @demeritcowboy 's suggested route and squashed.

[demeritcowboy]

I tested this in drupal 9 with cdntaxreceipts but I still needed to clear drupal cache after in order to avoid the "page not found". Hmmmm.

[homotechsual]

I tested this in drupal 9 with cdntaxreceipts but I still needed to clear drupal cache after in order to avoid the "page not found". Hmmmm.

If you switch it out to do \Drupal::service('router.builder')->rebuild(); do you have to clear the Drupal Cache?

[homotechsual]

Okay this works now, it turns out we do need to do a router.builder rebuild for this to work!

[demeritcowboy]

Ok neat. I guess we're then back to where is the best place. So this would trigger on any extension action even disable but maybe that's ok/desired.

[homotechsual]

Yeah, I'm thinking that having extension routes in the cache post-disable is undesirable so it's cleaner to run this on any extension enable/disable/install/uninstall.

[demeritcowboy]

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-doc] PASS
  • [r-run] PASS
• Developer standards
  • [r-tech] PASS
    • It seems like this would only be called during extension lifecycle events or some already heavy cache clearing event. I can't think of a better place to put it unless it's left to extensions to decide when to do it.
  • [r-code] PASS
    • I don't know that it needs to call class_exists('\Drupal') given that this is already within a drupal8-only file. Ditto hasContainer() although I suppose that call might be a safeguard against some weird edge case.
  • [r-maint] ?
  • [r-test] PASS

[homotechsual]

I went with checking for \Drupal and ->hasContainer() because the cost is minimal of those checks and it would catch any possible scenario where this tries to get invoked during installs or in situations where Drupal isn't booted yet.

[eileenmcnaughton]

Thanks for the review @demeritcowboy - I think the r-maint is OK since it's just a little bit of code that seems pretty readable

[eileenmcnaughton]

@MikeyMJCO this is 'linked to issues' - are they closable now or just linked

[homotechsual]

Closeable along with PR #19888

[homotechsual]

@MikeyMJCO this is 'linked to issues' - are they closable now or just linked

Issues and other PR closed :-)

[eileenmcnaughton]

thanks

[eileenmcnaughton]

@MikeyMJCO - although I see I said above the code is pretty readable I'm reading it now & I'm confused :-)

Specifically I don't understand why it's nested in the if trigger part here - because it seems the line is about rebuilding paths which is 'all the rest of the function' rather than re-instating logging

If it IS correct for it to be in that loop maybe you could add a PR to explain why it should only be done when rebuilding mysql triggers

[homotechsual]

It's a computationally very expensive operation - like rebuilding triggers. It's not something that should be called as often as we flush caches - the performance cost would be very high.

When we do things like disable/enable or upgrade extensions we're already setting triggerRebuild to TRUE so it felt more appropriate to put it in that conditional than call it any time we rebuild the cache.

civicrm-core/CRM/Extension/Manager.php

Line 228 in c706f46

CRM_Core_Invoke::rebuildMenuAndCaches(TRUE);

[homotechsual]

Described in comments in #20414