Posted by xjm on November 29, 2012 at 2:12am
11 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | contact.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | VDC |
Issue Summary
Part of meta #1853522: [META] (Re)introduce Views data integration for core modules. From the 8.x-3.x branch of Views.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| contact.patch | 2.81 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 48,894 pass(es). | View details | Re-test |
Comments
#1
The last submitted patch, contact.patch, failed testing.
#2
Some small cleanup.
#3
Actually wrote a test and realized how much was actually broken, like it never actually appeared on some use-cases.
#4
Tested this, works, code looks great, tests looks complete, comments look great.
Awesome!
#5
Left the UUID in the test yml file.
#6
Is this really the only way to check the access? It's an extra query for every row when you enable this. I realise contextual module does it as well but it's a horrible pattern. The only thing I can think of though is doing what contact module does in its own callback.
#8
We could rewrite _contact_personal_tab_access to accept multiple accounts, though I'm really wondering whether it's worth to do so,
as N function calls on N rows is nothing compared to all other things going on like entity API.
Would you be okay with directly calling the access method of contact? We didn't used that in D7 because people might want to replace the access logic and so could do this in one place via hook_menu_alter.
#9
I think directly calling the access method is probably OK. It's not that much different to calling node_access() really.
If you think that's too fragile, just adding a comment explaining why we have to use menu_get_item() there might be enough as well.
#10
To be honest, this is such an edge case that to require overriding the views handler is fine for me.
#11
The last submitted patch, drupal-1853526-10.patch, failed testing.
#12
#10: drupal-1853526-10.patch queued for re-testing.
#13
The last submitted patch, drupal-1853526-10.patch, failed testing.
#14
#10: drupal-1853526-10.patch queued for re-testing.
#15
The last submitted patch, drupal-1853526-10.patch, failed testing.
#16
#10: drupal-1853526-10.patch queued for re-testing.
#17
#10: drupal-1853526-10.patch queued for re-testing.
#18
#10: drupal-1853526-10.patch queued for re-testing.
#19
The last submitted patch, drupal-1853526-10.patch, failed testing.
#20
#10: drupal-1853526-10.patch queued for re-testing.
#21
The last submitted patch, drupal-1853526-10.patch, failed testing.
#22
Moved some tiny bits around and fixed things here and there. The interdiff thought would be hard to read as files have been moved.
#23
#22: drupal-1853526-22.patch queued for re-testing.
#24
The last submitted patch, drupal-1853526-22.patch, failed testing.
#25
This should fix it.
#26
The last submitted patch, drupal-1853526-25.patch, failed testing.
#27
Now!
#28
+++ b/core/modules/contact/lib/Drupal/contact/Tests/Views/ContactLinkTest.php@@ -0,0 +1,116 @@
+ $result = $this->xpath('//div[contains(@class, "views-field-contact")]//a[contains(@href, :uri)]', array(':uri' => $contact_uri));
+ $this->assertTrue(count($result));
AssertLink is not sufficient here?
#29
Yeah we could use it here, though it feels always safer for me, to check the full output here, as this adds additional kind of test coverage.
Feel free to convince me of the opposite, then I'm happy to rerole.
#30
I think it's fine as-is.
#31
#27: drupal-1853526-27.patch queued for re-testing.
#32
#27: drupal-1853526-27.patch queued for re-testing.
#33
+++ b/core/modules/contact/tests/modules/contact_test_views/test_views/views.view.test_contact_link.yml@@ -0,0 +1,119 @@
+human_name: test_contact_link
That should be label, I think.
#34
Good catch, we def need to change this since the label patch landed :)