Add commit credit for: crowdcg

Page in question: admin/structure/contact

List of contact forms displays no links to view a contact form.

After creating a contact form, there is no way to view it, nor to link to it. If you read the help text extremely carefully you'll see the text:

"You can also create links to other contact forms; the URL has format contact/machine_name_of_form."

However, the machine name is also not in the table. The only way to get there is via the "edit" operation. This was a hard blocker to progress, and caused a call to the help desk.

I would tag this as novice, but we were worried that the only way to solve this might be to solved for all entity listing pages, which would obviously be a bigger deal.

Possible solutions:

  • Make sure the table of contact forms has a link to the forms. At least expose the machine name or something.
  • Fix this consistently for all entities so that any entity listing page has the ability to get to the form to create more of it
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Issue summary: View changes
webchick’s picture

Issue summary: View changes
Berdir’s picture

Issue tags: +Novice

#1997692: Create contact form block had a fix for this for a very long time (see #27 and #28 for example), but the issue was delayed on changing many other things too.

As I think I suggested there already, I think we should really just extract that part of the fix into this issue add a test if it's not there already and get it in.

The code isn't generic and it doesn't make sense for most other config entities, because most other config entities can't be viewed. So just fixing it here is perfectly fine IMHO :)

HellooooNewman’s picture

Working on this at DrupalNorth sprint

HellooooNewman’s picture

Status: Active » Needs review
FileSize
1.28 KB

patch attached, reviews welcome

Status: Needs review » Needs work

The last submitted patch, 5: there_is_no_link-2513396-5.patch, failed testing.

star-szr’s picture

I did some pair programming with @HelloNewman on this, I'm looking into the test failures.

star-szr’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
602 bytes
1.87 KB

This should do it, this is the test update from #1997692: Create contact form block.

Also this issue should credit @crowdcg (#1997692-28: Create contact form block), adding that to the issue summary.

star-szr’s picture

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

This could use test coverage to ensure the link points to the right place. I think the test I updated only checks that the label exists and (because of the XPath) that it's inside a link.

star-szr’s picture

Going to see if I can add those assertions real quick.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
864 bytes
2.26 KB

Test only patch serves as interdiff.

The last submitted patch, 11: there_is_no_link-2513396-11-test.patch, failed testing.

larowlan’s picture

+++ b/core/modules/contact/src/ContactFormListBuilder.php
@@ -32,13 +33,14 @@ public function buildHeader() {
+      $row['form'] = Link::createFromRoute($this->getLabel($entity), 'contact.site_page_form', ['contact_form' => $entity->id()]);

Should we be adding a new link template to the entity-type instead? We could make this the canonical link template, then we could use $entity->link(NULL, 'canonical') instead.

larowlan’s picture

something like this.
also fixed PHPCS this:

+++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -250,10 +250,13 @@ function testSiteWideContact() {
+    $view_link = $this->xpath('//table/tbody/tr/td/a[contains(@href, :href) and text()=:text]', array(':href' => \Drupal::url('contact.site_page_form', ['contact_form' => $contact_form]), ':text' => $label));

nitpick, arrays > 80 chars

larowlan’s picture

+++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -250,8 +250,13 @@ function testSiteWideContact() {
+    return;

whoops, cheating - fixing

larowlan’s picture

Also fixed the phpcs issue on this line (existing) but given we're already touching it

Status: Needs review » Needs work

The last submitted patch, 16: 2513396-contact-links.15.patch, failed testing.

The last submitted patch, 14: 2513396-contact-links.14.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
3.22 KB
722 bytes

Thanks @larowlan!

star-szr’s picture

Sorry for the noise, array can stay short.

MattA’s picture

+    $view_link = $this->xpath('//table/tbody/tr/td/a[contains(@href, :href) and text()=:text]', [
+      ':href' => \Drupal::url('entity.contact_form.canonical', ['contact_form' => $contact_form]),
+      ':text' => $label,
+      ]
+    );

Ideally, this should be split up into more variables instead of arbitrary newlines.

star-szr’s picture

@MattA I'm not sure why you think the newlines are arbitrary. Suggestions welcome if you can think of a way to improve the readability of the code but it looks like a pretty common pattern to my eyes. Thanks!

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Cottser

MattA’s picture

@Cottser Well, generally speaking, it leads to "after x number of arguments, add a newline" situations. Which is further complicated by arrays. Do I move it all on to the next line? Do I leave the first '[' on the previous line and have a newline in the middle of my parameter? If the array is long, how do I handle indenting?

The solution is simple, more variables. So for that particular line, something like:

$pattern = '//table/tbody/tr/td/a[contains(@href, :href) and text()=:text]';
$url = \Drupal::url('entity.contact_form.canonical', ['contact_form' => $contact_form]);
$view_link = $this->xpath($pattern, [':href' => $url, ':text' => $label]);

Typically this makes it easier to pick out and understand what goes into each variable, and the function call in question. That said, I'm more than willing to make exceptions.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

One thing I'm not sure of is whether this now requires an upgrade path, since it renames a route machine name. #2507899: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29

The way to test would be:

1) Install Drupal.
2) Make a link to a contact form in a menu (I believe Standard profile does this already)
3) Apply the patch
4) Refresh.
5) See if everything blows up.

If so, we need to write a hook_update_N() (with tests) so that existing sites can run update.php to recover. And/or, don't rename the route machine name.

larowlan’s picture

I did #25

Added a menu link to /contact/feedback - it is stored in the db as internal:/contact/feedback (menu_link_content_data) table, and then in the menu_tree table using the machine name.

Applied the patch.
Cleared caches.

Site did not blow-up.
Reference in {menu_tree} had it's route name updated.

That's some cool ju-ju, kudos to those who wrote said code.

pwolanin’s picture

If you add a menu link content entity using a path the new route name should be found the next time menu link discovery runs, so I don't think there's any need for an update hook.

Same should be true if a module or install profile defined a link.yml file referencing it, as long as the module or profile updated the yml

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Thanks a lot for the investigation! Sounds like we're good here, then. I grepped for any other references to contact.site_page_form (such as from the Help page or similar) and found none.

I also looked into MattA's suggestion about variable-izing the calls to $this->xpath(), however in a cursory grep, it seems like core looks a lot more like the current patch, in general.

Sounds like back to RTBC. We're in a commit freeze atm because of the beta, but I'll try and get this in tomorrow! W00t!!

  • webchick committed 197fa45 on 8.0.x
    Issue #2513396 by Cottser, larowlan, HelloNewman, webchick, crowdg,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Sorry, that day came and went but here we are. :)

Committed and pushed to 8.0.x. Thanks!

andypost’s picture

Status: Fixed » Closed (fixed)

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