As the issue summary only mentions PHPCS, I have resolved the PHPCS errors and changed the issue state to 'Needs Review.' However, since you mentioned that I need to resolve PHPUnit errors and pass the pipeline, I am now working on it.
Yes, this issue is to fix the PHPCS test. However, the phpunit tests are passing on dev, so if as a result of fixing the PHPCS test, the phpunit test breaks, it needs to be fixed here.
Thank you for fixing MR!154. However, as stated in the issue summary, we need to fix PHPCS issues in both the 2.0.x and the 2.1.x branches. It seems you hid the branch for 2.1.x--(3484975-phpcs-fixes-2.1.x)-- but this branch also needs to have an MR with the same fixes as 2.0.x. The reason is that both 2.0.x and 2.1.x are currently supported by the module. (Also, work generally must always be committed to the latest dev branch, which in this case is 2.1.x-dev, so there is no way we can commit 2.0.x without 2.1.x.)
Setting back to "Needs work" because we need an MR for 2.1.x-dev as well with green tests.
@kul.pratap Please make sure the test icon is green before setting to "Needs review." On the 2.0.x branch (3484975-phpcs-fixes), phpunit is failing, but it is not failing in dev (you can check the 2.0.x GitLab CI pipeline from the module page, which is currently here: https://git.drupalcode.org/project/metatag/-/pipelines/332334)
Since phpunit passes in dev but fails in 3484975-phpcs-fixes, then the 3484975-phpcs-fixes is broken and needs to be fixed before it can be reviewed.
The annotation class is still there, so annotations are still supported for people who have written their own plugins on top of JSON:API Extras and are using annotations.
As for the plugins in this module, it is fine to convert them to attributes now because the module's composer.json already has "php": ">=8.1", so attributes will definitely be supported (attribute support is from 8.1). So I don't think this should break anything for anyone.
As a module developer, I read over the change record and have a few questions:
1. If a module does not have any files where procedural hooks can be defined (for example, the only php code is classes in the /src directory), is there any benefit to specifying the parameter module_name.hooks_converted: true?
2. If a module has converted all hooks that can be converted to OOP but still has hooks that cannot be converted to OOP yet (for example, template_preprocess hooks), then module_name.hooks_converted: true cannot be specified and the attribute #[StopProceduralHookScan] needs to be used for such hooks, correct?
ptmkenny → created an issue.
Ok, thanks for the additional information.
My initial assessment is that this should be fixed in jsonapi_extras, not jsonapi_include. But, I am willing to consider a solution here if there's a good argument for fixing it in jsonapi_include instead.
Bumping to major because this can block sites on PHP 8.4.
This module has some PHP 8.4 deprecations, which are already addressed in the MR, so I'm changing the title because PHP 8.4 has now been released.
Sorry, I just saw this issue now. I'm assuming this is fixed by 🐛 webauthn-lib v5 removes jsonSerialize() Active ; if not, please feel free to re-open.
Ok, I've updated the MR and I think this is ready to go again.
Unfortunately this no longer applies to the latest dev, so setting to "Needs work" to fix the merge conflicts.
ptmkenny → created an issue.
Thank you! The problem is that it was set in services.yml.
ptmkenny → created an issue.
ptmkenny → created an issue.
Thanks for the review!
Yes, there are a lot of breaking changes; I need to do more testing before I make a new release with 5.x support.
@lawxen Thanks!
I'm setting this back to "Needs review" because the issue described in #17 and #20 is a separate bug that will be discussed here: 🐛 Didn't support jsonapi_default Active
MR157 needs the merge conflicts fixed.
Also added a note to the issue summary that this is an important issue because this fixes deprecation warnings in PHP 8.4.
@lawxen Could you please share the Drupal and module versions you used to reproduce this?
Thanks for this issue; the two modules should definitely be made compatible.
"the $include_parameter can't get the "include" query parameter on the second request"-- Why is this? Is the jsonapi_default module removing the include query parameter?
@lawxen Please open a new issue if you found a bug in the dev branch; this is a feature request.
Thanks for this issue. For the serializer, I think it will be safe to accept PublicKeyCredentialOptions, so I created an MR to do that.
Please review and let me know if this solves your issue.
Passing that into simplewebauthn/browser startRegistration() resulted in "TypeError: base64URLString is undefined"
I'm guessing that's because webauthn-lib 5 no longer serializes to JSON objects, so that's not "supposed" to work anymore? I do not know why this change was made, but this module is attempting to follow the underlying library as closely as possible.
Here's the relevant page from the docs: https://webauthn-doc.spomky-labs.com/pure-php/authenticator-registration
ptmkenny → changed the visibility of the branch 3490555-support-seralization-of to hidden.
ptmkenny → made their first commit to this issue’s fork.
Thank you @joshahubbers!
Thanks for the MR; these should definitely be filled in. I will re-use the description from composer.json, and I'm setting the package to "User engagement" because I think we should be referencing the Drupal.org categories where possible.
ptmkenny → made their first commit to this issue’s fork.
On the configuration form, if you are specifying Key Type "Encryption", you need to set a "Key size" (the key length), but you also need to set a "Key provider" (under "Provider settings"). For the "Key Provider", you can set "Environment" (for an envvar) or "File" for a file located on disk.
The prefix to the token is not part of the encrypted value, is it? So it should be irrelevant to the key length.
Rereading your post: are you trying to encrypt the shopify token using the token itself as the key? That won't work. You need to use an encryption key (for example, using the Real AES module) and then encrypt the shopify token with that (which could be stored with, for example, the Field Encryption module).
The Encrypt + Key modules are an API for encrypting data, nothing more. If you want to do secrets management using encryption, you will need additional modules (such as the Field Encryption or Vault for Drupal modules).
@dahousecat Yes, that could be opened up. Please open a new issue as a feature request.
How are you specifying the key, as an environmental variable or as a file?
You can set the key size on the configuration form. What key size are you setting?
ptmkenny → created an issue.
This was originally reported by pp.panatom in 🐛 Drupal 10.2 - field values on decimal fields getting lost Active , which I mistakenly closed-- sorry about that. I'm leaving that issue closed in favor of this one because it has tests.
3.2.0 has some trouble handling decimal values still; please try the MR in 🐛 Encrypted decimal field value not saved on drupal version 10.2.x+ Active and see if that fixes your issue.
ptmkenny → created an issue.
I could not reproduce this based on your steps to reproduce. Please provide more specific steps about how to reproduce this issue from a clean install of Drupal 10/11.
Also, please file bug reports after testing against the dev branch of the module (3.x-dev), just in case the bug has already been fixed in dev but is not yet released.
Forgot to wire up the attribute; that's fixed now and I think this is ready to go.
It depends on how you want mail on your site to work, but generally speaking, you can get rid of the other two, too, if you are using this module.
First I would install and configure this module and make sure you have it working properly before uninstalling the other modules.
ptmkenny → created an issue.
ptmkenny → created an issue.
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.
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.