Problem/Motivation

Generating combined values using tokens is very easy using something like this:
\Drupal::token()->generate('array', ['join' => $token], ['array' => $items], $options, $bubbleable_metadata);

Unfortunately it is not possible, to specify how the items of the array should be joined. Setting 'join' in $options has no effect, since token_render_array() is called with array('join' => '') + $options. This sets the join-option to an empty string, regardless of the value in $options.

Proposed resolution

Use array_merge() for the options.

CommentFileSizeAuthor
#10 2685525-impossible-to-override-join-option-10.patch1.78 KBmxh
#9 2685525-impossible-to-override-join-option-9.patch1.87 KBmxh
#4 fix_token_render_array_options-2685525-4.patch1.91 KBstborchert
#2 fix_token_render_array_options-2685525-2.patch582 bytesstborchert

Comments

stBorchert created an issue. See original summary.

stborchert’s picture

stborchert
he/him
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new fix_token_render_array_options-2685525-2.patch582 bytes

The patch simply uses array_merge() to set custom options.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks for the patch.

Trying to enforce having test coverage for all bugfixes.

Could also be fixed by doing $options + [...].

stborchert’s picture

stborchert
he/him
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new fix_token_render_array_options-2685525-4.patch1.91 KB

I've updated the patch using + instead of array_merge() since this is much faster and added a test.

hussainweb’s picture

hussainweb
Primary language English
commented 
+++ b/tests/src/Kernel/ArrayTest.php
@@ -57,4 +57,22 @@ class ArrayTest extends KernelTestBase {
+      'join:/' => 'a-b-c-d',
+      'join' => 'a-b-c-d',
+      'join:, ' => 'a-b-c-d',
+      'join: ' => 'a-b-c-d',

Shouldn't the delimiter in these strings override the join specified in the options? Where is this delimiter used anyway?

stborchert’s picture

stborchert
he/him
Primary language German
commented

No, if the delimiter is specified in the options it is meant to be used as override generally (at least as I understand the options ;) ). In my opinion it would be inconsistent to be able to set the delimiter on "normal" joins but not for dynamic tokens.

hussainweb’s picture

hussainweb
Primary language English
commented

I am all for setting the delimiter when not specified on the tokens itself. It just seems more obvious to me that the join delimiter specified in the string should override the one specified in code. I should say that I am not sure of the original use case here.

mxh’s picture

mxh commented
Status: Needs review » Patch (to be ported)

Confirming this problem, +1 for switching the order to $options + ['join' => ...] for being able to define the glue.

Patch must be updated to be compatible with version 1.3+

mxh’s picture

mxh commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 2685525-impossible-to-override-join-option-9.patch1.87 KB

Ported so it can be applied to the latest dev state.

mxh’s picture

mxh commented
StatusFileSize
new 2685525-impossible-to-override-join-option-10.patch1.78 KB

Re-rolled.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

I'd suggest to write a test for the new code to be green after this has been committed to ensure it works in the future and a separate patch with test to show it failed so far. If we agree it's OK to have tests for the future expected behaviour, that would also be OK. They should fail now.

Logically the patch is perfect and I'm willing to set this RTBC as soon as the tests are OK.