Scan for classes based on the PHP interface (WorkflowMessageInterface, ExampleDataInterface)

[totten]

Overview

Class-scanning allows one to write a class and have it recognized automatically (without any other explicit declarations). There are some existing class-scanners in core; this branch expands that. It is an alternative to #23848. It specifically scans for PHP classes based on the PHP interfaces.

(There should be at least 1 test-failure in the current draft. So far, I have been focused on running the integration-tests rather than r-run. Posting for a more complete test-run.)

Before

There is some precedent for class/interface-based scanning:

• Civi scans a few folders in civicrm-core for implementations of WorkflowMessageInterface.
• Civi scans a few folders in civicrm-core for implementations of ExampleDataInterface.
• Civi scans for BAOs that implement Civi's HookInterface or Symfony's EventDispatcherInterface.

However, each of these has a different scanner with slightly different quirks.

After

The PR provides a generic scanning mechanism. Additionally, it updates existing code to use the generic scanner for specific interfaces (WorkflowMessageInterface, ExampleDataInterface).

Technical Details: Generic mechanism

The helper, Civi\Core\ClassScanner::get(...), finds matching classes.

$classes = ClassScanner::get(['interface' => 'Civi\Foo\BarInterface']);
foreach ($classes as $class) {
  // Do something with $class.
}

Each extension can define classes that implement the interface, eg

// FILE: myextension/CRM/Myextension/Foo.php
class CRM_Myextension_Foo implements \Civi\Foo\BarInterface {
  public function doSomething() {}
}

The scanner requires a general opt-in (per extension). It should be safe for most extensions to opt-in, as it is designed to match existing code-conventions. However, the scanner requirements are slightly stricter. (For example, all PHP files in ./CRM or ./Civi must define eponymous PHP classes. If you have any script-files in those folders, they could be mis-fired by the scanner.) The opt-in requirement ensures that existing extensions will continue to work.

The typical opt-in should go through info.xml:

<!-- FILE: myextension/info.xml -->
<mixins>
  <mixin>scan-classes@1.0.0</mixin>
</mixins>

Alternatively, as a last resort (if you need to support an uncommon file-layout), there is an experimental hook:

function myextension_civicrm_scanClasses(array &$classes) {
  $classes[] = 'CRM_Myextension_Foo';
  $classes[] = 'CRM_Myextension_Bar';
}

Technical Details: Specific interfaces

• It scans the extensions for classes that implement WorkflowMessageInterface (in addition to scanning the previous core folders).
• It scans the extensions for classes that implement ExampleDataInterface (in addition to scanning the previous core folders).
• It scans the extensions for classes that implement HookInterface (in addition to the previous BAO). (*This was included in the original draft. It generally works in practice, but it needs work to address an edge-case in phpunit testing, and it should be updated to support Symfony-style subscribers.)

FAQ

Q: What about Doctrine annotations? What about PHP attributes? What about YAML files? Should we register components with these?

They all have nice properties. But they all create dependency-hell. PHP interfaces are simple, inspect-able, and compatible with all supported environments.

Q: What are the performance characteristics? How will adapt to large source-trees?

There are a couple expenses to look out for: the cost of walking the source-tree, and the cumulative size of the class-list. There are some guard-rails that should keep each of these moderate:

• It only scans active extensions which enable scan-classes@1.0. It does not scan external PHP libraries or CMS plugins.
• It targets the common class-folders (./CRM and ./Civi). It ignores other folders (like ./vendor, ./templates, or ./css).
• It performs one scan, regardless of how many interfaces or extensions are used.
• It only cares for interfaces in the CRM and Civi namespaces. (Other interfaces are ignored.)
• It caches the outcome, storing separate indices for each interface. (Ex: There is a cache-record for WorkflowMessageInterface with a list of matching classes. When enumerating WorkflowMessageInterface, it will load this list. It doesn't load the full class-list.)

There are limits, of course. If you define an interface that is both very broad (eg used in a huge number of classes) and very active (eg used in most page-loads), then the cumulative effect would a huge number of class-loads during a most page-loads. So... don't do that.

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

[colemanw]

This seems cool. What about EventSubscriberInterface? That would be a good one to support.

[totten]

This seems cool. What about EventSubscriberInterface? That would be a good one to support.

We discussed this a bit more the other day. Outcome: It should basically use a copy of EventSubscriberInterface, because (1) it is a useful pattern, (2) other packages may use EventSubscriberInterface in different ways, where auto-registration is problematic and (3) we should have a buffer against flipflopping in Symfony's definition of the interface.

I'm going to split out the hook/event auto-subscriber stuff as a separate PR because (1) this PR is already big enough and (2) the hook/event stuff needs some work to address a phpunit issue.

[eileenmcnaughton]

@totten I think this might need to be against the rc as we actually need to get something into 5.51 to handle this functionality for import parsers to fix extensions to work with it

[eileenmcnaughton]

In terms of the code - it looks pretty good - I am going to test it out against the rc in our WMF config later today

[totten]

I think this might need to be against the rc as we actually need to get something into 5.51 to handle this functionality for import parsers to fix extensions to work with it

OK, it should be easy for me to rebase to 5.51-rc.

As it currently stands, I'm a bit more comfortable with the WorkflowMessageInterface / ExampleDataInterface being exposed that way (because they were not really available downstream before - so it's greener terrain). I'm not sure about #23877, but that's a separate PR.

[totten]

Split out a small cleanup in #23885. (#23885 is a simple test-only-regression that should be merged ASAP. It is needed on master/5.52 but not on 5.51.)

Rebased on 5.5.1

[eileenmcnaughton]

hmm conflict....

[totten]

Conflict should be resolved now. It was between (a) 5.51-rc commit adding a couple lines to example (691345e) and (b) PR commit to rename example file (d170eb5) and

[eileenmcnaughton]

would it be a cleaner up-merge if we pulled in that commit?

[eileenmcnaughton]

This works in my testing, allowing loading of templates from extensions with the mixin. Per above it might be cleaner to port the conflicting patches in - but I'm also OK merging as is