🇯🇵Japan @ptmkenny

Thanks everyone!

ptmkenny → created an issue.

Lockr now has a notice on the homepage:

Lockr will be suspending SaaS services effective November 30,2024. Owners of all active Lockr accounts have been contacted regarding the end of services and migrations options. Please contact support@lockr.io with any questions.

Since both providers are no longer in business, I will remove this information from the README, but we'll leave this issue open in case someone has a recomendation.

ptmkenny → created an issue.

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.

Perfect!

I just tested this MR locally and can confirm that my problem reported in #7 (composer require of a module that needs 11.1 works, but actually enabling the module in drupal fails) is fixed.

@alexpott Yep, I saw that. But note that my comment seems to be that the reverse is also the case.

field_encrypt 3.2, which has a constraint "do not install on 11.1", can be composer required with 11.x-dev, even though it won't work. (your report)

But field_encrypt 4.0, which has a constraint "at least 11.1", can be composer required with 11.x-dev, but the module can't be enabled (giving the message "incompatible with this version of Drupal core")-- it seems it still thinks it is 11.0.x?.

So I'm not sure if this behavior should be a separate issue or part of this one.

I'm not sure if this is related, but I also ran into trouble getting field_encrypt 4.x, which requires ^11.1 in the module.info.yml, working locally.

In my case, when I tried using the constraint drupal/core: "^11.1.0-alpha1", composer install worked, but when I tried to re-initialize my site (I have a drush script that resets the datebase and reimports config), the config import failed with the error:

Unable to install modules: module 'field_encrypt' is incompatible with this version of Drupal core.

The same thing happened with the constraint "drupal/core": "11.x-dev@dev".

The module can be installed with composer and in Drupal with "drupal/core": "11.1.x-dev@dev".

ptmkenny → created an issue.

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

Whoops, already set, nevermind.

ptmkenny → created an issue.

ptmkenny → created an issue.

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

I propose asserting the new string instead of using contains to preserve backwards compatibility. Tests will fail on 10.3 and below, but GitLab CI isn't testing these, and the code will still work on 10.3, so I think it's better to require the exact value.

ptmkenny → created an issue.

After discussion, this will be handled in:

📌 Support OOP hooks Active

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

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

Yeah, allowing failure makes sense; I made this MR before I realized how hard 11.1 compatibility was going to be.

I've updated the MR to block installation on 11.1 for now. I definitely think there should be an 11.0-compatible release, and hopefully an 11.1-compatible release to follow shortly after.

Comment from fathershawn on Slack:

Both classes are essentially data objects. As I see it, they are related but not embodied in the same class.
Your instance of AttributeInterface powers discovery of your plugin classes and manages the metadata needed to instantiate them.
DefaultPluginManager::getDiscovery instantiates AttributeClassDiscovery
DefaultPluginManager extends PluginManagerBase and PluginManagerBase::getDefinitions passes it’s work to the discovery class. AttributeClassDiscovery::getDefinitions ultimately calls AttributeInterface::get to get the data from the attribute which has a return type of mixed and AttributeBase refines that as array|object . Which means that in the ::get method of your attribute you can return an instance of PluginDefinitionInterface
See \Drupal\Core\Layout\Attribute\Layout for an example.

ptmkenny → created an issue.

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.

@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?

Thank you very much for the quick response and clarifications!

Did this change the behavior of hook_module_implements_alter()? I'm getting a container initialization failure in the Field Encrypt module due to a \Drupal call: 🐛 Container not initialized in module_implements_alter() Active

Creating issue here: 🐛 Container not initialized in module_implements_alter() Active

ptmkenny → created an issue.

Even though the test is failing, I think this should be committed because, by adding a test, it shows that the tests are failing.

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

ptmkenny → created an issue.

The 2.x branch will not be updated to support Drupal 11. Use the 3.x branch instead.

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.
    

ptmkenny → created an issue.

On closer inspection, this seems to be a false alarm.

If you get a "container not installed" error, make sure your OOP hooks are correctly defined. I had incorrectly defined a hook not related to the field_encrypt module and was getting this exception that traced back to the field_encrypt module, but upon fixing my unrelated code, this exception was also resolved.

ptmkenny → created an issue.

The best practice is that the attribute will be backported to Drupal 10. So we'll commit this after core releases support.

This morning, I attempted to reproduce this again but could not. I tried using the same endpoint that I described in the issue summary, but the error no longer occurs for me.

So before I commit a fix for this, I need an example of when this is failing. It may be that the fix here is just covering another error where we should be throwing an exception instead.

The file list admin page is still available at https://example.com/backend/content/files

You can "fix" this by editing the configuration of the file list admin view at https://example.com/backend/structure/views/view/files/edit/page_1. Change the "path" from "admin/content/files" to "backend/content/files".

I don't understand why the content view at /admin/content is overridden correctly, but the files view at /admin/content/files is not. However, since there is a workaround available, I'm setting this issue to "Minor".

Thank you for the response. If you can point to where you think the specification is being violated, I'll consider it.

As for the principle-- well, this module flattens the response; it's basically the only thing the module does. If you don't want your responses flattened, you can just use JSON:API without this module.

@cchavi.sharma phpcs needs to be fixed, but much more important is documenting a way to reproduce this issue. We may add an additional unit test to make sure we don't break this again in the future.

Thanks for updating this. "Needs work" as tests are failing. Also, can you provide steps to reproduce this? I tried to reproduce it but failed, which is why I closed the issue in #5.

"Needs work" as per #17.

@drupals.user There haven't been any updates in 2+ years, so no, it isn't actively being updated.

As a workaround, you can use \Drupal calls without DI.

This sounds great. One request for clarification: the change record → notes that several hooks will "remain procedural", including theme hooks like hook_preprocess() and install hooks like hook_schema() and hook_update(). So does "disable procedural hook discovery" mean "disable all procedural hooks which can be implemented with the OO pattern" or does it mean "disable all procedural hooks period"?

At a minimum I would imagine hooks like hook_update() need to still be supported or sites will break when a contrib module has an update hook.

Thanks for the additional information. Setting back to active.

After struggling with this, I think we're blocked on ✨ Fatal error: Declaration of Drupal\Component\Plugin\Attribute\AttributeBase::setClass(string $class): void must be compatible with Drupal\Component\Plugin\Definition\PluginDefinitionInterface::setClass($class) Active .

ptmkenny → created an issue.

@jérôme dehorter Please do not hide branches without leaving a comment about why you are doing so. I am the module maintainer and I wrote the MR, and I think it may be committed to the module, so the MR should stay open.

ptmkenny → changed the visibility of the branch allow_keeping_includes to active.

ptmkenny → created an issue.

ptmkenny → created an issue.

Let's make this a 3.x feature.

This will be a 3.x feature and not backported to 2.x.

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

ptmkenny → created an issue.

ptmkenny → changed the visibility of the branch 2551893-add-entity-events-11x to hidden.

ptmkenny → changed the visibility of the branch 9.2.x to hidden.

Hmm, adding this change breaks Drupal 9, so perhaps this issue should be closed in favor of 📌 Add attributes alongside annotations Postponed , which then that issue can be committed someday when support for older versions of Drupal are dropped?

ptmkenny → created an issue. See original summary → .

As you referenced in the other issue, TamperManager needs to be updated to match this change record: https://www.drupal.org/node/3395582 →

I'll try to get to this tomorrow. I'll fix this issue and the other one first, and then see about adding a test just for this one.