Posted by dawehner on August 20, 2012 at 9:40am
22 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | views.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
| Issue tags: | Needs tests, VDC |
Issue Summary
Since all the entity integration like a generic entity property will be in core one day
it would make sense to create a generic entity integration which generates the stuff automatically.
On top of that the node/user/comment entity views controller would be able to add extra items like edit/add/delete links.
The general problem though with that is that it's will not be obvious anymore to find the actual views integration of the modules.
Comments
#1
Here is the code which could work, but well core property api doesn't have a definition for node yet.
#2
The last submitted patch, 1740492-entity-view-controller.patch, failed testing.
#3
#4
I did a quick 15-minute skim of the patch.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/DefaultEntityController.phpundefined@@ -0,0 +1,15 @@
+/**
+ * @Plugin(
+ * id = "default",
+ */
+class DefaultEntityController extends EntityControllerPluginBase {
Needs a one-line summary at the top of the docblock.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+use Drupal\Component\Plugin\PluginBase;
+
+
+/**
Extra blank line.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ * Generic entity views controller which utilities the entity property api.
The class docblock should start with a third-person verb. API should also be all caps.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+class EntityControllerPluginBase extends PluginBase {
+ /**
+ * @todo
+ */
+ protected $entity_type;
There should be a blank line between the start of the class and the first docblock.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ public function __construct(array $configuration, $plugin_id, $entity_type) {
Needs a docblock.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ /**
+ * Defines the result for hook_views_data().
+ */
+ public function viewsData() {
Needs return value documentation.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ /**
+ * Try to come up with some views fields with the help of the schema and
+ * the entity property information.
+ */
...
+ /**
+ * Comes up with views information based on the given schema and property
+ * info.
+ */
The function summary should be one line. We also need parameter and return value docs.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ // We name the field of the first reverse-relationship just with the
+ // base table to be backward compatible, for subsequents relationships we
+ // append the views field name in order to get a unique name.
Let's break this up into two sentences.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ $name = !isset($this->relationships[$info['base table']][$this->entity_info['base table']]) ? $this->entity_info['base table'] : $this->entity_info['base table'] . '_' . $views_field_name;
This ternary is a bit hard to scan; let's break it up.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ $return['relationship'] = array(
+ 'label' => drupal_ucfirst($info['label']),
Let's add an inline comment describing this hunk.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ // Add in direct field/filters/sorts for the id itself too.
ID should be all caps.
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ // Append the views-field-name to the title if it is different to the
+ // property name.
"...different from the property name."
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ switch ($type) {
Is there a cleaner way to do this than a big switch?
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ // @todo: This class_exists is needed until views 3.2.
Is this still relevant? Can we clarify it a little? What/why?
+++ b/lib/Drupal/views/Plugin/views/entity_controller/EntityControllerPluginBase.phpundefined@@ -0,0 +1,313 @@
+ return 'views_handler_relationship';
+ }
+
+
+
+}
We just need one blank line before the end of the class. :)
+++ b/lib/Views/field/Plugin/views/field/Field.phpundefined@@ -381,7 +381,7 @@ class Field extends FieldPluginBase {
-
+''
Oops. :)
I'll reroll touching some of this up.
#5
I didn't spend too much time on this; just added some @todo stubs where there are docs to fill in. Also not addressed:
#6
#7
Is this issue plan to make the view object an entity?
Subscribing
#8
@Sylvain Lecoy: The goal of this issue is to provide unified entity handling for views of entity data based on the new D8 entity system; it's not related to the View objects themselves. See: http://drupal.org/node/1400186 (Rearchitecture related to the view object itself is happening separately.)
#9
#10
The bases class should be abstract.
And does "'handler' => 'views_handler_field_numeric'," works?
Don't we need to use the plugin id?
#11
@aspilicious
This is just a start for the patch ...
#12
Is this title better?
#13
Also, for people wondering, #1741154: Replace ctools_include('export') with Configurable Thingies is the meta issue for the view object becoming a configurable.
#14
#5: views-1740492-5.patch queued for re-testing.
#15
@bojanz
This issue is probably a perfect task to talk and work together during badcamp and other badcamp-related places like in switzerland (not sure whether you attend any of those). What are your time-plans regarding this feature.
There might be problems which better should be solved somewhere in the entity storage level
so the earlier we identify potential issues the better.
#16
Lets be honest.
#17
#18
Nothing fancy so far, filled with a couple of questions.
The same as #1839624: Add a views integration for entity_test.module this issue requires #1783196: Get the views plugin manager from the DIC when possible to get in.
Some blockers for a 100% support on the entityNG controller side:
#19
Worked on getting automatic views integration with tests for revisions.
#20
sorry
#21
This looks very interesting. #1833334: EntityNG: integrate a dynamic property data table handling is adding different test entity classes for all combinations of revision/properties tables. Sounds like this could be used here as well as soon as it is in.
+++ b/core/modules/entity/lib/Drupal/entity/EntityViewsController.phpundefined@@ -0,0 +1,168 @@
+ function __construct($entity_type) {
+ $this->entityType = $entity_type;
+ $this->entityInfo = entity_get_info($entity_type);
+ $this->storageController = entity_get_controller($entity_type);
To make it easier to convert to DIC later, I suggest we pass in the storage controller as an argument.
+++ b/core/modules/entity/lib/Drupal/entity/EntityViewsController.phpundefined@@ -0,0 +1,168 @@
+ // @todo Is translating the right way to handle label/description?
+ $views_field['title'] = t($typed_data['label']);
+ $views_field['help'] = t($typed_data['description']);
Not necessary, this is already translated, unlike the 7.x version that was based on hook_schema().
#22
That's a good idea, thank you!
#23
+++ b/core/modules/entity/entity.views.incundefined--- /dev/null
+++ b/core/modules/entity/lib/Drupal/entity/EntityViewsController.phpundefined
Wondering where's the correct place for this. Given that the views_entity_get_views_controller() function lives in views.module, this might belong there as well.
This is views integrating with a core component, not the entity.module integrating it's tables with views.
For testing, it would be interesting to compare the generated views data with manually created views data definitions to check what we're missing here, e.g. for the node and users table. That will of course have to wait until these are converted to NG and we currently know which parts are missing anyway. But it might make sense to do a follow-up to convert existing types to this after the NG conversion.
#24
Yeah i have been struggling with that and moved stuff around from system to entity to views and back. One reason why it maybe should be part of entity module is that all the other core entity controllers also can be found in that directory so it might confuse people to have that in another place, but i really don't care :)
It's indeed a good idea, so some general things which are missing (but i would sort of introduce maybe with that or with another issue are things like: a generic link/uri field, maybe also label and in general every kind of single operation with a link.
#25
The other controllers are still in Drupal\Core\Entity, where this one definitely can't be as Core stuff can't depend on an optional module (we have quite a few user references in there, but that's a required module).
Doing additional features in follow-up issues sounds fine to me. What do we want to define as goal of this issue then? Basic support for the three tables and the existing properties? According to the test, data table support is missing, what exactly is blocking you there? I guess we need a way to identify which properties are in there (which is the same as they being translatable, right? Which is a property that I also need for TMGMT)
#26
Oh good point, so yeah the best place is probably views itself.
Regarding the goal of the issue: I think it should be a generic integration which works for every entity using entityNG. I don't think it makes sense to convert the existing implementations already to that controller, as it would make it really hard to review.
From my understanding #1833334: EntityNG: integrate a dynamic property data table handling blocks the data integration, but it's already long ago when I wrote that todo :)
One general problem is the question how to map typed data to handlers, currently it is hardcoded, but it feels like that we could put that information either on the typed data itself, or name the plugin_id's exactly how the typed data.
#27
Just some rerolling, no actual progress.
#28
It's a bit odd as you have different properties in the different tables, well.