Support OOP hooks

I don't get why we're getting the container not initialized error:

    Drupal\Tests\field_encrypt\Kernel\DynamicEntityHooksTest::testDynamicFunctionRegistration
    Drupal\Core\DependencyInjection\ContainerNotInitializedException:
    \Drupal::$container is not initialized yet. \Drupal::setContainer() must be
    called with a real container.
    

Tests are broken on minor, so postponing this on 🐛 Support Drupal 11.1 Active .

I don't think this actually needs to be postponed, I'll let you change that though, but just transferring comments here.

Unfortunately dynamically created hooks are unsupported in the new OOP system. It is listed under breaking changes.

You'll want to implement one hook for update and insert like this:

function field_encrypt_entity_insert() {
  if (in_array($entitty_type, \Drupal::state()->get('field_encrypt.entity_types', []) \Drupal::service('field_encrypt.process_entities')->decryptEntity(\$entity);
}

Then you can order that using HMIA normally.

Once you've done that you can remove the eval code and the function checking for eval availability.
You'll also be able to remove the phpcs ignore rule you have to ignore complaints about eval.

Let me know if you have any other questions!

One more note: I think once you do this, you WILL need the patch here: 🐛 Ordering of alter "extra hooks" is a gigantic mess Active since you will be reordering using extra types.

nicxvan is right: the new system can't support such eval'd code and it's perhaps easiest to decide runtime instead. But what if someone really, really, really wants to define hooks dynamically? It's doable: HookCollectorPass merely automates the following three things when it finds a method marked with the Hook attribute but it's totally doable manually.

1. Register the class as an autowired service (with the classname as the service id):

Drupal\foo\FooHooks:
  class: Drupal\foo\FooHooks
  autowire: true

2. Add a kernel.event_listener tag on the definition:

$definition->addTag('kernel.event_listener', [
  'event' => "drupal_hook.$hook",
  'method' => $method,
  'priority' => $hook_data['priority'],
]);


3. Record which module this listener belongs to in the container argument hook_implementations_map:
<?php
$map[$hook][$class][$method] = $hook_data['module'];
// ... later
$container->setParameter('hook_implementations_map', $map);

Note you can add as many hooks as you want on the same class/service and even the same method.

Also note the Hook attribute is not used after this.

So if you do not want to decide run time which entity type should fire your service then you could use a service provider to iterate your entity types and add the right tag directly on the field_encrypt.process_entities service method decryptEntity and record it in the map:

class FieldEncryptServiceProvider extends ServiceProviderBase {
  public function alter(ContainerBuilder $container) {
    $map = $container->getParameter('hook_implementations_map');
    $definition = $container->findDefinition('field_encrypt.process_entities');
    $class = $definition->getClass();
    $method = 'decryptEntity'; // perhaps this should be a class constant
    foreach ($container->get('state')->get('field_encrypt.entity_types', []) as $entity_type) {
      $hook = "entity_{$entity_type}_insert";
      $definition->addTag('kernel.event_listener', [
        'event' => "drupal_hook.$hook",
        'method' => $method,
        'priority' => 0,
      ]);
      $map[$hook][$class][$method] = 'field_encrypt';
    }
    $container->setParameter('hook_implementations_map', $map);
  }
}

You could keep the current code for pre-Drupal 11.1 codebases and use this for Drupal 11.1 and eventually drop the old one.

@nicxvan and @ghost of drupal past: Thank you for all that info; it's very helpful. The eval hooks will need to be rethought for the 11.1 versino.

However, there may be another issue still. The "container not initialized" failure appears to be coming from calling \Drupal::state() in hook_module_implements_alter(), not the eval hooks. I made an MR that drops all the eval hooks (disable_eval MR on this issue), and I'm still getting a "container not initialized" error in Drupal 11.1, but not in 11.0 (ignore the other test failures, because the eval hooks test obviously fails when they are not present).

It's caused by this part of the module_implements_alter():

  elseif (preg_match('/_(insert|update)$/', $hook)) {
    foreach (\Drupal::state()->get('field_encrypt.entity_types', []) as $entity_type_id) {
      if ($hook == $entity_type_id . '_insert' || $hook == $entity_type_id . '_update') {
        // Move our implementations as early as possible so other
        // implementations work with decrypted data.
        $group = $implementations['field_encrypt'] ?? FALSE;
        $implementations = ['field_encrypt' => $group] + $implementations;
      }
    }

The change record says that dynamic hooks don't work, but it doesn't say anything about new restrictions on accessing services in hook_module_implements_alter(). Could you please share your thoughts on this?

Yes sorry if it was not clear.

The explicit reason is because hmia is called during build.

But why are you calling drupal state there, it is so you can then dynamically generate hook implementations and order them.

If you take one of the two solutions recommended you can just order the specific hooks you create and you don't need the Drupal state call.

Hmm. Actually this hook calls state for two reasons. One is to reorder the dynamic hooks that are eval'd into existence; this implementation definitely has to be changed.

However, not all servers allow the use of eval(). For those servers, the Field Encrypt module allows developers to define the hooks that would otherwise be eval'd in a custom module. So this hook is also calling state to reorder hooks implemented in custom modules. So I guess we have to change the way we handle that case as well.

FWIW we cannot use the generic hook_entity_insert() here. We need to decrypt the entity at the very beginning of the calls to the hooks and hook_ENTITY_TYPE_insert is called before the generic hooks. That's why all this complexity exists in the first place. Using the eval method increases this module's compatibility with other modules massively as no module expects encrypted data :)

I think #9 might offer a way forward that removes the eval usage and is potentially very interesting.

In that case use priority PHP_INT_MAX.

If you don't mind continuing to document here we'll likely want to clean this up and post it as a guide for advanced usage for any other contrib maintainers that have similar complex needs.

ptmkenny → changed the visibility of the branch disable_eval to hidden.

ptmkenny → changed the visibility of the branch disable_eval_state to hidden.

ptmkenny → closed merge request !67

ptmkenny → closed merge request !66

@nicxvan Yes, once we get this working, I'd be happy to write up a guide.

ptmkenny → closed merge request !64

ptmkenny → closed merge request !68

This MR now basically works; I am running my behat tests on my site locally with the MR on 11.1.x-dev and there seem to be no errors related to field_encrypt.

Suggested release notes/change record snippet:

Field Encrypt 4.0 no longer uses eval(). If you used Field Encrypt 3.x and your server did not allow the use of eval(), you had to define hook_entity_type_insert() and hook_entity_type_update() in a custom module to allow entities to be decrypted. This code is no longer necessary in 4.x and should be removed prior to upgrading.

@alexpott FieldEncryptUpdatePathTest is failing; do you have any idea why that might be?

I'm leaving this issue "Postponed" for now because I think we should commit the hook conversion first and then commit this ServiceProvider change separately instead of committing everything at once.

I am using commit `ef79e5b425c` of oop_hooks_workaround4

And drush updb returns this error:

   [error]  Error: Call to a member function insert() on null in Drupal\Core\Lock\DatabaseLockBackend->acquire() (line 71 of /var/www/html/web/core/lib/Drupal/Core/Lock/DatabaseLockBacken  
  d.php) #0 /var/www/html/web/core/lib/Drupal/Core/Cache/CacheCollector.php(235): Drupal\Core\Lock\DatabaseLockBackend->acquire()                                                            
  #1 /var/www/html/web/core/lib/Drupal/Core/Cache/CacheCollector.php(312): Drupal\Core\Cache\CacheCollector->updateCache()                                                                   
  #2 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(691): Drupal\Core\Cache\CacheCollector->destruct()                                                                              
  #3 /var/www/html/vendor/drush/drush/src/Boot/DrupalBoot8.php(312): Drupal\Core\DrupalKernel->terminate()                                                                                   
  #4 [internal function]: Drush\Boot\DrupalBoot8->terminate()                                                                                                                                
  #5 {main}. 

No idea what is going on, but poking around through intuition and git bisect I found that src/FieldEncryptServiceProvider.php causes the error, and deleting that file makes the error go away, although likely causes other problems because I imagine it is there for a reason.

ptmkenny → closed merge request !71

@elkaro If you delete src/FieldEncryptServiceProvider.php, field encryption and decryption will no longer work. It'd be great if you could provide more info about what is going wrong.

@ptmkenny unfortunately I don't know any more than I put in my comment. When I ran drush updb and src/FieldEncryptServiceProvider.php was present, I got the error, if I temporarily removed the file I could run drush updb successfully.

On Drupal 11.1.4.

@elkaro Thanks for the response. Did you have Field Encrypt enabled for any entities?

@ptmkenny Yes I am using it to encrypt some node fields.

@elkaro Well, the good news is that the error "Call to a member function insert() on null" is the same error identified in the Unit tests, so we already have test coverage for this:

    Field Encrypt Update Path (Drupal\Tests\field_encrypt\Functional\FieldEncryptUpdatePath)
     ✘ Update
       ┐
       ├ Exception: Error: Call to a member function insert() on null
       ├ Drupal\Core\Lock\DatabaseLockBackend->acquire()() (Line: 71)

Now to fix the tests...

Ok, the problem is caused when $container->get('state') gets called in alter() during update:

  public function alter(ContainerBuilder $container): void {
    $map = $container->getParameter('hook_implementations_map');
    if (is_array($map)) {
      $definition = $container->findDefinition('field_encrypt.process_entities');
      $class = $definition->getClass();
      $container_state = $container->get('state');

Not sure how to fix this; will ask in Slack.

It looks like we have to go one of two ways here. Either to somehow fix state in Drupal core so it can be accessing during an alter implementation. Or we need to stop using state.

If we go for not using state we need to do something like:

• A new table just to store this value. We need something that is available early in the container - unfortunately I don' think we should be accessing entity definitions here otherwise we're going to hit the same problems.
• In the alter read from the table and set this value as a container parameter so all the other places we read from it can use it cheaply.
• In the places where we used to write to state write to the new db table instead.

I think ideally state would work in this situation so might try to find sometime to fix this in core but I feel that we might have to do new table approach for now so that we can be compatible with 11.1 because any fix for state is not going to happen there.

Using a separate key value collection is a good idea that might solve a lot of problems. We will need to change all the calls to state to use the new key value collection and we will need to provide an upgrade path.

Another option is the one outlined by nicxvan in #7 📌 Support OOP hooks Active ; I started this long series of MRs with the approach described in #9 instead, because at the time it seemed cleaner to me, and it was more like the old approach (we would dynamically create hooks for each entity type). However, given the trouble I have had making it work, maybe it would be better to start over with #7 and see where that leads.

Ok, the keyValue approach seems to be the right one, as all tests are passing (except 11.2 phpstan, but I think that's due to deprecations-- 11.1 is completely passing).

This is good to go now. We have test coverage and an update path. We should add some docs to the release notes about removing any manual implementations if you didn't use the eval() way of doing things in 3.x but this approach shows how OOP hooks are great and let's us do interesting things like this.

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