Let's not continue to babysit contrib.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
tim.plunkett’s picture

FileSize
12.64 KB

Just a quick test.

merlinofchaos’s picture

This isn't really babysitting; I think it's a very important feature.

While its primary purpose is version to version upgrading, sometimes tables and fields get renamed/moved, and this is the *only* way to update exported Views to keep them working. Without this, table/field names are basically locked forever.

I would really suggest not removing this.

Status: Needs review » Needs work

The last submitted patch, views-1808810-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
14.78 KB

With CMI we'll actually be able to reliably update this kind of stuff, since we don't have to worry about if its in the DB or in files spread around the file system, everything is central.
And since we'll be in core, modules renaming their tables/fields will handle it themselves.

Cleaned up more, I think this is fine.

tim.plunkett’s picture

These parts are directly related but not required, so I'm calling them out here. I don't think it's worth a separate follow-up.

+++ b/lib/Drupal/views/Tests/ModuleTest.phpundefined
@@ -115,29 +102,9 @@ function testviews_get_handler() {
-    $handler = views_get_handler('views_test_data', 'job', 'filter', 'string');
-    $this->assertEqual('Drupal\\views\\Plugin\\views\\filter\\String', get_class($handler));
+    $handler = views_get_handler('views_test_data', 'job', 'filter', 'standard');
+    $this->assertEqual('Drupal\\views\\Plugin\\views\\filter\\Standard', get_class($handler));

Somewhere along the way, 'string' became the original value of this, so if override was taken out this still passed.

+++ b/modules/node.views.incundefined
@@ -464,6 +463,7 @@ function node_views_data() {
     'field' => array(
+      'id' => 'standard',
       'click sortable' => TRUE,

@@ -495,6 +495,7 @@ function node_views_data() {
     'field' => array(
+      'id' => 'standard',
       'click sortable' => TRUE,

+++ b/views.moduleundefined
@@ -1291,57 +1291,11 @@ function views_include_handlers($reset = FALSE) {
-    // Set up a default handler, if both handler and id is not specified.
-    if (empty($data[$field][$type]['handler']) && empty($data[$field][$type]['id'])) {
-      $data[$field][$type]['id'] = 'standard';

These were the only cases that needed this code.

+++ b/views.moduleundefined
@@ -1291,57 +1291,11 @@ function views_include_handlers($reset = FALSE) {
-    if ($override) {
-      $data[$field][$type]['override handler'] = $override;

@@ -1357,8 +1311,7 @@ function views_get_handler($table, $field, $type, $override = NULL) {
-    $plugin_id = !empty($definition['override handler']) ? $definition['override handler'] : $definition['id'];
+    $plugin_id = $override ?: $definition['id'];

$override is either a string or NULL, no need to assign it above and check it again

+++ b/views.moduleundefined
@@ -1376,7 +1329,7 @@ function views_get_handler($table, $field, $type, $override = NULL) {
-  return views_get_plugin($type, 'broken');
+  return $manager->createInstance('broken');

yay less procedural code

+++ b/views.moduleundefined
@@ -1718,28 +1671,10 @@ function views_view_is_disabled($view) {
-    $view->update();
-    $view = $view->getExecutable();
-    // @figure out whether it makes sense to clone this here.
-    return $view->cloneView();
+    return $view->getExecutable();

While removing update() I realized we still have this old code.

dawehner’s picture

With CMI we'll actually be able to reliably update this kind of stuff, since we don't have to worry about if its in the DB or in files spread around the file system, everything is central.
And since we'll be in core, modules renaming their tables/fields will handle it themselves.

Well in d7 this was actually mostly used for the upgrade path and for that we will provide a better way.
Do you have any links regarding the automatic changing of config, is there anything concrete planned yet?

+++ b/lib/Drupal/views/Tests/ModuleTest.phpundefined
@@ -115,29 +102,9 @@ function testviews_get_handler() {
+    $handler = views_get_handler('views_test_data', 'job', 'filter', 'standard');
+    $this->assertEqual('Drupal\\views\\Plugin\\views\\filter\\Standard', get_class($handler));

Currently it is a string, so this makes sense: 'filter' => array(
'id' => 'string',
),

+++ b/views.moduleundefined
@@ -1376,7 +1329,7 @@ function views_get_handler($table, $field, $type, $override = NULL) {
   debug(t("Missing handler: @table @field @type", array('@table' => $table, '@field' => $field, '@type' => $type)));

Does this actually makes sense to be runned via t(), as this is basically something which will never be shown to the enduser. I know this is out of scope, but i'm just wondering.

+++ b/views.moduleundefined
@@ -1376,7 +1329,7 @@ function views_get_handler($table, $field, $type, $override = NULL) {
-  return views_get_plugin($type, 'broken');
+  return $manager->createInstance('broken');

Especially i guess this is actually broken code?

tim.plunkett’s picture

I talked to heyrocker, we can simply have a hook_update_N() go through each views.view.*.yml file and update everything :) Yay!

Talked about the other points in IRC. RTBC?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Well, i would probably suggest to write some helper api methods then, because we want to make it as easy as possible.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Fixed

http://drupalcode.org/project/views.git/commit/6778f04

I don't know how we would write a generic helper, but when we start to talk about upgrade path I'm sure we'll come up with one.

xjm’s picture

Status: Fixed » Needs review
FileSize
4.23 KB

The $move parameter was still in views_fetch_data(). Attached removes that and updates the docs.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed!

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Automatically closed -- issue fixed for 2 weeks with no activity.