Provide clarification regarding handle_multiple-annotation on Encode-plugin

Thanks @lammensj for creating this issue about our Slack thread. To the maintainers, I suggest removing the handle_multiples = TRUE from the encode plugin.

Another (non-breaking) solution might be to create a specific Decode-plugin that doesn't handle multiples. Different options are possible :-)

Hello!

Yes, this does look like a valid issue with this plugin - and I agree with you @lammensj that probably these should be 2 different things.

Posting my comment from slack for reference:

Yes - that does look like an issue with the plugin. I'm going to be honest, I'd not see that plugin before! It is doing 2 things and feels like it should really be 2 different plugins. It allows encoding and decoding. Encoding functions can handle multiple values, but the decoding functions don't - they expect the format of whatever function is configured - json_decone, unserialize and base64_decode all require a string input.

I think removing handles multiple is going to break things for people encoding things - I'll move my thoughts to the Drupal issue, but I suspect maybe 2 plugins (encode allows multiple inputs / produces single value, decode allows single input/ can produce multiple values) and then we might need to provide some backwards compatibility for people using the existing encode plugin to decode this?

https://drupal.slack.com/archives/C34CECZAL/p1696530311707069?thread_ts=...

Thank you so much for the contribution!

I've updated the MR with a coding standard fix + added functional test to ensure the decode plugin is submitting configuration as expected.

I've then pushed a further commit as an approach to deprecate using the decode methods in the encode plugin. I think even though we only have an alpha release, we have enough modules and people relying on stability to not just remove them outright. The trouble is its hard for tamper to do any kind of update hook given we don't really know how all people are using this module.

I'm keen for feedback / discussion around the deprecation - specifically if we should be removing these options from the UI at this point but allowing existing config to be re saved.

Ah, introduced a couple coding standards in that last push, will tidy up on Monday

This looks pretty good to me, nice move. The deprecation path sounds solid, although the former version of the plugin wasn't really usable in the first place, if implementations were to take the handle_multiples serious because it should have always received an array as data, and that should have already thrown an exception. So, you could also assume, the decode plugins had not been used, or they had been used by some other module in the wrong way.

Thank you for the review.

The multiple flag indicates that the plugin can handle multiple values, not that it only handles multiples. E.g with encode you can still pass in a string or an array. So it's possible to still have used the encode plugin with decode options and string source values.

I tested the patch with both the stable and the latest-dev versions of ECA & ECA Tamper modules and I'm still getting the "must be of type string" error when I run the action to encode in base64 (the input to be encoded is a simple string):

Before applying MR 20 to latest dev version of tamper module:
TypeError: base64_encode(): Argument #1 ($string) must be of type string, array given in base64_encode() (line 79 of /var/www/html/web/modules/contrib/tamper/src/Plugin/Tamper/Encode.php

After:
TypeError: base64_encode(): Argument #1 ($string) must be of type string, array given in /var/www/html/web/modules/contrib/tamper/src/Plugin/Tamper/Encode.php on line 31 #0 [internal function]: base64_encode(Array)

After, with stable versions of ECA & ECA tamper (note the array is missing from the inside of last parentheses):
TypeError: base64_encode(): Argument #1 ($string) must be of type string, array given in /var/www/html/web/modules/contrib/tamper/src/Plugin/Tamper/Encode.php on line 31 #0 [internal function]: base64_encode()

The PHP Serialize and JSON encode options work fine instead, in both stable & dev versions (after applying MR20), but I noticed that even if the data to encode was a simple string, the encoded result was an array, eg:

• JSON encoded = a:1:{i:0;s:15:"ciao, che bello";}
• PHP Serialized = ["ciao, che bello"]

For reference: the code responsible for always providing an array to the Encode-plugin, when using Tamper via ECA, can be found here: https://git.drupalcode.org/project/eca_tamper/-/blob/1.0.x/src/Plugin/Ta.... Be aware that this is a separate module.

@kopeboy, after further investigation I believe we should shift the problem that you're encountering to the eca_tamper-module. The last comment from @ericgsmith explains what the annotion-property "handle_multiples" means. This was not clear when eca_tamper was created and thus its adaption should be altered.

I've create https://www.drupal.org/project/eca_tamper/issues/3439502 ✨ Refactor usage of "handle_multiples" for Encode-plugin Active for this.

Reverted to the last status.

I kind of agree but I'm not an expert coder 🙃 I'll follow up there, Thank you.

Just wanted to sanity check the deprecation removal - https://drupal.slack.com/archives/C34CECZAL/p1714710131685999

As discussed above - there has only been an issue with ECA using this plugin, or with feed tamper if decode options were called with a multiple value - calling with decode and a single value via feeds tamper is working - so feeds would be impacted by removal. I'm happy with the change.

Actually reviewing #11 - while ECA should not be converting it to an array there is still the same issue here.

base64_encode only accepts strings, but serialize and json_encode can handle multiple values.

Maybe we should split them all out?

Related issue: 📌 Encode plugin: only call function from the available options Active .

Perhaps we would need test coverage too to check if multiple values are handled well for those modes that can.

FYI there was a recent MR at https://www.drupal.org/project/eca_tamper/issues/3439502 ✨ Refactor usage of "handle_multiples" for Encode-plugin Active , but that didn't solve the issue on the Base64 encode action.

I've added a table to the issue summary that gives an overview of all modes and whether or not they can work with arrays.

Keeping it in a single plugin is doable

Adding a separate Decode-plugin and then deprecating the decode options for the encode plugin sounds like a nice idea. But at the same time I think we could fix the issue in the Encode plugin itself by just checking whether or not we receive an array or string. If it is an array, we could just iterate across all values. The Keyword Filter plugin does this as well. This would be less disruptive for existing users of the plugin.

Return value can be multiple values sometimes

Additionally, the plugin does need to report whether the result is multiple values or not. 'Unserialize', 'JSON decode' and 'YAML decode' can return multiple values, so for these modes the method multiple() should return TRUE if the result is an array. The plugin 'Required' does this as well. The Keyword Filter plugin does not, but I think that is a bug. I will create a new issue for this later.

Test coverage

If we keep it all in a single plugin, we need additional test coverage to ensure that each mode can handle multiple values. We also need test coverage for the method multiple() returning the right value: TRUE when an array is the result, FALSE otherwise. I've added tasks for these to the remaining tasks list in the issue summary.

Since 📌 Encode plugin: only call function from the available options Active touches the same code, I think it would be better to first finalize that one and then work on a fix for this one. Test coverage could already be written however. I think I'll make a start on that.

Let me know your thoughts! :)

I've added some tests.