We currently have a new Display() method on our ViewExecutable class. This seems fine at first, but then look at the storage class. We also have a newDisplay() method on there which calls addDisplay to get a new id to use then passes this to the newDisplay method on the ViewExecutable. So this method doesn't create a new display at all but just creates an instance from storage.

I vote that we change the name of this method, as well as the docs, which currently start with "Creates and stores a new display.", or, do we even still need this method at all? #1817582: Lazy load display plugins will affect what goes on here I think. Looking at that patch, I think if a new display is added we also need to update the displayIDs property on DisplayArray?

CommentFileSizeAuthor
#31 drupal-1867304-30.patch8.6 KBdamiankloip
#30 drupal-1867304-30.patch0 bytesdamiankloip
#30 interdiff.txt2.12 KBdamiankloip
#29 drupal-1867304-29.patch10.28 KBdamiankloip
#29 interdiff.txt1.88 KBdamiankloip
#25 drupal-1867304-25.patch8.83 KBdawehner
#25 interdiff.txt913 bytesdawehner
#22 1867304-22.patch7.94 KBdamiankloip
#22 interdiff.txt1.2 KBdamiankloip
#17 1867304-17.patch7.73 KBdamiankloip
#17 interdiff.txt4.19 KBdamiankloip
#15 interdiff.txt1.01 KBdawehner
#15 drupal-1867304-15.patch4.29 KBdawehner
#13 1867304-13.patch4.29 KBdamiankloip
#7 drupal-1867304-7.patch3.92 KBdawehner
#4 durpal-1867304.patch3.91 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Title: Rename newDisplay method on ViewExecutable » Assess newDisplay method on ViewExecutable
damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

So maybe removing newDisplay on ViewExecutable could be an option and just let display array create the instance?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

newDisplayInstance maybe?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Active » Needs review
FileSize
durpal-1867304.patch3.91 KB

Really, now we have the DisplayArray class to manage our display instances, we don't really need newDisplay on the ViewExecutable. We can create our instance without it. It also saves the displays back to storage but doesn't change anything. So it's a bit broken anyway!

Here is a patch that removes newDisplay as it's only called by newDisplay on the View storage. I have also added some new methods to DisplayArray to access the displayIDs. Thoughts?

Status: Needs review » Needs work

The last submitted patch, durpal-1867304.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Postponed

Postponing on #1869566: Allow any collection of plugins to be lazily instantiated for now, these methods can move onto there anyway.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Postponed » Needs review
FileSize
drupal-1867304-7.patch3.92 KB

Rebased against #1869566: Allow any collection of plugins to be lazily instantiated

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Postponed

Well ...

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Isn't that that same, just rebased against the changes from #1850792: Make init() method consistent across all views plugins?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Yeah exactly.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.phpundefined
@@ -316,7 +316,10 @@ protected function generateDisplayId($plugin_id) {
+    $executable = $this->get('executable');
+    $executable->initDisplay();
+    $executable->displayHandlers->addDisplayID($id);
+    return $executable->displayHandlers[$id];

I'm wondering whether we could do this lazy aka only add the display ID if an executable actually exists..., because if you initialize it later, you will not have problems.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -2192,38 +2192,4 @@ public function setItemOption($display_id, $type, $id, $option, $value) {
-    $display = $this->storage->get('display');
...
-    $this->storage->set('display', $display);

So this never actually stored anything on the storage, this seems to be a bug in the old code :)

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

That sounds like a good plan to me, let's work on that once this is unpostponed and the lazy loading patch is in.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
1867304-13.patch4.29 KB

Here is a new patch based on the #1869566: Allow any collection of plugins to be lazily instantiated patch that just went in.

I have added the methods that were on DisplayArray previously to the PluginBag class, do we need these here? I vote yes anyway.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Postponed » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
drupal-1867304-15.patch4.29 KB
interdiff.txt1.01 KB

In general I'm wondering whether the ::set() method should call addInstanceID?
Is the fix a proper one?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.phpundefined
@@ -311,12 +311,20 @@ protected function generateDisplayId($plugin_id) {
+    if ($executable = $this->get('executable')) {

This doesn't help, because the get method always instanciats an executable, so we should better check for if ($executable = $this->executable); Maybe we should also document this few lines.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff.txt4.19 KB
1867304-17.patch7.73 KB

Yeah, that's a good point! I forgot we were doing that in get(). If we change this we also have to change the tests a bit too. What do you think?

The interdiff is slightly out of date, I amended a couple of docs (minor) too.

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

The last submitted patch, 1867304-17.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
Issue tags: +VDC

#17: 1867304-17.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

This looks really good already!

+++ b/core/lib/Drupal/Component/Plugin/PluginBag.phpundefined
@@ -95,6 +95,36 @@ public function remove($instance_id) {
+  public function addInstanceID($id) {

Should set() call addInstanceID?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -189,13 +189,15 @@ protected function displayTests() {
@@ -316,19 +318,23 @@ protected function displayMethodTests() {

That's a good improvement in terms of readability of the patch.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -316,19 +318,23 @@ protected function displayMethodTests() {
+    $executable->destroy();

Seems unneeded but I don't care.

Status: Needs review » Needs work

The last submitted patch, 1867304-17.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.2 KB
1867304-22.patch7.94 KB

Yeah, I guess it does make sense to also add the instanceID in set() too.

I also removed that destroy call. More of a habit than anything :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Awesome

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1867304-22.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt913 bytes
drupal-1867304-25.patch8.83 KB

Let's fix the test failures.

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

The last submitted patch, drupal-1867304-25.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

#25: drupal-1867304-25.patch queued for re-testing.

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

The last submitted patch, drupal-1867304-25.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.88 KB
drupal-1867304-29.patch10.28 KB

Now that we are adding to the instanceIDs array too (which we should have been to start with I think but meh) we need to unset it from here too so we don't get the pluginBag trying to create an instance.

So we need a couple of tests for this too, to check they are removed from the instanceIDs array. See interdiff.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff.txt2.12 KB
drupal-1867304-30.patch0 bytes

Ok, spoke to Tim on IRC. His thoughts are we should not be modifying instanceIDs when we add/remove instances. So here is basically an earlier patch, let's see how we get on.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
drupal-1867304-30.patch8.6 KB

Sorry, and a real patch to boot.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

This looks like a good API improvement.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

I think #1876942: Use direct methods instead of arrayAccess for display handlers should now be waiting on this too.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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