Always pass in the request into the view executable

Any reason to not pass it the request stack itself? Just curious.

Yes, I think I agree with that concept too. Makes sense.

How about a unit test for the factory whilst we are here.

Sure, let's do that.

1. +++ b/core/modules/views/tests/Drupal/views/Tests/ViewExecutableFactoryTest.php
   @@ -0,0 +1,108 @@
   + * Contains \Drupal\views\Tests\VIewExecutableFactoryTest.
   

   s/VIew/View/

2. +++ b/core/modules/views/tests/Drupal/views/Tests/ViewExecutableFactoryTest.php
   @@ -0,0 +1,108 @@
   +    $request_1 = $this->getMockBuilder('Symfony\Component\HttpFoundation\Request')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +
   +    $request_2 = $this->getMockBuilder('Symfony\Component\HttpFoundation\Request')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +
   +    $this->requestStack->expects($this->at(0))
   +      ->method('getCurrentRequest')
   +      ->will($this->returnValue($request_1));
   +    $this->requestStack->expects($this->at(1))
   +      ->method('getCurrentRequest')
   +      ->will($this->returnValue($request_2));
   +
   

   In my opinion it is not necessary to mock Request and RequestStack.

Generally looks good to me. Given that it touches ViewsExecutable, I suggest to also update getExposedInput() and use $this->request instead of \Drupal::request(). Also do not forget to update the documentation of setExposedInput().

It seems that this issue will allow us to remove a great deal of request-service dependencies from views handlers. The candidates (including a couple of false positives):

$ find . -path '*/Plugin/views/*' -type f | xargs git grep -l request
core/modules/node/lib/Drupal/node/Plugin/views/argument_default/Node.php
core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.php
core/modules/rest/lib/Drupal/rest/Plugin/views/style/Serializer.php
core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_default/Tid.php
core/modules/user/lib/Drupal/user/Plugin/views/argument_default/User.php
core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.php
core/modules/views/lib/Drupal/views/Plugin/views/argument/Date.php
core/modules/views/lib/Drupal/views/Plugin/views/argument_default/Raw.php
core/modules/views/lib/Drupal/views/Plugin/views/cache/CachePluginBase.php
core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php
core/modules/views/lib/Drupal/views/Plugin/views/pager/SqlBase.php
core/modules/views/lib/Drupal/views/Plugin/views/query/QueryPluginBase.php
core/modules/views/lib/Drupal/views/Plugin/views/style/Table.php
core/modules/views/lib/Drupal/views/Plugin/views/wizard/WizardPluginBase.php

For the reference Drupal\views\Plugin\views\area\HTTPStatusCode is an example of how the request is accessed properly.

#2059003: Remove try/catch around getRequest() in DisplayPluginBase::getHandlers() looks related.

You might want to rename this issue then because you guys just blew up the scope massively.

For the reference Drupal\views\Plugin\views\area\HTTPStatusCode is an example of how the request is accessed properly.

Yeah, we added that class so we are familiar with it.. :)

Also, I think we should still mock the request stack etc... The view executable factory is the class under test here.

You might want to rename this issue then because you guys just blew up the scope massively.

Sorry for that. The last part of my comment was meant as a pointer for a follow-up, but I failed to state that explicitely.

we should still mock the request stack etc...

I disagree. We do not have to expect any side effects from using them directly.

Well you mock things so you can set up expectations etc... too, but whatever.

Looks like we are going with the bigger issue now :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -893,8 +893,7 @@ public function getHandlers($type) {
+          if ($this->view->getRequest()->request->get('form_id') && isset($this->view->temporary_options[$type][$id])) {

getRequest()->request

Well you mock things so you can set up expectations etc... too, but whatever.

I agree in general. In this case the expectations do not contribute to the test coverage. On the other hand using Request and RequestStack directly improves readability a lot.

OK, let's move on from the test issue. It was just meant to be a small/quick test.

We might as well just carry on with this conversion now I guess!

10: vdc-2261181-10.patch queued for re-testing.

Reroll.

There is still $this->view->getRequest()->request->get('form_id') in core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php (see #15). It should be $this->view->getRequest()->get('form_id').

Oh, ignore #21, I'm blind.

I think we have a problem with cloned views. No request gets set on those.

Ah! destroy.

Committed/pushed to 8.x, thanks!