Posted by dawehner on December 3, 2012 at 7:51pm
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | views.module |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | VDC |
Issue Summary
In order to support the full use-case of #1857256: Convert the taxonomy listing at /taxonomy/term/%term to Views we need a way to render a taxonomy term as header of a view.
To make it as generic as possible we should introduce an entity area which allows to choose the entity type and entity ID (coming from a textfield so you could use arguments).
Comments
#1
This patch works fine so far, though we need real tests.
#2
This time with a patch. I couldn't get it working to use entity_view and compare the rendered output, so I choosed to just check the label.
#3
So there are tests now.
#4
+++ b/core/modules/system/lib/Drupal/system/Plugin/views/area/Entity.phpundefined@@ -0,0 +1,117 @@
+ $form['view_mode'] = array(
+ '#type' => 'select',
+ '#options' => $options,
+ '#title' => t('View mode'),
+ '#default_value' => $this->options['view_mode'],
+ );
I think view mode should be first, but that's just me :)
+++ b/core/modules/system/lib/Drupal/system/Plugin/views/area/Entity.phpundefined@@ -0,0 +1,117 @@
+ // Add tokenization form elements.
+ $this->tokenForm($form, $form_state);
Do we want all the token form available here.
+++ b/core/modules/system/system.views.incundefined@@ -0,0 +1,26 @@
+ 'title' => $label,
I think we should prefix this label with 'Rendered entity: ...' or something? People could easily get confused about the fact that every entity has on of these OR .. (Which I think is preferable) is to just have one generic rendered entity handler. So have an additional entity select field.
+++ b/core/modules/system/system.views.incundefined@@ -0,0 +1,26 @@
+ 'help' => t('Shows an @label in the area.', array('@label' => $label)),
Either way we could change this to something like, 'Render an entity....blah'
#5
I think view mode should be first, but that's just me :)I really don't care that here :)
Do we want all the token form available here.We talked about it, and it's doesn't hurt.
I think we should prefix this label with 'Rendered entity: ...' or something? People could easily get confused about the fact that every entity has on of these OR .. (Which I think is preferable) is to just have one generic rendered entity handler. So have an additional entity select field.I totally agreed, fixed it.
#6
#5: drupal-1857376-5.patch queued for re-testing.
#7
+++ b/core/modules/system/system.views.incundefined@@ -0,0 +1,29 @@
+ * Provide views data and handlers for node.module.
This says node.module, in system.views.inc
Also, catch seemed rather reluctant to add views integration in system module
#8
Yeah, this might have to move into views instead.
#9
The last submitted patch, drupal-1857376-5.patch, failed testing.
#10
Let's move it! Fixed also some minor points like some @see, empty lines and different test groups.
#11
Getting this issue going again (I know it's only been 5 days :)).
The previous patch was not filtering out the view entity type, as it was looking for 'views' and not 'view'. So I added some more test coverage for the data side of things.
I also changed the check to just check for all config entities, as we don't really want any of them. See interdiff.
#12
Thank for starting the issue again :)
+ if (!is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityInterface')) {So if block instances will be config entities we don't want to allow people to render them here? This really sounds wrong. In general, why should we restrict to something which actually could work.
#13
How about looking for things that provide a render controller class? Blocks will have that. And views can too #1857422: Add a render controller for Views, those can be used in an area handler currently.
#14
Yeah, that sounds sane. Do defaults get assigned for this?
#15
Oh damn, it has a default of EntityRenderController, so EVERYTHING will have one. I'll open an issue to remove that default, all entity types shouldn't be renderable.
#16
See #1882240: Remove default assignment of render_controller_class in EntityManager
#17
Yeah, I thought we might have a problem with this. That issue makes sense.
#18
Just to be sure, I think excluding views itself still makes sense, because the views area handler will always be special/provide a better use-case specific UI.
#19
Hopefully once these entity tests got converted it would be easier to have a proper unit test here.
I removed the config storage controller check and replaced with a 'render_controller_class' check.
#20
+++ b/core/modules/menu_link/menu_link.moduleundefined@@ -116,7 +116,7 @@ function menu_link_delete_multiple(array $mlids, $force = FALSE, $prevent_repare
-function menu_link_save(MenuLink $menu_link) {
+function menu_link_save(MenuLink $menu_link) {
I wonder how that happened :)
#21
Looking good overall, just a couple of things:
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.phpundefined@@ -0,0 +1,118 @@
+ $form['entity_id'] = array(
+ '#title' => t('ID'),
+ '#type' => 'textfield',
+ '#default_value' => $this->options['entity_id'],
+ );
I guess just having the entity ID is what we have to do, as an entity doesn't have to have a path, but maybe people expect a path? Just food for thought
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.phpundefined@@ -0,0 +1,118 @@
+ if (!empty($this->entityInfo['view_modes'])) {
+ foreach ($this->entityInfo['view_modes'] as $mode => $settings) {
+ $options[$mode] = $settings['label'];
+ }
This should use entity_get_view_modes now.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.phpundefined@@ -0,0 +1,118 @@
+ $entity_id = $this->globalTokenReplace($this->options['entity_id']);
Nice, we get ALL THE TOKENS.
+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/AreaEntityTest.phpundefined@@ -0,0 +1,91 @@
+ $entity_info = $this->container->get('plugin.manager.entity')->getDefinitions();
ughhh. I want entity_get_info back.... :)
+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/AreaEntityTest.phpundefined@@ -0,0 +1,91 @@
+ $entity_test = $this->container->get('plugin.manager.entity')->getStorageController('entity_test_render')->create(array('bundle' => 'entity_test_render', 'name' => $random_label));
Would having a variable for $data of something make that verboseness better?
+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/AreaEntityTest.phpundefined@@ -0,0 +1,91 @@
+ $this->assertEqual(trim((string) $result[0]), $random_label, 'The rendered entity appears in the header of the view.');
Is there any way we can assert something to do with the view mode too? Might be tricky with entity_test.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.phpundefined@@ -0,0 +1,118 @@
+ * id = "entity"
We should add views as the module here, as we are probably removing the default in ViewPluginManager. This could wait but I guess we might as well add it while the patch is being worked on? If not, we can just add it when we do that change.
#22
Yeah it's not the best UX so far. Once this patch is in we could iterate on top of it, and reuse the entity_reference autocompletion callbacks (optionally) to use it here.
The great here is that this allows us to drop loading the entity info.
Good idea!
Well ... we could put some logik into the render controller, like print out the name of the viewmode. Do you think we should do it in this issue?
#23
If it's a smallish change (adding the view mode), which I think it should be? Then I say go for it in here, then it's out of the way and tested. Once it is rendered, the assertion is pretty quick to add I guess.
Good idea using the entity reference autocomplete! That will make a nice follow up issue.
I wont change the status yet in case you want to roll a new patch with the view mode? Otherwise, test away.
#24
This needed way too long, so stepping back from the computer after that patch.
#25
The last submitted patch, drupal-1857376-24.patch, failed testing.
#26
#24: drupal-1857376-24.patch queued for re-testing.
#27
Awesome, This is great now. Sorry if those tests took a while :/ They are good though! I think that makes much better coverage.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.phpundefined@@ -0,0 +1,105 @@
+ protected function buildViewModeOptions() {
This is a good idea, for easy overriding if needed.
#28
Since this is an entity system integration (and required to convert the taxonomy term view) I think this is more of a task.
#29
I don't really get allowing an explicit ID here, for taxonomy terms it's going to need to take the term from arguments and the test is hard-coding this to a test entity with ID = 1?
#30
For taxonomy/term, while I don't really like them, I'd normally except the taxonomy term at the top to be an attachment.
#31
Ehm right, I planned to put that in later, but we can for sure do that now as well.
Extended the test coverage for that as well.
#32
I'd still like to know what we get from this compared to using an attachment display (except for not having to instantiate two displays). Or wait if one display is listing nodes and the other taxonomy terms is that why?
#33
So right, an attachment can be only created on the same view, which means in practice that you can only have the same entity types listed there.
In theory you could embed a view in the area (that's already possible), though this would require a lot more steps then simply embedding a single entity in there. Additional at the moment you can't do that for blocks (just an example) as we don't have a EQ query backend, but this will probably change in the future.
If you don't think that's useful for core, I will create a contrib project for it and put it in there.
#34
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.php@@ -0,0 +1,111 @@
+ $options = entity_get_view_modes($this->entityType);
+
+ return $options;
Here should be some processing for options before returned and used in view mode select.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Entity.php@@ -0,0 +1,111 @@
+ $build = entity_view($entity, $this->options['view_mode']);
+ // @todo Support to just return a render array.
+ return drupal_render($build);
Do we have an issue for this @todo somewhere?
#35
Can you explain what you meant with that?
Here is a follow-up: #1934420: Allow area handlers to return a render array
#36
Sorry, see EntityRow::buildViewModeOptions() or add rendered entity to header and select view mode.
#37
Oh yeah, you are absolute right!
#38
#39
The last submitted patch, drupal-1857376-37.patch, failed testing.
#40
Just fixing the test.
#41
This is looking great now!
#42
#40: drupal-1857376-40.patch queued for re-testing.
#43
The last submitted patch, drupal-1857376-40.patch, failed testing.
#44
The testbot could really be nicer and give some better error messages. :-)
#45
On the other hand it would help if I run the tests myself. Humans 1 - 1 Machines.
Thanks @dawehner for pointing this out.
#46
this is blocking #1857256: Convert the taxonomy listing at /taxonomy/term/%term to Views, also changes is legit. back to rtbc, it is going to take the bot a while to catch up but it ll let us know if it disagrees
#47
This would be so great to have it in!
+++ b/core/modules/views/views.views.incundefined@@ -106,5 +106,21 @@ function views_views_data() {
+ 'title' => t('Rendered entity - @label', array('@label' => $label)),
+ 'help' => t('Displays a rendered @label entity in an area.', array('@label' => $label)),
So I'm wondering whether we display 'entity' to the site builder now? Not sure about the recommended way at the moment.
#48
Reviews are mixed on that point. I know Bojhan's not a fan, I'm equally not a fan of hiding things under the covers, especially when a word actually means what it is (unlike "node").
However, since strings are something we can change anytime before RC1, I think it's ok to let this go in and have a follow-up discussion about that.
Committed and pushed to 8.x. Thanks!
#49
Automatically closed -- issue fixed for 2 weeks with no activity.
#50
+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/AreaEntityTest.php
+ public function testEntityArea() {
...
+ $result = $this->xpath('//div[@class = "view-header"]');
+ $this->assertTrue(strpos(trim((string) $result[0]), $entities[0]->label()) !== FALSE, 'The rendered entity appears in the header of the view.');
+ $this->assertTrue(strpos(trim((string) $result[0]), 'full') !== FALSE, 'The rendered entity appeared in the right view mode.');
+
+ $result = $this->xpath('//div[@class = "view-footer"]');
+ $this->assertTrue(strpos(trim((string) $result[0]), $entities[1]->label()) !== FALSE, 'The rendered entity appears in the header of the view.');
+++ b/core/modules/views/tests/views_test_config/test_views/views.view.test_entity_area.yml
+ header:
...
+ entity_id: 1
...
+ footer:
...
+ entity_id: !1
A hidden gem in the footer area entity_id.
Should the tests have passed? Depends on whether entity_test entity IDs are integers, and whether 0 is a valid entity ID.
> php -r "var_dump((int) '!1');"int(0)
#51
Can we please, please, please open followups rather than resurrecting closed issues? Until we can make some changes to improve the issue queue UX, reopened, multi-commit, sprawling issues present a barrier to contribution. They create confusion when blaming for the commit log, muddy the issue scope, make the subsequent changes more difficult to review, and make it more difficult to assign self-contained cleanups or bug fixes to novices.
#52
Filed #1957050: Bugged tests in the entity error handler?.
#53
@sun
The entity area handler allows you to replace tokens by arguments from the url/passed into the view. So !1 stands for the first argument of the view.
<?php
$this->drupalSetContent($view->preview('default', array($entities[1]->id())));
$result = $this->xpath('//div[@class = "view-footer"]');
$this->assertTrue(strpos(trim((string) $result[0]), $entities[1]->label()) !== FALSE, 'The rendered entity appears in the header of the view.');
?>
So we check here that the token got replaced with the actual right ID and then the full entity is loaded.
From my perspective this doesn't seem to be an issue, feel free to reopen if you think I'm wrong.
#54
Automatically closed -- issue fixed for 2 weeks with no activity.