Views argument validators that modify argument values aren't reflected in token replacement

Curious what the test bot thinks of this. ;)

Someone will undoubtably want a test that shows the bug. I couldn't find a single argument validator in core that alters the argument value. :( So, I guess we'd need to write one. Or better yet, let's move https://www.drupal.org/project/views_taxonomy_term_name_into_id into core. ;)

Sweet, thanks for the test! Looks good to me.

Since this is looking viable to commit soon, I figured it'd be wise to clean up the related code comments a bit, too.

Also, happy to report this applies cleanly to the end of 8.6.x branch, too.

Sure, makes sense. I'll re-roll and give it a try. Stay tuned.

Yup, works fine with local testing. This should be RTBC as soon as the bot comes back clean.

Thanks!

Side note: If we don't like touching $argument->argument directly, it probably shouldn't be public:
#2959929: Consider making \Drupal\views\Plugin\views\argument\ArgumentPluginBase::$argument protected

Another quasi-related follow-up for folks who understand this code:
#2959933: Document \Drupal\views\Plugin\views\argument\ArgumentPluginBase::$value and clarify how/why it's different than ArgumentPluginBase::$argument

Cheers,
-Derek

Yay, thanks!

Cheers,
-Derek

Ugh. I was kinda wondering about that uid argument test using string values. Maybe this works?

While we're at it, maybe that test should ensure the non-transform case isn't broken? Or perhaps we already have tests for this elsewhere?

Not sure why #20 failed (I'm probably doing something wrong by trying to reuse the same test case and not sufficiently restarting things), but #19 is the right fix. Probably RTBC, but I'll let someone else decide that.

Thanks/sorry,
-Derek

Sorry for the trouble this issue has caused. :/

@xjm: Duly noted about requesting Postgres tests for views issues. Re: converting a string into a number in the test... that's exactly what views_taxonomy_term_name_into_id is trying to do, so I'm happy to have a test in core that ensures it works. Frankly, that module should be in core, then we could have test coverage of exactly that case.

Generally, it seems the "argument validators that transform values" case is sorely undocumented and untested in core right now. This is a small step in the right direction.

Feel free to commit #27. I think it's RTBC.

Meanwhile, at the risk of testbot and issue churn, here's a new test-only patch so it's obvious that this fails on both platforms (albeit they'll potentially fail for different reasons).

I'll re-upload #27 afterwards so the bot doesn't change the status here.

Here's #27 again, as-is, to prevent the bot from changing this to needs work.

Okay, #31 failed as expected:

MySQL: https://www.drupal.org/pift-ci-job/936704
Postgres: https://www.drupal.org/pift-ci-job/936705

In both cases:

1) Drupal\Tests\views\Kernel\ViewExecutableTest::testArgumentValidatorValueOverride
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     '{{ arguments.uid }}' => '1'
-    '{{ raw_arguments.uid }}' => '1'
+    '{{ raw_arguments.uid }}' => 'this value should be replaced'
 )

#27 (and #32) show all tests passing on both MySQL and Postgres.

This confirms that:

a) The test ensures the functionality of this issue works. It's not failing b/c Postgres is brittle, it's failing because core doesn't honor argument validator plugin value transformations.

b) We did not break Postgres this time. ;)

Even though technically it's "my" patch, I really just split and re-uploaded parts/whole of Lendude's #27. So I'll be bold and re-RTBC.

Sweet, thanks once more! And sorry again for the trouble and extra work.

Cheers,
-Derek

p.s. @xjm: Glad you liked #20. ;) I'm slightly sad some variant of that test didn't make it into core. Oh well...