Options provider classes were added to Rules in #3254620: Add new OptionsProvider files.

In the core issue #2329937: Allow definition objects to provide options, options provider classes will be able to use \Drupal\Core\DependencyInjection\ContainerInjectionInterface to inject dependencies. We should do this with the few options providers we have that require service injection. And maybe also take this opportunity to add tests for these few options providers. See what I wrote about options provider tests in #3254620-8: Add new OptionsProvider files

Comments

TR created an issue. See original summary.

tr’s picture

Issue summary: View changes
tr’s picture

Status: Active » Needs review
StatusFileSize
new10.45 KB

Here is the dependency injection patch. No tests yet.

tr’s picture

StatusFileSize
new20.16 KB
new9.83 KB

Here's a new patch, which is the same as #3 but with tests and some changes to OptionsProviderBase.

Lessons learned from writing these tests:

  1. Dependency injection does work with options providers. The core issue #2329937: Allow definition objects to provide options defines a OptionsProviderResolver class that uses the core class_resolver service to instantiate the options provider class and inject the dependencies. We don't need to replicate that OptionsProviderResolver class or depend on the core patch, we just have to use the class_resolver service the same way, which is one line of code in the test.
  2. Unit tests are useless for testing options providers. Most of our options providers return a static, fixed array - there's no sense in writing a test for those. But five of our options providers use services to return a dynamic list of options - these are the ones that need testing. And again, Unit tests are useless for this because we would have to mock the service classes and mock the output of the services, so if core ever changed the name or output of these services the tests wouldn't complain because the mocks will still work. Too much work for no return.
  3. Kernel tests are useful to some extent for testing options providers, but only if your options providers don't depend a lot on which core modules are enabled. Kernel tests don't enable modules, so things like user entities, user roles, content types, blocks, etc. are not available in Kernel tests unless they have been specifically configured by the test. But options providers that provide lists of entities, roles, content types, etc. won't return anything unless we spend a lot of effort setting up the test. That's not true of all options providers, but it is true of all the ones we have in Rules at the moment. So for the purpose of this issue, no Kernel tests.
  4. Functional tests are resource-intensive, but for our purposes they're the only way to go here. Functional tests will set up all the modules, entities, roles, etc. that we need to test our options providers. But instead of the 'testing' profile or the 'minimal' profile, we should use the 'standard' profile for testing as it sets up content types and many more entity types than the other profiles.
  5. OptionsProviderBase needed some work. I modeled the changes on the SimpleOptionsProviderBase from the core issue. There are two concepts involved: 1) possible/settable OPTIONS, and 2) possible/settable VALUES. The options providers return keyed arrays intended to be used in a select form element, for example. The key is the VALUE, while the text string stored at that key is the OPTION. OptionsProviderBase wasn't returning the keys at any point, it was only returning the options.
  6. Also in OptionsProviderBase, the output of a provider could have categories - when used in a select form element these top-level category keys are displayed as group headers and are non-selectable. For example when listing the content types, the group might be 'Content' which might contain an array of value/options for each content type: 'page' => 'Basic page', 'article' => 'Article', etc. The options provider in this case should return 'page', 'article' as the VALUES and 'Basic page', 'Article' as the OPTIONS, but it should never return 'Content' as either a value or an option, as this is merely a label for a category rather than a valid choice. The SimpleOptionsProviderBase in the core issue addresses this by flattening the array, and we need to do the same in OptionsProviderBase.
  7. Finally, I added a sort to the LanguageOptions so that the options would be returned in predicatable order for the tests. All the other options providers already did this.
tr’s picture

StatusFileSize
new20.27 KB
new9.93 KB

I cancelled the test in #4 because I uploaded an older version of the patch. Here's the correct version of the patch to test / review.

tr’s picture

New patch with coding standards fixed. No functional changes.

jonathan1055’s picture

Excellent analysis in #4. I have just checked the flattening you added, by hacking the MessageTypeOptions and introducing a two-level array to group the message type. It works fine, as shown by the attached.

One more coding standard fault was introduced when you fixed the 8, so heres a new patch.

  • TR committed e00891a on 8.x-3.x
    Issue #3255038 by TR, jonathan1055: Dependency injection in options...
tr’s picture

Status: Needs review » Fixed

Thanks, I caught that too. Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.