I'm trying to use the entity embed module to embed core Contact forms.

I have an entity embed button with the following:
Embed type: Entity
Entity type: Contact form

This works in displaying the form on the node, along with being able to be filled in & submitted successfully. However there are two resulting issues:

1) The form is submitted twice. I'm guessing this is once when the form is submitted normally, then a second time when entity_embed tries to render the form again? Duplicate form submission caused by Metatag module.
2) After submission, the form isn't rendered again. I've looked, and it's because of (by default) the contact form submit returns a Redirect Response (EnforcedResponseException) rather than the renderable form array. This triggers the try/catch in EntityEmbedFilter::process(). Disabling the redirect results in the form displaying, but leaves all form values filled in.

Has anybody used this with the core Contact module? Would this use case require specific filters etc. - if so, can anyone point me in the right direction of what needs to be done so I can take a stab at this?

CommentFileSizeAuthor
#18 2748581-more-specific-catch-18.patch754 bytesnadavoid
#11 2748581-more-specific-catch-11.patch753 bytesvolkerk
#9 2748581-more-specific-catch-9.patch903 bytesvolkerk
#4 2748581-more-specific-catch.patch601 bytesvolkerk

Comments

smaz created an issue. See original summary.

smaz’s picture

smaz
Primary language English
Location England, UK
commented
Issue summary: View changes

Ok, I've found out why the duplicate submissions: Metatag module is generating the Description tag for the page based on the teaser. When it renders the teaser, it's re-triggering the form submission when entity_embed brings in the form during filter processing.

hanzoz’s picture

hanzoz commented

Hey,

I've the same issue where if the form was being embedded to a page and after the user submit, the "Redirect Page" value is not working.

really need some assistance for this issue

volkerk’s picture

volkerk commented
Status: Active » Needs review
StatusFileSize
new 2748581-more-specific-catch.patch601 bytes

Only catch the exception type (EntityNotFoundException) which is thrown in EntityEmbedFilter.

volkerk’s picture

volkerk commented
Category: Support request » Bug report
slashrsm’s picture

slashrsm commented
Issue tags: +D8Media

Can someone confirm that #4 solves the problem?

Status: Needs review » Needs work

The last submitted patch, 4: 2748581-more-specific-catch.patch, failed testing.

smaz’s picture

smaz
Primary language English
Location England, UK
commented
Status: Needs work » Needs review

I've just tested #4, and and it does fix the issue.

I have:

** before patching **
1) Created an entity embed button for contact forms, and added it to a text format (Full HTML)
2) Created a node, and embedded the 'Website feedback' contact form
3) Submitted the embedded form, and verified it reloaded the current page but without the form, as per the original issue

** after patching **
4) Re-submitted the embedded form, and was redirected to the homepage (default for that form) with the submission message displayed
5) Updated the contact form to set the redirect to a custom node
6) Resubmitted the embedded form, and was redirected to the specified node with the submission message displayed

That's great, thank you for the patch!

However, I've set it to needs work instead of RTBC because:

  1. I'm not sure if that test failure above is caused by this?
  2. Do we also want to add a comment as to why we only catch EntityNotFoundException, rather than Exception?

I've also not tested with embedding YAML Forms, but that could be a separate issue if it doesn't work with that - the main thing is working with the core Contact module.

volkerk’s picture

volkerk commented
StatusFileSize
new 2748581-more-specific-catch-9.patch903 bytes

I improved the patch to also catch the RecursiveRenderingException which is thrown some lines up.

IMHO it's generally a good idea to only catch exceptions which you throw, form redirection is done using an EnforcedResponseException, which was catched here, therefore breaking the redirect and proving my point.

Status: Needs review » Needs work

The last submitted patch, 9: 2748581-more-specific-catch-9.patch, failed testing.

volkerk’s picture

volkerk commented
Status: Needs work » Needs review
StatusFileSize
new 2748581-more-specific-catch-11.patch753 bytes
slashrsm’s picture

slashrsm commented 
         catch(\Exception $e) {
-          watchdog_exception('entity_embed', $e);
+          if ($e instanceof RecursiveRenderingException OR $e instanceof EntityNotFoundException) {
+            watchdog_exception('entity_embed', $e);
+          }
+          else {
+            // Keep throwing it.
+            throw $e;
+          }

Should we rather have multiple catch statements?

smaz’s picture

smaz
Primary language English
Location England, UK
commented
Status: Needs review » Needs work

I've just done some more testing - I realised the version I was testing against before was 8.0-alpha1, not the latest dev like I thought. Apologies.

It looks like Contact forms can no longer be selected in the 'Entity type' list, because it doesn't have a view builder. This commit fixed the check for view builders.

I'm not sure what could be done to allow entities that don't have view builders, if anything?

slashrsm’s picture

slashrsm commented

I'm not sure what could be done to allow entities that don't have view builders, if anything?

Nothing :). It is up to the entity type to provide it. Contact storage module adds view builder for contact forms. What happens if you enable it?

smaz’s picture

smaz
Primary language English
Location England, UK
commented

Nothing :). It is up to the entity type to provide it. Contact storage module adds view builder for contact forms. What happens if you enable it?

That allows 'Contact form' to be selected as an entity type, but only has two display options - Entity ID or Label, which just output the string for id / label when embedded.

Does that mean we'll need to look at creating a module to provide a view builder for contact forms that's suitable for Entity Embed? If so I can try & take a punt at it.

Cheers

slashrsm’s picture

slashrsm commented

It seems that the problem is the fact that config entities use pseudo view mode "Default", which doesn't actually exist. We could try to add support for that in \Drupal\entity_embed\Plugin\Derivative\ViewModeDeriver. Check if default view mode exist and create plugin derivative for the pseudo view mode if not.

volkerk’s picture

volkerk commented
Status: Needs work » Needs review

I already created an issue for the 'rendered entity formatter' vs 'view mode' problem, see https://www.drupal.org/node/2796269.

Anyway I think this bug should be fixed on it's own, if you prefer separate catch statements we could rewrite it.

nadavoid’s picture

nadavoid commented
StatusFileSize
new 2748581-more-specific-catch-18.patch754 bytes

Rerolled the patch in #11 against the latest dev, commit bdb4e9d

Status: Needs review » Needs work

The last submitted patch, 18: 2748581-more-specific-catch-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Closed (works as designed)
Issue tags: +Media Initiative

#14++

I'd also be very careful to use Entity Embed to embed forms: Drupal's forms are highly dynamic, interactive pieces of content (in the sense that they require communication with the server), whereas generally "viewing an entity" means it's something static, something to be read/consumed.

I think arguably, this is working as designed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3055050: Loosen EntityEmbedFilter's greedy exception capture: catch only relevant exceptions

Months have passed, #3055050: Loosen EntityEmbedFilter's greedy exception capture: catch only relevant exceptions fixed this :)