Get the views plugin manager from the DIC when possible

Yes, looks good. When I originally did the DI patch/branch I was getting these directly, with this type of thing in mind.

views-useDIC.patch queued for re-testing.

+++ b/lib/Views/system/Plugin/views/wizard/System.phpundefined
@@ -0,0 +1,26 @@
+ * Provides a very generic Wizard that can be constructed for any base table.

This seems to be not totally right ... it's the system specific implementation

+++ b/views.moduleundefined
@@ -1424,35 +1425,17 @@ function views_get_plugin($type, $plugin_id) {
  *   An array of plugin definitions for a single type, or an associative array

Probably the docs have to be adjusted as well.

+++ b/views_ui.moduleundefined
@@ -521,57 +521,6 @@ function views_ui_view_preview_section_rows_links($view) {
-  // @todo - handle this via an alter hook for plugins?
-  foreach ($base_tables as $table => $info) {
-    if (!isset($wizard_tables[$table])) {
-      $wizard = $default_wizard;
-      $wizard['title'] = $info['title'];
-      $wizard['base_table'] = $table;
-      $wizard_plugins[$table] = $wizard;

We have to find a way to be able to provide a wizard for each table automatically, as you can't add a new view without it. Maybe an addition for the plugin manager class?

• Introduced a wizardDiscovery which adds the plugins which aren't in core
• Fixed some general rebase stuff

There was an issue with relying on a function which just existed on admin.inc, so i moved it to views.module. I think views_fetch_base_tables seems to be a generic helper method not tight to the admin interface.

assign my myself for fixing.

There was some code changed lately, so here is a reroll.

I had a talk with eclipse how to properly implement that.

The best solution was to

• Specificy the plugin id used for the wizard on hook_views_data, if it's empty views should provide the table as default.
• Therefore Implement a plugin with a derivative which creates plugins for all entries in hook_views_data which did not specified a plugin_id
• Existing wizards will just work as they do know, but they should be specific.
• This still allows us to build multiple wizards per base_table (not sure maybe gardens is doing that).

Here is a first version of that, the listing shows the system table, but creating a new wizard doesn't work yet.

+++ b/views.moduleundefined
@@ -1369,6 +1370,45 @@ function views_fetch_data($table = NULL, $move = TRUE, $reset = FALSE) {
+function _views_weight_sort($a, $b) {

Apparantly this is a configEntityBase sort function now. So we can remove this.

It's not what it looks like: http://www.youtube.com/watch?v=npjOSLCR2hE :)
This is about views data, which is a pure array, not something related with config entities at all.

Rerolled the patch against recent changes. The fail of the test is still there, but i asked eclipseGc for some help.
It seems to be that annotations + derivatives doesn't work together that fine.

Rerolled against the latest changes and fixed the test, see attached interdiff.

I'm sorry

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -101,8 +101,9 @@ public function init(ViewExecutable $view, &$display, $options = NULL) {
+      $container = drupal_container();
...
+        $plugin = $container->get('plugin.manager.views.display_extender')->createInstance($extender);

Any reason not to just do drupal_container()->get() here?

Thanks for the review!

The reason for that is probably that you have possible multiple calls to this.
But you are right, actually it should get the plugin manager instead.

First version for core.

#25: core-1783196-25.patch queued for re-testing.

#25: core-1783196-25.patch queued for re-testing.

#25: core-1783196-25.patch queued for re-testing.

Just a rerole.

I'm really wondering why i uploaded the wrong patch :(

#34: drupal-1783196-35.patch queued for re-testing.

Removed views_get_plugin_definition but the test failure is still reproducable. The problem is that for some reason
the TestFieldWidgetMultiple file get's scanned, even the plugin is disabled.

Rerolled, let's see how many patches are now failing.

This looks good, maybe the patch is slightly bigger than the title suggests though :) Oh well...

+++ b/core/modules/views/lib/Drupal/views/Plugin/Type/DefaultWizardDeriver.phpundefined
@@ -0,0 +1,55 @@
+          'class' => 'Drupal\views\Plugin\views\wizard\Standard'

Do we need the class here? Shouldn't we just need the plugin ID?

Do we need the class here? Shouldn't we just need the plugin ID?

So on a normal plugin you have the annotation discovery which sets the class.
If you have derivates you end up generating the class definition from getDerivativeDefinitions() which does
not have the class at that point. Sadly there is currently no base table in core which does not have a separated wizard
so you will not realize that.

#42: drupal-1783196-40.patch queued for re-testing.

#42: drupal-1783196-40.patch queued for re-testing.

#42: drupal-1783196-40.patch queued for re-testing.

+++ b/core/modules/views/views.moduleundefined
@@ -1375,7 +1407,7 @@ function views_get_applicable_views($type) {
-      $plugin = views_get_plugin_definition('display', $display[$id]['display_plugin']);
+      $plugin = drupal_container()->get('plugin.manager.views.display')->getDefinition('display', $display[$id]['display_plugin']);

This has been the reason why it didn't worked.

#50: drupal-1783196-50.patch queued for re-testing.

#50: drupal-1783196-50.patch queued for re-testing.

A rerole against current core.

Once #1836204: Test failure in Drupal\system\Tests\Menu\BreadcrumbTest is in, this should pass.

Sorry for the noise but changing title because components don't show up on the dashboard and I always forget what this issue is about.

Since #347988: Move $user->data into own table is not going in smoothly, I think we should copy our dependency into this patch or break out the dependency from the user->data patch.

#56: drupal-1783196-56.patch queued for re-testing.

So I talked to @dawehner about this issue (I had no idea why it would be blocking #1821844: Aggregator views integration) and apparently this patch also includes changes that are necessary to allow new base tables to be added without having to declare their own custom wizards. (?) Tagging for a summary. Can we clarify the situation, and either re-scope the issue or split it into two patches?

#56: drupal-1783196-56.patch queued for re-testing.

Rerolled the patch against latest HEAD.

Yep, this looks good and has done for ages :) can we please get this in!

I'd suggest updating the title and summary to explain the patch.

Yeah, that would've been nice. Had NO idea what this patch was about, but luckily Tim was online at Stupid O'Clock, and walked me through. Also I'm not sure how this patch is "major"... seems like pretty straight-forward refactoring.

Committed and pushed to 8.x. Here's the gist of my review, including one "normal" follow-up task.

+++ b/core/modules/comment/comment.views.incundefined
@@ -22,6 +22,7 @@ function comment_views_data() {
   $data['comment']['table']['entity type'] = 'comment';
+  $data['comment']['table']['wizard_id'] = 'comment';

+++ b/core/modules/file/file.views.incundefined
@@ -27,6 +27,7 @@ function file_views_data() {
   $data['file_managed']['table']['entity type'] = 'file';
+  $data['file_managed']['table']['wizard_id'] = 'file_managed';

+++ b/core/modules/node/node.views.incundefined
@@ -422,6 +423,8 @@ function node_views_data() {
   $data['node_revision']['table']['entity type'] = 'node';
...
+  $data['node_revision']['table']['wizard_id'] = 'node_revision';

+++ b/core/modules/taxonomy/taxonomy.views.incundefined
@@ -105,6 +105,8 @@ function taxonomy_views_data() {
   $data['taxonomy_term_data']['table']['entity type'] = 'taxonomy_term';
+  $data['taxonomy_term_data']['table']['wizard_id'] = 'taxonomy_term';

+++ b/core/modules/user/user.views.incundefined
@@ -24,6 +24,7 @@ function user_views_data() {
   $data['users']['table']['entity type'] = 'user';
+  $data['users']['table']['wizard_id'] = 'user';

The first thing I noticed was that these are a bit inconsistent, in that in most entity type and wizard_id match but in others they don't. Basically this is for disambiguation; in the case of something like "node" it exposes two different forms of its data, so "node" vs. "node_revision" allows for different settings.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.phpundefined
@@ -224,7 +224,7 @@ public function addDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
-    $plugin = views_get_plugin_definition('display', $plugin_id);
+    $plugin = drupal_container()->get('plugin.manager.views.display')->getDefinition($plugin_id);

I generally really dislike this business of dropping wrapper functions which are discoverable, and show up in my IDE, are findable on api.drupal.org, etc. in favour of making developers call directly into the guts of the magical mystery meat object that is drupal_container(). OTOH, I can't argue that this isn't consistent with what we do elsewhere, so is not a reason to hold up the patch.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.phpundefined
--- /dev/null
+++ b/core/modules/views/lib/Drupal/views/Plugin/Type/DefaultWizardDeriver.phpundefined

There wasn't really enough in here for me to get a sense of what's going on without Tim's help, but I think that's more to do with my unfamiliarity with Views than a specific docs problem here. This looks pretty standard with other plugin patches I've committed.

For the curious, though, what is essentially being done in this patch is we're swapping out the custom Views plugin management stuff for this with the more standard one provided by core.

Also, if you're curious what one of these wizardy things looks like, check core/modules/node/lib/Drupal/node/Plugin/views/wizard/Node.php. You won't figure it out from the patch because it's not touching the "consumer" parts (which is nice, that the API for wizards doesn't really change).

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/wizard/Standard.phpundefined
@@ -0,0 +1,20 @@
+class Standard extends WizardPluginBase {

This was the one spot where I definitely raised an eyebrow, since a class called "Standard" doesn't make any sense, and violates coding standards. However, I was informed by Tim that other areas in Views (arguments, filters, etc.) also do this "Standard.php" class name. So for now, I think it makes sense to keep this consistent, but let's get a follow-up filed to make Views conform to the OO naming guidelines.

Thanks, Tim!

@webchick, the patch was major because it blocks the hook_views_data() integrations somehow for stuff that didn't already have a wizard. Or something. That's part of what I kept begging people to explain in the issue.

This was the one spot where I definitely raised an eyebrow, since a class called "Standard" doesn't make any sense, and violates coding standards. However, I was informed by Tim that other areas in Views (arguments, filters, etc.) also do this "Standard.php" class name. So for now, I think it makes sense to keep this consistent, but let's get a follow-up filed to make Views conform to the OO naming guidelines.

That's not so much a followup issue as a followup release cycle phase, and on our roadmap. :)

The patch removes a lot of calls to views_get_plugin(), but doesn't remove the function itself, is that intended ? (just wondering)

Oh, good point, and we could also maybe convert views_get_plugin_definition?

The code itself

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -1215,7 +1215,7 @@ public function optionsSummary(&$categories, &$options) {
@@ -1260,7 +1260,7 @@ public function optionsSummary(&$categories, &$options) {

He we could reuse the container but i agree this would make it more difficult if we get either the plugin managers or the container injected.

I did a fast grep and there hasn't been any other kind of usage of this function anymore, so beside of the testbot failure and the previous comment.

Reupload the patch.

We really ought to be injecting the actual dependencies, but this at least eliminates the custom wrapper and Tim and Daniel tell me that injecting actual dependencies is a later step. So, +1 on small cleanups.

Injecting dependencies will be an interesting exercise.
- For instance, 1st hunks in the patch are methods in DisplayPluginBase. How would we inject something from the DIC into a plugin instance ? Especially if each plugin class doesn't need the same DIC entries ?
- How would we inject something from the DIC into a ConfigEntity object ?

(having similar cases with Field API plugin managers being read directly from the drupal_container(), in places where I'm not sure how injection could take place)

So what views does is to have a configEntity and a runtime object (viewExecutable) which uses the information stored on the configEntity to instanciate all the plugins and then inject itself into all this plugins, so you end up with $this->view on all views plugins. So the outside world actually never talks directly with the configEntity but just the executable.

After some thinking it seems to be that injecting the dependencies might be helpful, though we will always have to add the container as well (as long we agree we always want to avoid drupal_container()), because you never know which services are used for which subplugin (which might come from contrib).

we will always have to add the container as well, because you never know which services are used for which subplugin (which might come from contrib.

I'm wondering whether we could have a ContainerAwarePluginFactory, with a way for plugin classes to tell it which DIC entries they need.
Would mean a static method, or more entries in the annotations.

OK, sorry for polluting this issue - #82 is RTBC still :-)

Shameless plug: regarding #83-#86 - opened #1863816: Allow plugins to have services injected
@Crell, your feedback would be much appreciated over there :-)

Looks fine as an interim step. Committed/pushed to 8.x.