Views was merged into core on Oct. 22, 2012! You can help out on the core views issues and the remaining tasks.

Problem/Motivation

The Views module is installed on 70% of all Drupal sites, and will be essential to the success of Drupal 8. The Views in Drupal Core Initiative has already ported Views to Drupal 8 and done significant cleanup and refactoring.

The next step is to add Views to core. Adding Views to core will have numerous advantages:

  • Consistency: Many disparate, legacy solutions are currently used for data listings in core modules. Converting these listings to Views will both improve the Drupal developer experience and make it easier for site builders to customize their sites.
  • Learnability: First-time users of Drupal often don't realize what is possible with contributed modules. Having Views in core will mean that new site builders can more quickly understand Drupal's capabilities out-of-the-box.
  • Release cycle: The stability of Views has in the past been an indicator of when the community considers a release of Drupal "ready". Drupal 7 usage did not start to increase until a development version of Views was available for D7, and it did not pass Drupal 6 usage until Views was stable.
  • Contributor experience: Hundreds of contributed modules rely on the Views API, so these modules are blocked on Views for each release.
  • Stability: If Views is in core, changes that cause Views regressions will be core release blockers, and Views bugs will be treated as core bugs.

Proposed resolution

Update, Oct. 22: Views has been merged into core!

Note that this issue is not the end goal of the VDC initiative. It is only an important milestone. Since less than two months remain until feature freeze, it is important to distinguish what we must do to add Views as a core feature from what we should do to integrate and improve Views. (See the VDC roadmap for full details for what tasks are targeted in each phase of the Drupal 8 release cycle.)

Details

 

  1. Add the 8.x-3.x branch of the Views module to Drupal core. Done

  2. Make views.module and views_ui.module optional core modules. Done

  3. Move Views' module-specific integrations into their respective core modules. Done (#3)

  4. Merge the VDC sandbox into D8 core. Done

  5. #1807020: Add Views and Views UI to the standard install profile (blocked on #1708692: Bundle services aren't available in the request that enables the module)

  6. #1806334: Replace the node listing at /node with a view (somewhat blocked by #1807020: Add Views and Views UI to the standard install profile.)

  7. #1864980: [meta] Figure out how to integrate Views into core

(Note: This issue is not for discussion about whether Views should be added to core; see Dries' blog post on Views in core for that decision.)

Remaining tasks

Sub-issues

Core gates

According to Drupal core policy, core features must meet a set of minimum requirements that are defined by the Drupal core gates. We need to test Views in terms of each of these gates.

  1. Testing

    http://drupal.org/core-gates#testing

    Views already includes an automated test suite, but the coverage is not complete.

    Before feature freeze we will ensure that Views has basic test coverage for CRUD operations, storage, all plugins and handlers types, all default views, and basic view creation through the UI. We will also manually test Views in the VDC sandbox once the proposed resolution is complete there.

    Later, we will expand the Views test coverage to more thoroughly test complex Views, all UI features, etc.

  2. Documentation and coding standards

    http://drupal.org/core-gates#documentation

    The API documentation was rewritten for Views 3 at DrupalCon Denver.

    Before feature freeze we will ensure that there is at least minimal documentation for all hooks, plugins, and handlers. The documentation gate will be applied to all new or updated code in Views patches.

    Later, we will ensure that all files, classes, functions, and methods have documentation that meets core standards, and we will apply all core coding standards (including camelCase method names) to the Views codebase.

  3. Accessibility

    http://drupal.org/core-gates#accessibility

    As a contributed module, Views has not yet been thoroughly tested for its accessibility compliance.

    Before feature freeze we will test the core module for each point of the core accessibility gate and identify any critical issues discovered during testing:

    Later, we will plan full accessibility testing and address non-critical accessibility issues. Meta issue: #1802678: [META] Views: accessibility review

  4. User experience

    http://drupal.org/core-gates#usability

    Views 8.x-3.x has the same user interface as Views 7.x-3.x, which has already undergone a usability review and been improved based on user testing. However, we know serious issues remain, especially in the more advanced full editing interface. (Few problems were found with the wizard views setup).

    Before feature freeze we will provide screenshots of all parts of the Views UI and attempt to identify critical usability issues through testing.

    Later, we will seek a full usability review and resolve critical usability issues.

    Related issues:

  5. Performance

    http://drupal.org/core-gates#performance

    Views includes more sophisticated caching and cache invalidation functionality than most core listings provide, so it can actually improve the performance of some listings. However, a poorly designed or optimized View can cause severe performance issues. Our goals are to ensure that core's default listings are performant, and to provide guidance for the creation of custom Views.

    Before feature freeze we will do full performance testing on our first core View implementation (the main /node listing) and compare its performance with the current node listing to ensure there are not severe regressions. Issue: #1806370: Do full performance testing on the main node listing before and after it is converted to a view

    Later, we will evaluate the frontend performance of the administrative UI, test the performance of each new core view listing as it's added, and explore ways to inform the user when a view is badly optimized.

User interface changes

API changes

  • views.module and views_ui.module will be added as optional core modules.
  • The standard profile will include both these modules.
  • TBD.

Followup tasks

The following issues and tasks should be addressed after feature freeze.

Before code freeze (Apr. 1 2013)

  1. #1790610: [META] Ensure the Configuration and ConfigEntity systems fully support Views CRUD and status operations
  2. Clean up the base test class.
  3. Convert method names to core standards.
  4. Convert remaining core listings to Views.
  5. Finish converting the query plugin to load entities after queries are executed.
  6. Use core API and UI for modal dialogs if/when it is available. (#1667742: Add abstracted dialog to core (resolves accessibility bug), #1175830: Update to jQuery UI 1.10.2).
  7. Explore further integrations with other new core features (e.g., the Field and Entity APIs, Blocks and Layouts, other core query builders, etc.), potentially including:
    • #1740492: Implement a default entity views data handler
    • Ensure Views properly implements core multilingual functionality. (Dependent on the Multilingual Initiative.)
    • Integrate Views' display architecture with new Drupal 8 blocks and layouts functionality. (Dependent on the Blocks and Layouts Initiative.)
    • Test the deployment workflow for Views using the configuration system. (Dependent on the Configuration Management Initiative.)
  8. Additional cleanup and refactoring.

Before the first Drupal 8 release candidate

  1. Provide complete upgrade path from Views 7.x-3.x to core Views.
  2. Provide full functional and unit test coverage.
  3. Provide full API documentation.
  4. Move the old advanced help documentation onto Drupal.org and update it for the new architecture.#1892470: Update Views documentation for D8
  5. Clean up and optimize the administrative UI, form structure, and JavaScript.
  6. Resolve any non-critical accessibility issues.
  7. Consider mobile-friendly administration for Views (evaluate both the configurable listing controller UI and the main Views administrative UI). (Dependent on the Mobile initiative.)
  8. Identify and address any performance concerns.
  9. Explore ways to provide user feedback on specific views that are not well-optimized.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +VDC

Tagging.

xjm’s picture

larowlan’s picture

Issue tags: +Accessibility

Tagging for a11y team

larowlan’s picture

Issue summary: View changes

Updated issue summary.

nod_’s picture

nod_’s picture

Issue tags: +Accessibility

sorry about that. restoring tag.

gdd’s picture

HEY GUESS WHAT GUYS IM ADDING A TAG! MAYBE I WILL AT THE SAME TIME DELETE YOURS! HEY SORRY IF SO YOU CAN ADD IT BACK! KTHXBYE

gdd’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: +Accessibility

Is it safe yet?

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

FileSize
2.5 MB

1. Add the 8.x-3.x branch of the Views module to Drupal core.
Done (as of #0)

2. Make views.module and views_ui.module optional core modules.
Done (as of #0)

3. Move Views' module-specific integrations into their respective core modules.
Done (as of this patch)

4. Convert the main node listing at /node to use Views.
Not yet finished

5. Include Views and Views UI in the standard install profile.
Blocked on #1786990: [Module]Bundle is registered too late in WebTestBase::setUp()

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Clarify what must be done for each gate.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

FileSize
2.48 MB

Rerolled to keep up with 8.x, and also to add a couple commits from Views to unblock the /node listing page.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Issue summary: View changes

remove non-essential issue about /theme vs /templates

xjm’s picture

Issue summary: View changes

Updated issue summary.

pounard’s picture

I don't want to be a party pooper, but does this patch really adds drush integration? It would a first in core, and that goes against core being neutral regarding contrib.

dawehner’s picture

Good point: Created a task for that: #1809818: Move drush integration from views into drush

damiankloip’s picture

This is not the prosoped finished patch by any means, but yes it's good to have an issue for that.

pounard’s picture

@#12 Of course, but it still good for you to have that kind of feedback even on an unfinished patch, external pairs of eyes might see details you could have missed.

pounard’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Thanks @pounard, good call.

As a meta issue, this is probably the ideal place for folks to point out things like #1809818: Move drush integration from views into drush. Some might be done before our first merge, others might be targeted for later, but the sooner we file issues the better.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Is there an issue for this stuff? Views should be able to make the assumption that people use #attached properly or have to fix their code.

+  /**
+   * Start caching javascript, css and other out of band info.
+   *
+   * This takes a snapshot of the current system state so that we don't
+   * duplicate it. Later on, when gather_headers() is run, this information
+   * will be removed so that we don't hold onto it.
+   */
+  function cache_start() {
+    $this->storage['head'] = drupal_add_html_head();
+    $this->storage['css'] = drupal_add_css();
+    $this->storage['js'] = drupal_add_js();
+  }
dawehner’s picture

Thanks for the idea. It seems to be a larger effort to fix that: #1811828: Use #attached to find the css/js of a view

dawehner’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

lpalgarvio’s picture

some notices...

Admin Views
turns some admin pages to views (req. views bulk operations)

related:
#1537922: D8UX: Proposal for the Content Manager
#1510532: [META] Implement the new create content page design

Views System
adds support for listing system objects, like modules

related:
#538904: D8UX: Redesign Modules Page
#1765576: Redesign Permissions Page

lpalgarvio’s picture

Issue summary: View changes

Updated issue summary.

kenorb’s picture

2.5MB patch? lol:)

xjm’s picture

@kenorb, Yep, it's a lot of code. :) The patch is just to demonstrate that it passes the automated test suite; we have a sandbox to merge into core, though, since no normal person can review that whole patch:
http://drupal.org/sandbox/tim.plunkett/1799554

xjm’s picture

Issue summary: View changes

Updated issue summary.

Dries’s picture

I just talked to tim.plunkett. I asked him if merging this to core sooner rather than later would help the VDC team's productivity in getting the rest of what needs to be done finished. He said yes, so given that, I'd like to merge this into core early next week.

I told Tim that I'd be comfortable merging VDC without /node being Views-enabled. This means it is no longer blocked on #1706064: Compile the Injection Container to disk or any other issue.

I also believe that the accessibility and usability gate goals laid out here are important, but I don't believe they should hold up the initial patch, and will in fact be easier to address once this is in core. It will block the release, not the initial commit. It is a matter of deferring the gates, not abandoning them. In fact, I want to encourage the accessibility and usability sprints at BADCamp to focus on this.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC, per #20, to get some eyeballs on this.

falcon03’s picture

Hi Dries,

well, I quite agree with you: committing views to core could speed up also accessibility testing.

However, I have to say that if we commit views to core we should mark all the accessibility issues as "critical", since views in core will be one of the most important changes for drupal 8.. and, as a blind user, I would be really frustrated knowing that all Views Accessibility issues weren't solved (as I said in other posts, there is the strong idea among italian blind users that drupal is one of the most, if not the most at all, accessible cms out-of-the-box).

BTW, I am a novice in drupal core development, so correct me if I am doing some mistakes...

xjm’s picture

@falcon03, Yes, we will definitely be moving the accessibility critical issue to the core queue immediately when Views is merged in: #1802678: [META] Views: accessibility review. Anything we discover during testing that breaks basic accessibility will be a Drupal 8 release blocker. Thank you for the feedback!

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

I updated the summary to reflect @Dries' post in #20.

tim.plunkett’s picture

FileSize
2.48 MB

Here's an updated patch, for the last 9 days of changes.

catch’s picture

Alright I tried to review this a bit. I only looked at a few files, and only bits of those files. Overall you can see how much work has gone into the 8.x port of Views. There's a few things that make me uncomfortable committing as-is, this isn't necessarily an exclusive list since I only just started reviewing.

1. Is it really necessary to commit all the support for every core module (aggregator, contact, search) in the initial patch? Just skimming through I found several bugs and other issues, these could easily be handled by people not on the VDC team and would make good follow-up patches (add views support for $module) once views module itself and one implementation is in. Most of the work has of course been done already but I don't have a massive amount of confidence that once the code is in core people will go through to review it. It's also going to mean that any patch with a schema change or similar is either going to have to update the views support, or if it's not tested run the risk of unintentionally breaking it. If we have a bunch of tasks with a tracking meta-issue then it spreads all that work around a bit more (both in terms of people and time).

Since aggregator was at the top I started there - although I only reviewed less than 25% of the aggregator integration, but still that's enough that'd I'd rather it and several other modules were split out of the patch. We could do something like include node + field_sql_storage modules and put the rest into sepearate issues. That would also make this patch sufficiently smaller that my browser wouldn't be almost crashing right now.

2. Same thing goes for views like the default archive view - not sure we actually need these in core at all, if we do they could be in the standard profile.

3. There's some switching on the core database supported types that is absolutely not allowed in core (it's occasionally OK to have engine specific code, but not to limit support to the three that core ships with. That should either be dropped or a critical issue will need to be added to move it to db drivers.

This isn't an exclusive list, I ran out of steam reviewing before I'd got anywhere near the end of views module itself.

Very spotty/terse dreditor review follows. A lot of this is minor stuff but it appears all over the place, which goes back to #1 above.

index 0000000..a610253
--- /dev/null

--- /dev/null
+++ b/core/modules/aggregator/aggregator.views.incundefined

:( #293318: Convert Aggregator feeds into entities.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/CategoryCid.phpundefined
@@ -0,0 +1,41 @@
+    $query = db_select('aggregator_category', 'c');

No reason for db_select() here. Also at some point the db connection should be injected.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Fid.phpundefined
@@ -0,0 +1,42 @@
+    $result = db_query("SELECT f.title FROM {aggregator_feed} f WHERE f.fid IN (:fids)", array(':fids' => $this->value));
+    $query = db_select('aggregator_feed', 'f');

This is the exactly the same query twice, $result just gets overwritten by the second one.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -0,0 +1,270 @@
+    // It is very important to call the parent function here:
+    parent::optionsSummary($categories, $options);

Why?

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -0,0 +1,270 @@
+    // Maybe people don't have block module installed, so let's skip this.
+    if (db_table_exists('block')) {
+      db_update('block')
+        ->fields(array('delta' => $delta))
+        ->condition('delta', $old_delta)
+        ->execute();

module_exists()?

Also ouch, but I supposed this will all die with block plugins.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -0,0 +1,270 @@
+    if ($hashes != $old_hashes) {
+      variable_set('views_block_hashes', $hashes);

Should be state().

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -0,0 +1,270 @@
+
+  /**
+   * Save the block cache setting in the blocks table if this block allready
+   * exists in the blocks table. Dirty fix untill http://drupal.org/node/235673 gets in.

#235673: Changes to block caching mode not caught is in, it was even backport to 6.x by now.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
index 0000000..6e2b890
--- /dev/null

--- /dev/null
+++ b/core/modules/book/book.views.incundefined

We ought to be able to move towards killing this if #1801304: Add Entity reference field gets in. Its another set of integration which could go in after the initial patch.

+++ b/core/modules/comment/comment.views.incundefined
@@ -0,0 +1,580 @@
+ * Implements hook_views_data().
+ */

This probably ought to be able to go away with the entity property API no?

+++ b/core/modules/comment/comment.views.incundefined
@@ -0,0 +1,580 @@
+    'field' => array(
+      'id' => 'comment',
+      'click sortable' => TRUE,

Well maybe not. But presumably some of this information could be handled centrally by the entity system (or views on behalf of it since there's no entity module yet), or move this stuff to classes so that modules can extend it.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeLink.phpundefined
@@ -0,0 +1,57 @@
+    // Call comment.module's hook_link: comment_link($type, $node = NULL, $teaser = FALSE)
+    // Call node by reference so that something is changed here
+    comment_node_view($node, $this->options['teaser'] ? 'teaser' : 'full');
+    // question: should we run these through:    drupal_alter('link', $links, $node);
+    // might this have unexpected consequences if these hooks expect items in $node that we don't have?
+
+    // Only render the links, if they are defined.
+    return !empty($node->content['links']['comment']) ? drupal_render($node->content['links']['comment']) : '';

All these comments relate to Drupal 6, and not the actual code here.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.phpundefined
@@ -0,0 +1,124 @@
+    if ($nids) {
+      $query = db_select('node', 'n');
+      $query->addField('n', 'nid');
+      $query->innerJoin('comment', 'c', 'n.nid = c.nid');
+      $query->addExpression('COUNT(c.cid)', 'num_comments');
+      $query->leftJoin('history', 'h', 'h.nid = n.nid');
+      $query->condition('n.nid', $nids);
+      $query->where('c.changed > GREATEST(COALESCE(h.timestamp, :timestamp), :timestamp)', array(':timestamp' => NODE_NEW_LIMIT));
+      $query->condition('c.status', COMMENT_PUBLISHED);
+      $query->groupBy('n.nid');

Query isn't dynamic.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/row/View.phpundefined
@@ -0,0 +1,65 @@
+/**
+ * Plugin which performs a comment_view on the resulting object.
+ *
+ * @Plugin(
+ *   id = "comment",
+ *   module = "comment",
+ *   title = @Translation("Comment"),
+ *   help = @Translation("Display the comment with standard comment view."),
+ *   theme = "views_view_row_comment",
+ *   base = {"comment"},
+ *   entity_type = "comment",
+ *   type = "normal"
+ * )
+ */
+class View extends Entity {

Class names should be a bit more descriptive than this rather than completely relying on the namespace. I saw the class definition before the comments and my eyes nearly popped out.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/sort/Thread.phpundefined
@@ -0,0 +1,40 @@
+      $alias = $this->tableAlias . '_' . $this->realField . 'asc';

There was not asking if this is secure just above this code (which dreditor sadly didn't select).
Did this question ever get answered?

+++ b/core/modules/field/field.views.incundefined
@@ -0,0 +1,463 @@
+/**
+ * Implements hook_views_data_alter().
+ *
+ * Field modules can implement hook_field_views_data_views_data_alter() to
+ * alter the views data on a per field basis. This is weirdly named so as
+ * not to conflict with the drupal_alter('field_views_data') in
+ * field_views_data.
+ */
+function field_views_data_alter(&$data) {
+  foreach (field_info_fields() as $field) {
+    if ($field['storage']['type'] != 'field_sql_storage') {
+      continue;
+    }
+
+    $function = $field['module'] . '_field_views_data_views_data_alter';
+    if (function_exists($function)) {
+      $function($data, $field);
+    }
+  }
+}

This should all be in field_sql_storage module.

+++ b/core/modules/field/field.views.incundefined
@@ -0,0 +1,463 @@
+/**
+ * Returns the label of a certain field.
+ *
+ * Therefore it looks up in all bundles to find the most used instance.
+ */

It does what?

+++ b/core/modules/field/field.views.incundefined
@@ -0,0 +1,463 @@
+  $data = array();
+
+  $current_table = _field_sql_storage_tablename($field);
+  $revision_table = _field_sql_storage_revision_tablename($field);

This is all field_sql_storage stuff. Field module should know nothing about that module.

+++ b/core/modules/field/field.views.incundefined
@@ -0,0 +1,463 @@
+              $alias_title = t('@label (!name)', array('@label' => $label_name, '!name' => $field['field_name']));
+              // CCK used the first 10 characters of $label. Probably doesn't matter.

Ghosties.

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_default/Node.phpundefined
@@ -0,0 +1,40 @@
+  function get_argument() {
+    foreach (range(1, 3) as $i) {
+      $node = menu_get_object('node', $i);
+      if (!empty($node)) {
+        return $node->nid;
+      }
+    }
+
+    if (arg(0) == 'node' && is_numeric(arg(1))) {
+      return arg(1);

This is weird. It's also copy/pasted verbatim into a function in views module somewhere (see below).

+++ b/core/modules/views/config/views.settings.ymlundefined
index 0000000..f2b3a62
--- /dev/null

--- /dev/null
+++ b/core/modules/views/config/views.view.archive.ymlundefined

Why is this is in views module and not node module?

This also brings up another issue - if either module is switched off, we'll still be copying default views into config despite them not being relevant. That question makes my head hurt but could use a follow-up (as could adding these defaults at all).

+++ b/core/modules/views/config/views.view.archive.ymlundefined
index 0000000..8c1f2a7
--- /dev/null

--- /dev/null
+++ b/core/modules/views/config/views.view.backlinks.ymlundefined

Same with this and search module. I'd also be tempted to move these out of the patch and evaluate one by one - we'll have enough default views in core replacing core listings, I'm not sure the actual defaults that Views has happened to ship with historically make sense to include (maybe in the standard profile instead?).

+++ b/core/modules/views/config/views.view.taxonomy_term.ymlundefined
index 0000000..5d31e2f
--- /dev/null

--- /dev/null
+++ b/core/modules/views/config/views.view.tracker.ymlundefined

+++ b/core/modules/views/config/views.view.tracker.ymlundefined
+++ b/core/modules/views/config/views.view.tracker.ymlundefined
@@ -0,0 +1,150 @@

@@ -0,0 +1,150 @@
+disabled: true
+api_version: '3.0'
+module: node
+name: tracker
+description: 'Shows all new activity on system.'
+tag: default
+base_table: node

This isn't up-to-date with the core tracker schema which has some denormalization. Would be a critical performance regression if the tracker listing was updated to use it in its current state.

+++ b/core/modules/views/includes/ajax.incundefined
@@ -0,0 +1,373 @@
+/**
+ * Page callback for views user autocomplete
+ */

Why can't this use core user autocomplete?

+++ b/core/modules/views/includes/ajax.incundefined
@@ -0,0 +1,373 @@
+
+/**
+ * Page callback for views taxonomy autocomplete.
+ *
+ * @param $vid
+ *   The vocabulary id of the tags which should be returned.
+ *
+ * @param $tags_typed
+ *   The typed string of the user.
+ *
+ * @see taxonomy_autocomplete()
+ */

Same question for taxonomy term autocomplete.

+++ b/core/modules/views/includes/cache.incundefined
@@ -0,0 +1,159 @@
+/**
+ * Return data from the persistent views cache.
+ *
+ * This is just a convenience wrapper around cache_get().

Not any more...

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/HandlerBase.phpundefined
@@ -0,0 +1,930 @@
+  /**
+   * Creates cross-database SQL date formatting.
+   *
+   * @param string $format
+   *   A format string for the result, like 'Y-m-d H:i:s'.
+   *
+   * @return string
+   *   An appropriate SQL string for the DB type and field type.
+   */

This needs to be moved into the database drivers or removed, core can't hard-code database support like this and it's not necessary to support any core features that I know of.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/ArgumentPluginBase.phpundefined
@@ -0,0 +1,1121 @@
+  /**
+   * Process the summary arguments for display.
+   *
+   * For example, the validation plugin may want to alter an argument for use in
+   * the URL.
+   */

Trying to completely ignore code style, but all these methods are missing public and should be camel case.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/Date.phpundefined
@@ -0,0 +1,145 @@
+      foreach (range(1, 3) as $i) {
+        $node = menu_get_object('node', $i);
+        if (!empty($node)) {
+          continue;
+        }
+      }
+
+      if (arg(0) == 'node' && is_numeric(arg(1))) {
+        $node = node_load(arg(1));

I've seen this before. Also I don't understand why it's in Views module at all.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/Date.phpundefined
@@ -0,0 +1,145 @@
+      // ISO week number for date
+      case 'WEEK':
+        switch ($db_type) {

Again, we can't get away with this.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -0,0 +1,2233 @@
+  /**
+   * Set the display for this view and initialize the display handler.
+   */

One of the more serious performance issues with Views is that every display is loaded whenever one of them is viewed. Is there an issue looking at this? I once found over 60 displays on a single View, which was the fault of the people building the view but was also killing their site.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -0,0 +1,2233 @@
+  /**
+   * Get the first display that is accessible to the user.
+   *
+   * @param array|string $displays
+   *   Either a single display id or an array of display ids.
+   *
+   * @return string
+   *   The first accessible display id, at least default.
+   */

Seems from here that it might be possible to instantiate the displays during this access check instead of before?

I'm leaving this RTBC in the hope some other people will panic and actually review this, but I'm not comfortable with this going in until 1-3 are either justified or implemented.

bojanz’s picture

This probably ought to be able to go away with the entity property API no?

We have a plan for the new entity api, property api, and more automatic views integration for entities in #1792828: [META] Provide entity integration with Views.
That makes me agree we shouldn't just commit the core module integrations into core yet, because they might radically change (even in the next few weeks).

xjm’s picture

The VDC team is co-authoring a reply to @catch's points right now. :)

xjm’s picture

Thanks @catch; this is really good feedback. The following (addressing all points, I hope) was written partly by @dawehner and partly by myself, with input from @damiankloip and @tim.plunkett.

  1. Regarding:

    Trying to completely ignore code style, but all these methods are missing public and should be camel case.

    Cleaning up method and variable naming and public vs. protected is on the roadmap for during feature freeze and the fundamental objects got some love already. See:
    http://drupal.org/community-initiatives/drupal-core/vdc-roadmap

  2. Regarding:

    +++ b/core/modules/field/field.views.incundefined
    @@ -0,0 +1,463 @@
    +          	$alias_title = t('@label (!name)', array('@label' => $label_name, '!name' => $field['field_name']));
    +          	// CCK used the first 10 characters of $label. Probably doesn't matter.
    

    Ghosties.

    Thanks for mentioning. We will remove this on the next iteration of the patch.

  3. Regarding:

    +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeLink.phpundefined
    @@ -0,0 +1,57 @@
    +	// Call comment.module's hook_link: comment_link($type, $node = NULL, $teaser = FALSE)
    +	// Call node by reference so that something is changed here
    +	comment_node_view($node, $this->options['teaser'] ? 'teaser' : 'full');
    +	// question: should we run these through:	drupal_alter('link', $links, $node);
    +	// might this have unexpected consequences if these hooks expect items in $node that we don't have?
    +
    +	// Only render the links, if they are defined.
    +	return !empty($node->content['links']['comment']) ? drupal_render($node->content['links']['comment']) : '';
    

    All these comments relate to Drupal 6, and not the actual code here.

    Another thing we will clean up. (This can be probably be done during code freeze as it’s quite minor functionality. Filed: #1817638: Cleanup node/field/NodeLink).

  4. Regarding:

    +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.phpundefined
    @@ -0,0 +1,124 @@
    +	if ($nids) {
    +  	$query = db_select('node', 'n');
    +  	$query->addField('n', 'nid');
    +  	$query->innerJoin('comment', 'c', 'n.nid = c.nid');
    +  	$query->addExpression('COUNT(c.cid)', 'num_comments');
    +  	$query->leftJoin('history', 'h', 'h.nid = n.nid');
    +  	$query->condition('n.nid', $nids);
    +  	$query->where('c.changed > GREATEST(COALESCE(h.timestamp, :timestamp), :timestamp)', array(':timestamp' => NODE_NEW_LIMIT));
    +  	$query->condition('c.status', COMMENT_PUBLISHED);
    +  	$query->groupBy('n.nid');
    

    Query isn't dynamic.

    Here is an issue to do that: #1817608: replace the query in NodeNewComments with a static one

  5. Regarding:

    +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/row/View.phpundefined
    @@ -0,0 +1,65 @@
    +/**
    + * Plugin which performs a comment_view on the resulting object.
    + *
    + * @Plugin(
    + *   id = "comment",
    + *   module = "comment",
    + *   title = @Translation("Comment"),
    + *   help = @Translation("Display the comment with standard comment view."),
    + *   theme = "views_view_row_comment",
    + *   base = {"comment"},
    + *   entity_type = "comment",
    + *   type = "normal"
    + * )
    + */
    +class View extends Entity {
    

    Class names should be a bit more descriptive than this rather than completely relying on the namespace. I saw the class definition before the comments and my eyes nearly popped out.

    We totally agree but at the same time the class name shouldn’t contain redundant information. Issue: #1817554: Review and improve naming of entity specific row plugins.

  6. Regarding:

    +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/sort/Thread.phpundefined
    @@ -0,0 +1,40 @@
    +  	$alias = $this->tableAlias . '_' . $this->realField . 'asc';
    

    There was not asking if this is secure just above this code (which dreditor sadly didn't select).
    Did this question ever get answered?

    It probably should be wrapped in db_escape_table() for safety, though the values are coming from code and not user input.

  7. Regarding:

    +++ b/core/modules/field/field.views.incundefined
    @@ -0,0 +1,463 @@
    +/**
    + * Implements hook_views_data_alter().
    + *
    + * Field modules can implement hook_field_views_data_views_data_alter() to
    + * alter the views data on a per field basis. This is weirdly named so as
    + * not to conflict with the drupal_alter('field_views_data') in
    + * field_views_data.
    + */
    +function field_views_data_alter(&$data) {
    +  foreach (field_info_fields() as $field) {
    +	if ($field['storage']['type'] != 'field_sql_storage') {
    +  	continue;
    +	}
    +
    +	$function = $field['module'] . '_field_views_data_views_data_alter';
    +	if (function_exists($function)) {
    +  	$function($data, $field);
    +	}
    +  }
    +}
    

    This should all be in field_sql_storage module.

    +++ b/core/modules/field/field.views.incundefined
    @@ -0,0 +1,463 @@
    +/**
    + * Returns the label of a certain field.
    + *
    + * Therefore it looks up in all bundles to find the most used instance.
    + */
    

    It does what?

    +++ b/core/modules/field/field.views.incundefined
    @@ -0,0 +1,463 @@
    +  $data = array();
    +
    +  $current_table = _field_sql_storage_tablename($field);
    +  $revision_table = _field_sql_storage_revision_tablename($field);
    This is all field_sql_storage stuff. Field module should know nothing about that module.
    
  8. Regarding:

    Why is this is in views module and not node module?

    The sandbox moves all module-specific integrations from Views to their respective modules' directories. That said...

    Is it really necessary to commit all the support for every core module (aggregator, contact, search) in the initial patch?

    This isn't up-to-date with the core tracker schema which has some denormalization. Would be a critical performance regression if the tracker listing was updated to use it in its current state.

    Same with this and search module. I'd also be tempted to move these out of the patch and evaluate one by one - we'll have enough default views in core replacing core listings, I'm not sure the actual defaults that Views has happened to ship with historically make sense to include (maybe in the standard profile instead?).

    Alright, so we've spent over an hour discussing this this morning, and the consensus is that we will keep the integrations for comment, field, file, image, node, taxonomy, system, and user. The other integrations (aggregator, book, contact, filter, translation, locale, statistics, poll, search) will be removed and filed as separate patches

    Note that it is not simple to maintain these integrations outside of core. In the 8.x-3.x we needed some namespace hack to be able to split up the core-module integration code from views code. The core PSR-0 handling registers namespaces for each module on the module path, so for example “Drupal\node” for “core/modules/node/lib/Drupal/node”. The namespace hack now was to register \Views\node to views\lib\Views\node so the node specific plugins could get found by the annotation discovery code.

    Some of the default views are clearly features and should probably live in contrib.

  9. Regarding:

    This also brings up another issue - if either module is switched off, we'll still be copying default views into config despite them not being relevant. That question makes my head hurt but could use a follow-up (as could adding these defaults at all).

    The issue around default config provided by one module in the namespace of another is discussed in issues: #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API, #1765936: Invoke ConfigStorageController::delete on module uninstall and add tests, #1790610: [META] Ensure the Configuration and ConfigEntity systems fully support Views CRUD and status operations. We've also been talking to heyrocker for the past several days about what we want the behavior to actually be.

  10. Regarding:

    There's some switching on the core database supported types that is absolutely not allowed in core (it's occasionally OK to have engine specific code, but not to limit support to the three that core ships with. That should either be dropped or a critical issue will need to be added to move it to db drivers.

    The engine-specific code is all about date handling, which we hope the date component will help us with. #1817650: Add a date component which handles sql integration

  11. Regarding:

    +++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_default/Node.phpundefined
    @@ -0,0 +1,40 @@
    +  function get_argument() {o
    +	foreach (range(1, 3) as $i) {
    +  	$node = menu_get_object('node', $i);
    +  	if (!empty($node)) {
    +    	return $node->nid;
    +  	}
    +	}
    +
    +	if (arg(0) == 'node' && is_numeric(arg(1))) {
    +  	return arg(1);
    

    This is weird. It's also copy/pasted verbatim into a function in views module somewhere (see below).

    This is indeed ugly code but it works.

    One idea is to support other path structures like node/%node like foo/bar/%node or node/123 (but taken over by panels module). This will for sure be done completely different once we have a proper context system in place. Some similar code is in the user argument_default plugin.

    Opened an issue #1817630: Cleanup get_argument() on node/user default argument

  12. Regarding:

    Why can't this use core user autocomplete?

    Same question for taxonomy term autocomplete.

    There are sadly some limitations in the core autocomplete functions which have been required by views, but these functions could be massively simplified as you don’t need the "last name" logic anymore.

    Issues:

  13. Regarding:

    No reason for db_select() here. Also at some point the db connection should be injected.

    Good point. I think the proper level would be on the query plugin, as it handles all querying already. @damiankloip is looking into this (no issue yet).

  14. Regarding:

    Display/Block.php ...
    module_exists()?
    Also ouch, but I supposed this will all die with block plugins.

    The idea was to keep the block table up to date, even block.module is disabled.
    As the block display plugin is part of block.module now, we can skip the checking already.
    See #1621236: Fatal error on updating display machine name, when blocks module is not installed for more discussion but you are right this will die, yeah!

    Should be state().

    I agree, though this will also not be required with block plugins.
    #1817684: Use state() instead of variable_set() and remove redundant saveBlockCache() method in Block.php display plugin

  15. Regarding:

    One of the more serious performance issues with Views is that every display is loaded whenever one of them is viewed. Is there an issue looking at this? I once found over 60 displays on a single View, which was the fault of the people building the view but was also killing their site.

    That’s a good idea! The config at the moment is all stored in one file, so it will be loaded on the storage object, but we should implement lazy loading on the actual display object. (#1817582: Lazy load display plugins).
    Additionally, if we would save every setting into the config, and have the translations handled by config metadata wrapper we would actually also have some way which would not require to do this heavy heavy unpackOptions method but just put the values into the object.

    Seems from here that it might be possible to instantiate the displays during this access check instead of before?

    This is probably not a problem once we have lazy-loading of displays.

So, we have issues or plans for all points above, and the next patch to review should be smaller. :)

I also recommend cloning the sandbox and reviewing the code in there; bit more manageable than a 2.5 MB patch.

tim.plunkett’s picture

FileSize
2.38 MB

Here is a rerolled patch with the integration for aggregator, book, contact, filter, translation, language, locale, statistics, poll, search all removed.

Dries’s picture

Great review @catch. I'd be comfortable with splitting some things off the patch (xjm's list: aggregator, book, contact, filter, translation, locale, statistics, poll, search) and submitting and reviewing them separately.

Dries’s picture

Looks like Tim did exactly that when I was typing up my comment. It it took the patch from 2.5MB to 2.38MB ... :P

gdd’s picture

I have posted some commentary on the issue of installing/uninstalling defaults views at #1776830-9: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API if anyone is interested in diving into that problem.

Anonymous’s picture

i had a look through this code, mainly core/modules/views.

overall it looks good, and i love the fact that views are config files :-)

clicking around the views UI, and parts didn't work for me, though :-(

1. install drupal standard profile
2. disable overlay, enable testing, views, views_ui
3. go to admin/structure/views, click on the drop down button thingie and choose 'delete' for the archive view
4. go directly to WSOD, do not pass Go, do not collect $200

i think we definitely need tests all the basic operations of views_ui before it lands. i looked at the tests, and couldn't figure out how to test the new fangled dropdown thingies, but i'll return to it when i have more time.

some things i didn't understand about the code i did read below.

ViewStorageController does:


  protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
    foreach ($queried_entities as $id => $entity) {
      // Create a uuid if we don't have one.
      if (empty($entity->{$this->uuidKey})) {
        // Only get an instance of uuid once.
        if (!isset($this->uuidFactory)) {
          $this->uuidFactory = new Uuid();
        }
        $entity->{$this->uuidKey} = $this->uuidFactory->generate();
      }
      $this->attachDisplays($entity);
    }

    parent::attachLoad($queried_entities, $revision_id);
  } 

that comment doesn't tell me why we're doing the uuid stuff on load. it feels a bit wrong - the config storage controller does that sort of thing during create, which seems a lot more reasonable. can we either move that to create, or explain why we're doing write operations on an entity during a load?

re. heyrocker's comment in #33, views.view.foobar.yml files already have a module key, but it's not used to denote ownership in the way he's proposing. i disabled and uninstalled comment module, views.view.comments_recent.yml remains but doesn't show up in the listing because views checks the module key against module_exists(), and doesn't list any views for disabled modules.

i'm not sure what i think of that, and we don't need to work it out with this patch, but i was a bit surprised that views.view.comments_recent.yml doesn't live in core/modules/comment/config, and that the file is not deleted when i uninstall comment.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

David_Rothstein’s picture

I just updated the issue summary to point out that committing this has a pseudo-dependency on the discussion in #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw.

tim.plunkett’s picture

Deletion not functioning as expected is a known issue, blocked on CMI discussions.

We have 2 criticals we'd be bringing into the queue, and of course all VDC issues would come over. I don't necessarily think we're even pseudo-blocked on that.

damiankloip’s picture

#34, I can't remember exactly why we did this at the time. It had something to do with our legacy and default views. I guess we didn't want uuids on default views but once they were saved we did. This was a while ago so should be reviewed again.

xjm’s picture

@David_Rothstein, this is not blocked on #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw. Until another policy is adopted, adding Views is subject to our current policy, with the 15/100/15/100 thresholds.

xjm’s picture

Issue summary: View changes

Added note about pseudo-dependency on the discussion at http://drupal.org/node/1810428

xjm’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

I agree that #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw is not a blocker. It's an impetus for us to get that question resolved relatively quickly :D but definitely not a blocker for Views in Core.

effulgentsia’s picture

I'm all for merging in Views as soon as possible. But, if we do so, do we open up 1 critical for "make Views accessible" or open up 1 critical for each accessibility issue discovered? Do we open up 1 critical for "solve all Views performance issues" or 1 critical for each performance issue discovered? If the latter, then once people do the right thing and discover specific performance and accessibility issues and file them, if we don't also change our threshhold policy somehow, then all other features will be blocked for a while, potentially all the way until feature freeze, at which point they'll be blocked until D9 starts. However, I think merging in Views is a good thing to do, and we should solve #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw in a way that allows it while still keeping technical debt visible and manageable.

xjm’s picture

We open one critical for every release-blocker accessibility issue we discover, and one major for each major accessibility discovered, and so on. And then we solve them, and/or any of the 230 other criticals and majors, and features are unblocked again.

Views has a working D8 port with no non-core dependencies, thanks to 22 weeks of hard work. If we want it in core, now is the time to commit to that, so that we can leverage core process and the core community to sustainably resolve issues that affect 70% of all Drupal sites.

juan_g’s picture

Wow, you are really making Drupal history now... good work.

Views comes with a lot of power to be directly used by core and contrib, but in exchange will increase core code size in over 10%, I think. So, this is an exceptional situation indeed, which requires exceptional action.

However, for instance, of the seven critical VDC issues, two are in the VDC sandbox and five already in the current D8 core.

xjm’s picture

@juan_g, those latter five aren't "critical VDC issues"; they are critical core issues that affect VDC. :)

juan_g’s picture

Yes, that's the point. Good job.

David_Rothstein’s picture

@David_Rothstein, this is not blocked on #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw. Until another policy is adopted, adding Views is subject to our current policy, with the 15/100/15/100 thresholds.

But this issue specifically proposes that Views not be subject to our current policy, since Dries commented here a couple days ago that he would like to commit this without it having passed the accessibility and usability gates. I believe this makes sense also; however, adjusting the gates without adjusting the thresholds would really not be cool (they are very closely tied together), which is why it's related to #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw.

xjm’s picture

But this issue specifically proposes that Views not be subject to our current policy

It does no such thing. Not at all. Not in the slightest. I didn't put that in the summary and I don't see it anywhere in the comments. It's only about the specific numbers we want to set for the next eight weeks.

Dries proposed staggering the requirements for the gates because of the unique situation with Views--the size of it, the fact that we have seven years of technical debt that will in no way be resolved during this release cycle no more than core's will, and that the Views merge doesn't actually affect anything else yet, and that putting Views in actually makes it easier for us to do the needed testing and patching. Really merging Views in the way Dries has described is more akin to the initial Twig or Symfony commits than it is to overhauling our theme system or our routing, because Views isn't used anywhere else in core yet. There's no regressions, only a new hunk of code.

That's utterly separate from thresholds. If we have 101 major bugs on Monday, I expect to have to wait.

David_Rothstein’s picture

Reading your comment I'm actually pretty sure we agree with each other, although obviously it doesn't sound that way :) I suggest we chalk it up to a miscommunication and move on?

In short, to treat it the way you're suggesting (which makes all the sense in the world) we have to change the policy a bit, and #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw is where that's being discussed. And many people who have commented there are concerned that if we don't change it somehow, other features which come later will wind up being blocked (and this would be for no good reason, exactly because of your point above - "there's no regressions, only a new hunk of code"). So, let's change it, in that issue. ASAP :)

David_Rothstein’s picture

Issue summary: View changes

clarified merge date

webchick’s picture

Issue tags: +Avoid commit conflicts

Dries said he's going to try and merge this in sometime this afternoon. Assigning this tag. We should probably try not to commit anymore D8 patches until this is done.

If someone wants to post the command(s) he should use to merge in whatever branch from the sandbox work, that'd be helpful.

webchick’s picture

Issue summary: View changes

add merge commands

damiankloip’s picture

Issue tags: -Avoid commit conflicts

*looks at tim.plunkett*

cweagans’s picture

Issue tags: +Avoid commit conflicts

@webchick, see issue summary :)

tim.plunkett’s picture

For the record, the git history should look like this after merging
git log --oneline --graph

Screen Shot 2012-10-22 at 2.28.24 PM.png

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Merged! Great job to the entire Views in Core team! Thanks all.

pounard’s picture

Just some very quick notes from a very quick overview:

  • A lot of object method return explicit references to objects where it's unnecessary, because objects are always passed by reference. This may cause PHP weird bugs (it does when you do that with arrays, it makes the clone operation to misbehave).
  • Many coding standard rules are violated blantantly
  • Why almost every properties are public?
  • A lot of procedural wrappers seem unnecessary, such as views_view_is_enabled() for example, and duplicated, e.g. views_view_is_disabled(), and used only in tests (case in which it shouldn't leave in .module file)
  • Drupal /node path takes an average 100ms (on my box) while the /frontpage default view actually takes an average of 130ms (~+30%)
  • The init() method is not declared on the PluginBase class, neither in another class or interface in the hierarchy, thus code such as views_get_plugin($type, $name)->init(/* some args */); is both unable to follow in a decent IDE, and invalid (calling a non existing method in the interface, even thought it exists on the implementation)
  • Caching unpacked options seems overkill, am I the only one to think that? (it something that causes a lot of extra SQL queries in a D7 I'm working on), if options are that big it sounds there's a problem in the design
xjm’s picture

Category: feature » task
Priority: Critical » Normal
Status: Fixed » Active

Setting back to active so we can continue to use this meta for tracking.

@pounard, thanks for the feedback! Lots of good points. For performance, we're starting to explore some things in #1811816: Benchmark node_page_default vs. node view.

Regarding

Many coding standard rules are violated blantantly
Why almost every properties are public?

I am going to put this in big text because it seems I have to keep repeating it, even though it is stated in the summary. :)

Coding standards cleanups are all targeted for the next phase of the release cycle, after Dec. 1.

This includes renaming methods to camelCase, reformatting doxygen, evaluating class members for whether they should be public vs. protected, etc. etc. See:
http://drupal.org/community-initiatives/drupal-core/vdc-roadmap

All new patches must conform to our standards. Existing Views code will be updated later when we have some more pressing things sorted, like performance and accessibility issues, configuration system issues, and some major refactorings.

dawehner’s picture

The init() method is not declared on the PluginBase class, neither in another class or interface in the hierarchy, thus code such as views_get_plugin($type, $name)->init(/* some args */); is both unable to follow in a decent IDE, and invalid (calling a non existing method in the interface, even thought it exists on the implementation)

There is an issue for it: #1778356: Unify and standardize the init() method for the query plugin.

Caching unpacked options seems overkill, am I the only one to think that? (it something that causes a lot of extra SQL queries in a D7 I'm working on), if options are that big it sounds there's a problem in the design

Well, we introduced that because people had the requirement for caching. For d8 though we have plans that this will not be required anymore thanks two
different reasons: We don't need it any more for translations, and additional #1764380: Merge PluginSettingsInterface into ConfigurableInterface might give us enough performance. Additional when CMI exports all config (so also the default values of a view) we could dramatically simplify the unpackOptions logic.

To sum it up, we are working on making this better.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

We have been and are in the middle of a big refactoring for node data storage to support multilingual properties. Of course the merge of Views set it back considerably from a passing state to *now needing to rework node handling in views as well*. I'm not sure this is feasible in the multilingual initiative without helping hands from Views folks, so if you could help us out in #1498674: Refactor node properties to multilingual that would be highly appreciated. Thanks a lot!

tim.plunkett’s picture

This has and will continue to happen. Comment, image, D8MI patches being "set back" by VDC. But we are here to help!
Ping @dawehner, @damiankloip, @xjm, or myself in IRC, we're glad to share the work instead of playing catch-up ourselves :)

xjm’s picture

Issue tags: -Avoid commit conflicts
effulgentsia’s picture

Coding standards cleanups are all targeted for the next phase of the release cycle, after Dec. 1.

@xjm: I'm wondering if this refers only to the priorities of the existing active VDC developers. If someone wants to do the work and submit patches for Views coding standards cleanup now, is that encouraged, or will that mess things up for you / be ignored / or otherwise discouraged? I'm not volunteering, just wanting to clarify for others. Thanks.

xjm’s picture

@xjm: I'm wondering if this refers only to the priorities of the existing active VDC developers. If someone wants to do the work and submit patches for Views coding standards cleanup now, is that encouraged, or will that mess things up for you / be ignored / or otherwise discouraged? I'm not volunteering, just wanting to clarify for others. Thanks

Discouraged strongly for two reasons (both from my personal experience in this area):

  1. Reviewing these patches is time-consuming, tedious, and requires great attention to detail. It takes longer to review them than to create them, and someone with high expertise still needs to review them to ensure they're correct.
  2. Commit conflicts. The VDC team has a very high patch throughput, and we're going to be doing a lot of refactoring over the next 38 days. We don't have time for messy rerolls, or even not-so-messy ones.

So, I will postpone anything that is purely cleanup with no functional change or refactor. (I've actually suggested doing this throughout core until Dec. 1). We have an entire phase of the release cycle devoted to API cleanup, and an entire phase after that devoted to polish. Let's use them.

xjm’s picture

Also, reiterating, #60 only applies to the existing Views codebase. New patches should still conform to all our standards, as per the core gates and other policy/process issues.

xjm’s picture

Forgot to add, thanks @effulgentsia for helping clarify that. :)

xjm’s picture

I've uploaded the removed module integrations in their own issues: #1853522: [META] (Re)introduce Views data integration for core modules. Most will need cleanup, test coverage, review, etc.

xjm’s picture

xjm’s picture

Per popular request, I've filed the following postponed metas for cleanup efforts later in the cycle:

I'm also working on updating the official roadmap based on the new release schedule, and will update the summary here after that. Thanks everyone!

xjm’s picture

xjm’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

WorldFallz’s picture

Issue summary: View changes

Updated issue summary.

LeeHunter’s picture

Issue summary: View changes

Added link to the docs issue #1892470 for moving advanced help

alexpott’s picture

Issue tags: -Configuration system

Belatedly joining the tag war

alexpott’s picture

Issue summary: View changes

Updated issue summary.

mgifford’s picture

Is this all we need to do to close this issue? #1806298: Test whether Views' form fields are labeled

effulgentsia’s picture

diff --git a/core/modules/views/lib/Drupal/views/Plugin/views/sort/MenuHierarchy.php b/core/modules/views/lib/Drupal/views/Plugin/views/sort/MenuHierarchy.php
new file mode 100644
index 0000000..0b74348
--- /dev/null
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/sort/MenuHierarchy.php

Is this plugin useful to anyone? It's about to be removed in #2301319-31: MenuLinkNG part5: Remove dead code; and party!, so if we need it back, let's open a child issue to add it back in a way that works with the new menu link architecture.

jibran’s picture

I think this can be closed now.

Crell’s picture

Assigned: Unassigned » xjm

Assigning to someone to close it. :-)

klonos’s picture

...pretty funny that this issue is still in "active" state. The two remaining tasks (#1806298 and documentation) can be left as follow-ups and I'm sure this issue can be closed. We do have a change notice, right?

catch’s picture

Status: Active » Fixed

Yes let's close it, views is in core.

klonos’s picture

Thanx.

Status: Fixed » Closed (fixed)

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