This method is currently living in our View storage class, it doesn't really belong there. The storage class doesn't need to care about plugin managers or anything like that. This is only used in Views UI, specifically in the list controller.

Let's move it there. I have currently just commented out the (random) test assertion in ViewStorageTest for this. Not sure we have a test specifically for the listing controller?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

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

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -9,6 +9,7 @@
+use Drupal\views\Views;

@@ -156,4 +157,24 @@ public function render() {
+  public function getDisplaysList($view) {

This can be protected now

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -9,6 +9,7 @@
+use Drupal\views\Views;

@@ -156,4 +157,24 @@ public function render() {
+    $manager = Views::pluginManager('display');

We can inject this here!

+++ b/core/modules/views_ui/views_ui.theme.incundefined
@@ -103,7 +101,7 @@ function template_preprocess_views_ui_view_info(&$variables) {
-  $output .= '<h3 class="views-ui-view-title">' . $variables['title'] . "</h3>\n";
+  $output .= '<h3 class="views-ui-view-title">' . $variables['label'] . "</h3>\n";

Oh come on :) Scope creep!

damiankloip’s picture

FileSize
3.72 KB
6.31 KB

Ha :) How about this then? I'm not sure about injecting the storage controller? I guess we have to just get it from the container like that? As createInstance doesn't have knowledge that the actual EntityManager has called it.

tim.plunkett’s picture

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -7,13 +7,55 @@
+    $this->entityType = $entity_type;
+    $this->storage = $storage;
+    $this->entityInfo = $entity_info;

Any reason to not just call parent::__construct() I don't care much either way

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -7,13 +7,55 @@
+    $this->displayManager = $display_manager;

@@ -156,4 +198,24 @@ public function render() {
+    $manager = Views::pluginManager('display');
...
+      $definition = $manager->getDefinition($display['display_plugin']);

Seems you forgot to actually use it?

Status: Needs review » Needs work

The last submitted patch, 2012882-3.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
6.31 KB

Whoops, missed out a use. Also made changes from feedback in #4, thanks.

EDIT: I did not call the parent, as that calls entity_get_info() in it's constructor. We can inject that now.

dawehner’s picture

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -156,4 +198,23 @@ public function render() {
+   * Gets a list of displays included in the view.
+   *
+   * @return array
+   *   An array of display types that this view includes.

Missing @param documentation

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
@@ -156,4 +198,23 @@ public function render() {
+  protected function getDisplaysList($view) {

That's what I don't really get is how to test such functionality. This is clearly a method which could be proper unit tested, but it's marked as protected

damiankloip’s picture

FileSize
916 bytes
6.45 KB

Hmm, not sure. I guess we could only test it in the UI :/ If only we had some way to test protected methods or something.... ;) (Do you remember that issue?)

dawehner’s picture

FileSize
10.08 KB

Here is a unit test for this piece. This also fixes a small issue in the actual code, as it uses render arrays now.

Status: Needs review » Needs work

The last submitted patch, drupal-2012882-9.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
838 bytes
10.12 KB

I guess we might have to do this for now?

Status: Needs review » Needs work

The last submitted patch, 2012882-11.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
644 bytes
10.12 KB

This should fix it.

Status: Needs review » Needs work

The last submitted patch, drupal-2012882-11.patch, failed testing.

damiankloip’s picture

I guess we both had the same change for that.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
10.22 KB

hmm, SO something has changed recently that breaks table cells being rendered with drupal_render() ?!! I'm sure this worked once upon a time.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, 2012882-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#16: 2012882-16.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, 2012882-16.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
10.17 KB
37.71 KB

We have to go deeper!

Note: This is an interdiff against #13

damiankloip’s picture

Of course!!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Nice fix @dawehner!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a544e13 and pushed to 8.x. Thanks!

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