Inject the view executable factory where possible

I experimented with this replacement in a few places -
In views_ui\Form\Ajax\AddItem the replacement was fine but in views_ui/admin.inc functionality was disrupted to the point that the 'add' button in a view would not function.

My thought was to add the following code to each occurrence of $view->getExecutable(), to see if executable factory can safely be used instead.

$ex2 = Views::executableFactory()->get($view);
$executable = $view->getExecutable();
if($executable == $ex2){
  drupal_set_message('view object match');
}else{
  drupal_set_message('view object does not match');
}

I'd like to get verification from a more experienced contributor that determining an object match is sufficient evidence that ->getExecutable can be replaced by executable factory.
If this looks like it will work, I'll apply this test to each occurrence, change to executable factory when the objects match, and create a patch.

@a.hover Did you have any questions?

It looks like the ::getExecutable method still exists in core/modules/views/lib/Drupal/views/Entity/view.php but it is not being called anywhere in the system.

Is this issue still relevant?

If we are not using the global, perhaps it should be removed altogether, lest someone accidentally start using it again?

I found some usages of getExecutable() with phpStorm "Find Usages" function. Will try to replace it with Views::executableFactory()->get($view); and publish a patch.

Please, check this patch and say if it is going in the right direction.

HI,
I just rerolled #8 patch witch didn't apply on latest 8.0.x. Without doing any changes, I ran tests witch have previously failed (Drupal\views\Tests\Handler\HandlerTest, Drupal\views_ui\Tests\DisplayCRUDTest) and finally those tests pass.

Drupal Dev Days Mentor Sprint - Montpellier (France) 2015

Thanks for moving this issue forward. It wasn't finished though so I'm moving it back to "Needs work" but that's great it's green now. Do you want to finish my work or should I take care of it?

Also, I've added drupaldevdays tag since you are working on it here.

Ok, I think I went through all occurrences of getExecutable(). Let's try patch it against testbot.

This just switches to using a static method call, which is a wrapper around \Drupal::service('views.executable')

As the issue title says, we should be injecting the factory instead.

Let's see what the testbot says

1. Agreed, fixed
2. ok
3. done
4. ok
5. I didn't add it there since it's not needed in any other relationship handlers.
6. fixed
7. fixed
8. fixed
9. This is needed to initialise the display handler.
   I'm thinking this was not necessary before since this was already being called in View::calculateDependencies() when a new view object was constructed.
10. fixed
11. fixed
12. fixed
13. fixed
14. Removed the comment since it's inaccurate as we are actually using setOption() there

Perhaps dawehner should verify 5 and 9 since I'm not sure about those.

When I remove the $executable->initDisplay() there I get a white screen with the following error:

Call to a member function has() on a non-object in /var/www/html/d8.local/drupal/core/modules/views_ui/src/ViewEditForm.php on line 209

Same kind of error is thrown whenever I try to call something on $executable->displayHandlers without adding the $executable->initDisplay() line first.

Well at least I should be using the $executable variable there.

Ok then I can actually remove that line since the display is initialised in setDisplay()

I just checked every addition of the the $executable->initDisplay() line.
Looks like the above was the only one that could be removed though, in the other occasions we call a method on $executable->displayHandlers or $executable->display_handler without calling $executable->setDisplay() before that.

Thanks for reviewing, added the beta evaluation.

Improved the IS a bit.

Thanks for the feedback

This is a lot of disruption for:

Normal because we are replacing code that works with code that is the current best practice.

How likely is it that this will break existing Drupal 8 sites?

Well in order to keep DS working in D8 I had to update the views integration 12 times since january. (last time this week) This change will brake it *again*.
So I don't think it's ok, while being in beta, to randomly change constructor arguments if they don't fix any real problem.

Well, it's a grey line.

For example, let's compare these two cases:

(1) For some reason we add new constructor argument to 'DefaultPluginManager' and by doing that we would break every contrib module that deals with custom plugin managers.
(2) We commit this patch and we would potentially break contrib modules that extends one of the affected classes.

From a technical perspective (1) and (2) are the same.

(1) will never be allowed at this stage unless it fixes a critical/major bug
(2) possibly will be allowed because the affected code out of core will be very limited at the moment.

To be honest, I personally don't care because I don't have any working D8 sites at the moment. But *if* people are using it (and apparently according to the statistics, they do), they have no clue which version of the module they need to use to make everything work for them.

This issue was reverted to Needs Review in comment #39, based on needing to discuss how much disruption it's likely to cause (really a Needs Work?)

At any rate, I'm going to remove the novice tag, because I don't think it's reviewable by a novice in its current state.

+++ b/core/modules/views/src/EventSubscriber/RouteSubscriber.php
@@ -121,8 +132,7 @@ public function routes() {
-      if (($view = $view->getExecutable()) && $view instanceof ViewExecutable) {
+      if ($view = $this->executableFactory->get($view)) {

I think that replacing the $view variable with the executable is bad code style because it makes it really hard to read or debug the code, or inherit methods.

It's a problem with the existing code, so I'd be ok with this being done in a follow-up.

Adding parent #2729597: [meta] Replace \Drupal with injected services where appropriate in core because views executable is a service. All of those code paths eventually reach Views::executableFactory() which is just a wrapper for \Drupal::service('views.executable').

One reason to do IoC and inject the service is that we're calling a bunch of functions that all wrap wrappers. Except for the very last one, of course. So let's just call the last one, once.