Provide the plugin_id to support views metadata integration

adding tag for d8mi since in #1 @dawehner says

This patch is required to have a proper translation support for views.

#9: drupal-1810480-9.patch queued for re-testing.

Trying a re-roll.

But #1648930: Introduce configuration schema and use for translation :( Totally thinking of a different issue. Proceed.

update

Yeah, I was going to say... :)

This patch looks good to me, there are a couple of places that are wrapping '' around strings. Do we need to do this? generally in yaml you don't need to for single word blocks. Do we have a standard? or are those the only ones left unchanged?

Fair enough, I like this in that case.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestBase.phpundefined
@@ -60,6 +60,35 @@ protected function enableViewsTestModule() {
+      $this->assert(TRUE, $message ? $message : t('Value @first is equal to value @second.', array('@first' => var_export($first, TRUE), '@second' => var_export($second, TRUE))), $group);
...
+      $this->assert(FALSE, $message ? $message : t('The first value differs from the second by @diff.', array('@diff' => var_export($diff, TRUE))));

Should be format_string() rather than t().

diff --git a/core/modules/views/lib/Drupal/views/ViewExecutable.php b/core/modules/views/lib/Drupal/views/ViewExecutable.php
@@ -2061,15 +2061,18 @@ public function addItem($display_id, $type, $table, $field, $options = array(),
     // If the desired type is not found, use the original value directly.
     $handler_type = !empty($types[$type]['type']) ? $types[$type]['type'] : $type;
 
-    // @todo This variable is never used.
-    $handler = views_get_handler($table, $field, $handler_type);
-
     $fields[$id] = array(
       'id' => $id,
       'table' => $table,
       'field' => $field,
     ) + $options;
 
+    // Load the plugin ID if available.
+    $data = views_fetch_data($table);
+    if (isset($data[$field][$handler_type]['id'])) {
+      $fields[$id]['plugin_id'] = $data[$field][$handler_type]['id'];
+    }

For the sake of reviewers, here's the actual change.

diff --git a/core/modules/views/tests/views_test_config/test_views/views.view.test_views_move_to_field.yml b/core/modules/views/tests/views_test_config/test_views/views.view.test_views_move_to_field.yml
deleted file mode 100644
index 22642b5..0000000

diff --git a/core/modules/views/tests/views_test_config/test_views/views.view.test_views_move_to_handler.yml b/core/modules/views/tests/views_test_config/test_views/views.view.test_views_move_to_handler.yml
deleted file mode 100644
index d66c95d..0000000

diff --git a/core/modules/views/tests/views_test_config/test_views/views.view.test_views_move_to_table.yml b/core/modules/views/tests/views_test_config/test_views/views.view.test_views_move_to_table.yml
deleted file mode 100644
index 956a7f0..0000000

I think removing these is out of scope? Can we file a separate issue to kill them if appropriate?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestBase.phpundefined
@@ -60,6 +60,35 @@ protected function enableViewsTestModule() {
   /**
+   * Asserts that two arrays are the same, but not checking the order of it.
+   *
+   * @param array $first
+   *   The first value to check.
+   * @param array $second
+   *   The second value to check.
+   * @param string $message
+   *   (optional) A message to display with the assertion. Do not translate
+   *   messages: use format_string() to embed variables in the message text, not
+   *   t(). If left blank, a default message will be displayed.
+   * @param string $group
+   *   (optional) The group this message is in, which is displayed in a column
+   *   in test output. Use 'Debug' to indicate this is debugging output. Do not
+   *   translate this string. Defaults to 'Other'; most tests do not override
+   *   this default.
+   * @return bool
+   *   TRUE on pass, FALSE on fail.
+   */
+  protected function assertArrayEqual(array $first, array $second, $message = '', $group = 'Other') {
+    $diff = array_diff($first, $second);
+    if (empty($diff)) {
+      $this->assert(TRUE, $message ? $message : format_string('Value @first is equal to value @second.', array('@first' => var_export($first, TRUE), '@second' => var_export($second, TRUE))), $group);
+    }
+    else {
+      $this->assert(FALSE, $message ? $message : format_string('The first value differs from the second by @diff.', array('@diff' => var_export($diff, TRUE))));
+    }

This might be useful for other things, could put directly in Simpletest maybe?

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -2061,15 +2061,18 @@ public function addItem($display_id, $type, $table, $field, $options = array(),
+    // Load the plugin ID if available.
+    $data = views_fetch_data($table);
+    if (isset($data[$field][$handler_type]['id'])) {
+      $fields[$id]['plugin_id'] = $data[$field][$handler_type]['id'];
+    }

How was the plugin ID found before? Doesn't look this this patch removes that code?

hmm OK, is there a follow-up for the hook_views_data() bit?

OK one more question then.

Something like
+ plugin_id: node_created_month

If a module changes hook_views_data() and their views plugin to a different name, then currently can an existing view survive this since that's inferred rather than explicity? With the patch wouldn't every view need to be updated to point to the new plugin name?

This might be useful for other things, could put directly in Simpletest maybe?

The Views tests also have a method like that for objects IIRC.

Regarding changing a plugin ID: IMO, if a module changes its data tables or plugin IDs or whatever, it's that module's responsibility to provide an upgrade path with that. I think it'd be a matter of loading all saved views, checking them for the old plugin, and then updating them and re-saving. Pretty simple. However, we could either provide API for it or document how to do it. Loosely related issues:

• #1398040: Detect if default configuration of a module has been changed, and allow to restore to the original
• #1620140: Allow synchronizing config entities from default config when modules are updated
• #1515312: Add snapshots of last loaded config for tracking whether config has changed

I filed a specific followup to discuss this further: #1874394: Provide an API for upgrading configuration

Ok sounds like the follow-ups in #29 should address catch's concerns. If I'm wrong, please feel free to roll this back.

Committed and pushed to 8.x. Thanks!

Nevermind. :P This broke HEAD because it references the non-existent views_fetch_data() function.

Reverted.

Yep, happy with this now, as it uses our shiny new views_data service and not views_fetch_data.

#32: drupal-1810480-32.patch queued for re-testing.

+++ b/core/modules/comment/tests/modules/comment_test_views/test_views/views.view.test_comment_rss.ymlundefined
@@ -61,4 +62,3 @@ human_name: test_comment_rss
-uuid: 9b1b1e58-d41b-468a-9d04-4e6bde742c29

This might not be allowed, but I don't mind. Having an issue just for that seems overkill.

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_revision_vid.ymlundefined
@@ -9,26 +9,31 @@ display:
-          id: nid:

Same, here, but still ok :)

+++ b/core/modules/system/lib/Drupal/system/Plugin/views/row/EntityRow.phpundefined
@@ -117,7 +117,7 @@ public function pre_render($result) {
+      // Get all entities which will be used to render in rows.c

I think this one IS a mistake?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -332,12 +339,13 @@ protected function displayMethodTests() {
+    $this->assertArrayEqual($item2, $expected_item);

Awesome

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1989,9 +1989,6 @@ public function addItem($display_id, $type, $table, $field, $options = array(),
-    // @todo This variable is never used.

We might not want to bother removing this if we want #1825896: Add module owner to plugin data on handlers in?

Otherwise, let's please try and get this in asap!

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_revision_vid.yml
@@ -9,26 +9,31 @@ display:
       fields:
         vid:
           id: vid
           table: node_revision
           field: vid
+          plugin_id: standard
         nid_1:
           id: nid_1
           table: node_revision
           field: nid
+          plugin_id: standard

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_status_extra.yml
@@ -111,12 +112,14 @@ display:
       sorts:
         nid:
           id: nid
           table: node
           field: nid
           order: ASC
+          plugin_id: standard

+++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_groupwise_term.yml
@@ -51,6 +54,7 @@ display:
           id: tid
           order: DESC
           table: taxonomy_term_data
+          plugin_id: standard

Is "standard" really a plugin ID...? (?)

+++ b/core/modules/user/tests/modules/user_test_views/test_views/views.view.test_view_argument_validate_user.yml
@@ -17,6 +17,7 @@ display:
           table: views
           validate:
             type: user
+          plugin_id: 'null'

null?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestBase.php
@@ -60,6 +60,35 @@ protected function enableViewsTestModule() {
   /**
+   * Asserts that two arrays are the same, but not checking the order of it.
...
+  protected function assertArrayEqual(array $first, array $second, $message = '', $group = 'Other') {

I guess you meant "ignoring the array keys"?

assertEqual() ignores the order already.

The comment is fair enough I think, but I don't think we should be discussing actual plugin IDs in this issue. And yes, we have one called standard.. I know :)

#47: drupal-1810480-47.patch queued for re-testing.

RTBC if it still is green.

Committed and pushed to 8.x. Thanks!