Problem/Motivation

At the moment the view classes has a lot of different variables and methods
which are at the end sadly only somehow related to a view, but not really seperated.

Especially for the config entity it would be helpful to split the runtime information from the configuration.

Proposed resolution

Split up the class into two clases: ViewStorage and ViewExecutable or something named like that.

ViewExecutable

The ViewExecutable would contain information like:

  • base_table, base_field
  • built,executed,editing
  • args,build_info,result,pager-information
  • exposed_data, exposed_input
  • current_display
  • etc.

The ViewExecutable can be constructed by using a view entity object in the constructor.
So the code could look like:

public function __construct(ViewStorage $storage) {
  $this->storage = $storage;
}

To be able to access the storage information you could either access them via $view->storage->foo though as you see this requires quite
some work for the existing code. Therefore i propose some magic __get and __set which will first look at the storage and then return
information stored on the ViewExecutable object.

ViewStorage

View storage will certainly hold all the "entity" information of a view so:

  • name
  • base_table
  • human_name
  • display
  • etc.

Remaining tasks

Decide what belongs to the executable

API changes

The objects will be seperated so maybe api has to changed as well.

CommentFileSizeAuthor
#60 views-1751358-59.patch84.26 KBdawehner
#59 views-1751358-59.patch70.82 KBtim.plunkett
#57 views-1751358-57.patch68.38 KBdawehner
#54 views-1751358-53.patch70.82 KBtim.plunkett
#52 views-1751358-52.patch68.43 KBtim.plunkett
#50 views-1751358-49.patch67.52 KBtim.plunkett
#47 views-1751358-47.patch66.84 KBtim.plunkett
#47 views-1751358-47-more-debug.patch67.19 KBtim.plunkett
#46 views-1751358-46.patch67.78 KBtim.plunkett
#44 views-1751358-44.patch65.65 KBtim.plunkett
#43 views-1751358-43.patch63.62 KBtim.plunkett
#41 views-1751358-39.patch63.03 KBtim.plunkett
#40 views-1751358-40.patch63.17 KBdawehner
#37 views-1751358-37.patch63.26 KBdawehner
#35 views-1751358-35.patch68.49 KBdawehner
#33 views-1751358-33.patch68.49 KBdawehner
#30 views-1751358-30.patch57.67 KBtim.plunkett
#28 views-1751358-28.patch53.44 KBtim.plunkett
#26 views-1751358-26.patch52.29 KBtim.plunkett
#23 views-1751358-23.patch56.75 KBtim.plunkett
#23 interdiff.txt4.61 KBtim.plunkett
#20 views-1751358-20.patch54.72 KBtim.plunkett
#18 1751358-split-up-18.patch55.11 KBdawehner
#16 1751358-split-up-interdiff.txt5.49 KBdawehner
#16 1751358-split-up-14.patch53.29 KBdawehner
#14 views-1751358-14.patch49.78 KBtim.plunkett
#12 views-1751358-12.patch47.8 KBtim.plunkett
#12 interdiff.txt7.12 KBtim.plunkett
#10 views-1751358-10.patch43.44 KBtim.plunkett
#10 interdiff.txt2.24 KBtim.plunkett
#8 views-1751358-8.patch42.62 KBtim.plunkett
#6 views-1751358-6.patch42.5 KBtim.plunkett
#4 views-1751358-4.patch43.53 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sylvain Lecoy’s picture

Sylvain Lecoy CreditAttribution: Sylvain Lecoy commented

I created a discussion group on this: http://groups.drupal.org/node/249958.

Sylvain Lecoy’s picture

Sylvain Lecoy CreditAttribution: Sylvain Lecoy commented

Would you like to work with me on that particular problem ? I need some understanding of the overall architecture then we can categorize attributes and parameters.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Just updated how to construct a ViewExecutable/ how to access information in itself.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes

updated summary

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
views-1751358-4.patch43.53 KB

At least adding a new view works, but then a lot of things break.

One question i do have and could not answer yet:
Where should the displays be "stored" on runtime.
It seems to make sense to store the displays on the executor object.
Ideally viewDisplay could be removed so $executable->display['default']->getOption() could work
but to reduce the amount of changes for now i would vote for $executable->display['default']->handler as it is now.
On the storage just the display_options are stored as the pure array.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-6.patch42.5 KB

Rerolled and fixed the one line.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-8.patch42.62 KB

Missed the rename while rerolling.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt2.24 KB
views-1751358-10.patch43.44 KB

Fixed up a couple things

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt7.12 KB
views-1751358-12.patch47.8 KB

Pushed up http://drupalcode.org/sandbox/damiankloip/1685040.git/shortlog/refs/head...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-14.patch49.78 KB

More fixes.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
1751358-split-up-14.patch53.29 KB
1751358-split-up-interdiff.txt5.49 KB

Just posting a patch which is not applied to the branch yet.
This allows to at least not fatal all of the tests.

Some random spin-off based on this issue: #1787270: Split up default views into one per module

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
1751358-split-up-18.patch55.11 KB

Thanks for helping on this issue!

Committed the changes from #14 and made some fixes on top of it.

Regarding magic methods: you don't have to do the default code, as the function will never be called without it.
http://www.php.net/manual/en/language.oop5.overloading.php#object.get
...

Status: Needs review » Needs work

The last submitted patch, 1751358-split-up-18.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
FileSize
views-1751358-20.patch54.72 KB

Sorry about the unpushed code.

Your last patch left out the & for __get

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett
Status: Needs review » Needs work

I had way less fails in #14, going to figure out why

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt4.61 KB
views-1751358-23.patch56.75 KB

More stuff.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Status: Needs review » Needs work

@tim.plunkett... I was looking at the most recent patch and was confused at times by the ordering of the various 'use' statements. I am not sure what our standard is, but does it make sense to put them in alphabetical order if there is no standard yet?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
views-1751358-26.patch52.29 KB

Right now we're just trying to get it to work, we can clean that up before commit.
Plus, there is no core convention.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-28.patch53.44 KB

More fixes.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-30.patch57.67 KB

Added more fixes, but also a couple more tests.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned
Status: Needs review » Needs work

With the exception of the ViewTest failure, it's all 404 stuff, which I couldn't track down.

I pushed all of my commits up to 1751358-split-up in Damian's sandbox.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
views-1751358-33.patch68.49 KB

Pushed two changes. The first one fixes the menu issue. The second one cleans up some code.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
views-1751358-35.patch68.49 KB

Another try to upload a patch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
views-1751358-37.patch63.26 KB

94501b7 Make sure that only enabled views are used in hook_menu

This commit should fix a lot of the test errors, maybe this time the test actually applies,
but i don't 100% get that.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

There should be more green with that.

e5a2633 document the style_options variable and rename setItemsPerPage on the pager plugin

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
views-1751358-40.patch63.17 KB

Now even with a patch

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-39.patch63.03 KB

Added an __isset()

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-43.patch63.62 KB

There was a bug in cloneView.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: Unassigned » tim.plunkett
FileSize
views-1751358-44.patch65.65 KB

Okay, now I'm throwing exceptions in the else cases of the magic methods, to try to find bad properties.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-46.patch67.78 KB

More fixes.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-47-more-debug.patch67.19 KB
views-1751358-47.patch66.84 KB

Okay, here's the patch above without the debug code, and another patch with more debug code.

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

The last submitted patch, views-1751358-47-more-debug.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
FileSize
views-1751358-49.patch67.52 KB

Hm, removed too much.

Status: Needs review » Needs work

The last submitted patch, views-1751358-49.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
FileSize
views-1751358-52.patch68.43 KB

Added a helper for initDisplay.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented 
+++ b/lib/Drupal/views/ViewExecutable.phpundefined
@@ -736,7 +839,7 @@ public function initPager() {
-        $this->pager->setItemsPerPage($this->items_per_page);
+        $this->pager->set_items_per_page($this->items_per_page);

Why are we removing the camelcase on functions? :s

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned
FileSize
views-1751358-53.patch70.82 KB

Okay! Here it is with docs and @todos added.

I can create follow-ups after I go to lunch :)

But I think this is good to go.

Status: Needs review » Needs work

The last submitted patch, views-1751358-53.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Assigned: Unassigned » dawehner

Assign myself.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
views-1751358-57.patch68.38 KB

When the storage controller saves a config file you don't have a executable yet
and you also don't want one.

    if (!$config->isNew() && !isset($entity->original)) {
      $entity->original = entity_load_unchanged($this->entityType, $id);
    }

so let's for __set first check whether certain properties exists

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

A list of properties which aren't on the viewExecutable __set

  • display
  • base_field

maybe more but couldn't find more.
So if we fix these usages we could maybe remove the __set helper.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
views-1751358-59.patch70.82 KB

I forgot to push my docs changes before, this includes the fix from #57.

I think we should phase out the magic methods in a follow-up.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Assigned: dawehner » Unassigned
FileSize
views-1751358-59.patch84.26 KB

New version which fixed a lot of the existing code (beside display) to access the right object automatically (getHumanName(), name, base_table, base_field, etc.)

Status: Needs review » Needs work

The last submitted patch, views-1751358-59.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: Split up view class into view and viewExecutable » Split up View class into ViewStorage and ViewExecutable
Status: Needs work » Reviewed & tested by the community

I'd say #59 is RTBC, and deal with #60 in a follow-up.

tim.plunkett’s picture

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

http://drupalcode.org/project/views.git/commit/65fb7f9
http://drupalcode.org/project/views.git/commit/74ce4fc

Committed!

#1788232: Move UI properties from ViewExecutable to a new ViewUI class is the first follow-up.

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

some updates