Token: set random value integer limit min/max not working

I wanted to give some context here. I have a model that gets a list of Users via views, then gets the count of those, so that I can then randomize a number between 1 and that count. Then I subtract one to get an array key and grab that item from the array.
What I found was that when saving that the length value was using an empty check which was setting the length value to true or false.
After that I noticed that the value wasn't actually using tokens so I passed the length value through the token service at runtime.
After that this worked.

Adding the patch file so it can be used with composer

@jlockhart thank you for the MR, but this is beyond the scope of this issue. The fix in line 198 is related to this bug report. Everything else should be handled in a different issue as a feature request. And when supporting tokens, the form field requires an extra flag to tell the system and the user that it does. See similar action plugins.

Regarding patch files: please don't do that. It's against the DA recommendation. If you don't want to rely on the MR, which is OK, then you need to download the diff locally and patch form there. Patch files on d.o have no guarantee to remain where they are now.

@jurgenhaas Thanks for the feedback
On the patch; yeah sorry old habit there.

On the issue: just to double check. According to that fields form settings it has both the tokens setting and help text indicating it supports tokens.

    $form['length'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Length or min/max'),
      '#description' => $this->t('The length or min/max range of the random value. For sentences, this value determines the number of words and for paragraphs it determines the number of paragraphs. For integer mode, please provide comma separated values for minimum and maximum value. Missing values default to PHPs built in mix and max integer values. For the image mode, please provide a comma separated min and max resolution, defaults to "100x100,1920x1080". For the image mode, the token will contain the path to the generated random image file.'),
      '#default_value' => $this->configuration['length'],
      '#required' => TRUE,
      '#weight' => -10,
      '#eca_token_replacement' => TRUE,
    ];

So when I tried to use token in my action and it didn't work I thought it made sense to add here.
I'm still learning this module so if there is more missing, or the token needs to be handled a different way let me know.
Would like me to move that work to a different bug ticket?

Oh, my bad. So we declared token support but don't actually support tokens. Sure, that's a bug ;-) and your approach is alright. It could have been a separate issue, but as both parts are about a bug fix, let's keep it here together.

One final thought before merging this: while looking at the code again I realized that the password generator part is using a fixed length of 32, not idea why. Should we use the length variable there too?

Oh ok cool.

On the password, I didn't notice that. Yeah for me I would expect it to use the value I pass to it rather than a fixed number.
My only concern would be some sane maximum value. If I'm reading the generate() method correctly its just looping the length and adding a random character to the password. If someone accidentally adds a token that returns a huge number that might be a problem?

Not sure on that.

That's a valid point, but aren't there plenty of things that could be done that can easily get a PHP process into trouble, like e.g. a loop that goes through an insane list of records? I agree, there is a lot of power, but isn't the responsibility with the user, a.k.a. the developer of the model in that case? On the other hand, a max value of e.g. 1000 wouldn't hurt, but then we also have to explain that. And maybe somebody uses that function to generate a random string that really needs to be longer than that.

So, I tempted to go without a limit, but very open to other thoughts.

Agreed, all valid points. Cant solve for every use case. I think you're right in letting the developer set the limit.

OK, made that quick change and merged this. No need for a back port since this plugin was introduced in 2.1.0

Thank you both for your contributions on this.