Add commit credit for: crowdcg
Page in question: admin/structure/contact
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
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff.txt | 872 bytes | star-szr |
#20 | there_is_no_link-2513396-20.patch | 3.21 KB | star-szr |
#19 | interdiff.txt | 722 bytes | star-szr |
#19 | there_is_no_link-2513396-19.patch | 3.22 KB | star-szr |
#16 | 2513396-contact-links.15.patch | 3.21 KB | larowlan |
Comments
Comment #1
webchickComment #2
webchickComment #3
Berdir#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 :)
Comment #4
HellooooNewman CreditAttribution: HellooooNewman at Northern Commerce commentedWorking on this at DrupalNorth sprint
Comment #5
HellooooNewman CreditAttribution: HellooooNewman at Northern Commerce commentedpatch attached, reviews welcome
Comment #7
star-szrI did some pair programming with @HelloNewman on this, I'm looking into the test failures.
Comment #8
star-szrThis 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.
Comment #9
star-szrThis 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.
Comment #10
star-szrGoing to see if I can add those assertions real quick.
Comment #11
star-szrTest only patch serves as interdiff.
Comment #13
larowlanShould 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.
Comment #14
larowlansomething like this.
also fixed PHPCS this:
nitpick, arrays > 80 chars
Comment #15
larowlanwhoops, cheating - fixing
Comment #16
larowlanAlso fixed the phpcs issue on this line (existing) but given we're already touching it
Comment #19
star-szrThanks @larowlan!
Comment #20
star-szrSorry for the noise, array can stay short.
Comment #21
MattA CreditAttribution: MattA commentedIdeally, this should be split up into more variables instead of arbitrary newlines.
Comment #22
star-szr@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!
Comment #23
larowlanThanks @Cottser
Comment #24
MattA CreditAttribution: MattA commented@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:
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.
Comment #25
webchickOne 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.
Comment #26
larowlanI 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.
Comment #27
larowlanScreenshots for posterity
Before
https://www.dropbox.com/s/evrh9ij7d284zu3/Screenshot%202015-06-30%2009.1...
https://www.dropbox.com/s/gwhvl743tbxxp6x/Screenshot%202015-06-30%2009.1...
After
https://www.dropbox.com/s/wpe0e4pffkh8z2a/Screenshot%202015-06-30%2009.3...
Comment #28
pwolanin CreditAttribution: pwolanin as a volunteer commentedIf 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
Comment #29
webchickThanks 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!!
Comment #31
webchickSorry, that day came and went but here we are. :)
Committed and pushed to 8.0.x. Thanks!
Comment #32
andypost