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?

Files: 
CommentFileSizeAuthor
#20 deper.png37.71 KBdawehner
#20 vdc-2012882-20.patch10.17 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,233 pass(es). View
#20 interdiff.txt1.64 KBdawehner
#16 2012882-16.patch10.22 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#16 interdiff-2012882-16.txt1.03 KBdamiankloip
#13 drupal-2012882-11.patch10.12 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,502 pass(es), 37 fail(s), and 0 exception(s). View
#13 interdiff.txt644 bytesdawehner
#11 2012882-11.patch10.12 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 54,985 pass(es), 38 fail(s), and 0 exception(s). View
#11 interdiff-2012882-11.txt838 bytesdamiankloip
#9 drupal-2012882-9.patch10.08 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#8 2012882-8.patch6.45 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,364 pass(es). View
#8 interdiff-2012882-8.txt916 bytesdamiankloip
#6 2012882-6.patch6.31 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,212 pass(es). View
#6 interdiff-2012882-6.txt1.16 KBdamiankloip
#3 2012882-3.patch6.31 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 56,103 pass(es), 0 fail(s), and 21 exception(s). View
#3 interdiff-2012882-3.txt3.72 KBdamiankloip
vdc.move-getDisplaysList.patch5.14 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,319 pass(es). View

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
FAILED: [[SimpleTest]]: [MySQL] 56,103 pass(es), 0 fail(s), and 21 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 55,212 pass(es). View

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
PASSED: [[SimpleTest]]: [MySQL] 56,364 pass(es). View

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
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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
FAILED: [[SimpleTest]]: [MySQL] 54,985 pass(es), 38 fail(s), and 0 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 56,502 pass(es), 37 fail(s), and 0 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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
PASSED: [[SimpleTest]]: [MySQL] 57,233 pass(es). View
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.