Turn contact form submissions into full-blown Contact Message entities, without storage

Contains a few nice-to-have code clean-up todos, but this shit works.

Fixed Field API integration.

Proof:

Fixed typos and removed stale/obsolete @todos.
Fixed strange constructions in user cancel forms.

• Fixed bogus #maxlength for subject originally contained in contact_personal_form().
• Added required category storage and list controller adjustments for Field UI.
• Fixed EntityFormController::submit() does not clean submitted form values before buildEntity().
• Fixed auto-reply e-mail processing and bogus test expectations.

The good news: Contact messages are fully working entities. :)

Even better news: This work revealed a range of bugs in many different spots.

What still needs work:

Field API blows up if there are actual fields configured for the message. Configuration of the fields works fine. Only when visiting /contact, the system bails out with a "::getWidget() called on a non-object" fatal error (or similar).

As with the other adjustments in this patch, this almost certainly means that Field API assumes an entity ID to exist somewhere. Most likely, it would magically start to work, if the contact_message would define an 'id' entity key in its entity info (even though that wouldn't ever be populated). In turn, there are most probably two possible routes here; the soft/lame one (add a fake ID key + property), or the hard one (bending/teaching Field API to stop presuming that an ID would exist). ;)

1. Fixed Field Attach API throws fatal error on a fieldable entity without bundle value.
2. Added Preview action for contact form, and entity render controller basics.
3. Re-implemented contact form category selection as tabs/local tasks.

Updated for entity type plugins.

1. Merged HEAD.
2. Updated for latest changes in HEAD.

This blows up on the new Entity Field Query though... — it looks like that added an assumption to Field API that all fields would be attached to entities that are stored (and have a 'base table')... :(

1. Fixed field_has_data() wrongly assumes that all entities would be stored.
2. Fixed contact category tab/local-task should not appear when there is only one category.
3. Fixed missing $default_method variable in user_cancel_confirm_form().

Also created #1848874: Clean up and simplify user cancel methods and processing and will copy those changes into that issue.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

1. Fixed type-hints, namespace imports, and phpDocs.
2. Removed stray @todo.
3. Added hook_field_extra_fields() for default contact form elements.
4. Tweaked code comment for copy option.

Overall, this looks relatively complete to me now and could use some reviews - will fix the test failures now.

Updated ContactSidewideTest for new categories.

No worries:

+++ b/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Message.php

+ *   id = "contact_message",
+ *   label = @Translation("Contact message"),

The internal entity type name is properly namespaced to Contact module already. :)

What we could do is to add a property + config option to each contact category that indicates whether to expose it as a tab - would that resolve that use-case? Sounds rather trivial to me.

That said, I'd prefer to do that in a follow-up issue, since this patch is large enough already, and actually this patch does not introduce any additional features on its own - it focuses on the entity + bundle + fieldable conversion only.

Updated issue summary.

Thanks for testing! :)

I've added a few more things that could possibly be improved in follow-ups to the issue summary.

1. Added rendering of fields into contact message mail.
2. Cleaned up code and comments.
3. Removed fallback code for legacy site-wide contact form not passing a category.

I consider this patch pretty much complete (and done if testbot approves). :)

Whoopsie - Fixed contact message mail view is not rendered.

Fixed only variables can be passed by reference.

Hm. I'm not able to reproduce the Drupal\comment\Tests\CommentLanguageTest test failure locally.

Slightly adjusting issue title to clarify that this is not a "conversion" issue like all the others, which are refactoring stuff, whereas this issue is a new feature.

Also, I think this feature is ready. Some parts of the new code could be simplified and improved later on, but that will require to rethink and refactor some Entity/Field API internals first.

I think this is a very good and important step into the right direction, not only for Contact module's future, but also to harness and demonstrate the power of our entity/field systems. Lastly, for many sites, this means that they do not necessarily have to install Webform module anymore.

Final reviews, anyone? :)

Please note that this patch still contains user cancel method changes, which have been split out into #1848874: Clean up and simplify user cancel methods and processing already, but are required for this patch to work (since it dynamically switches the contact message author/sender form elements from input fields into #type 'item').

A "View contact form" operation on the category listing page sounds like a good idea for #599770: Clean up the contact forms listing UI: Allow to set the default category and weights on the listing page

Also note that #1848874: Clean up and simplify user cancel methods and processing is still part of this patch. I don't mind for it to get committed as part of this patch here though.

Thanks for your review, @Berdir!

I think it should be possible to use entity_page_label() for this

Hm. That function sounds like its purpose is to supply an entity's label for the page title...? This callback here is about the menu link title though - the page title is detached from that.

I think that's a separate discussion, which we should definitely have for D8. All of the entity type providing modules need to implement way too many callbacks right now, and I think that should be unified and simplified - perhaps even on the Entity class itself (e.g., label() + blockTitle()... or similar).

Will need a EntityNG follow-up if there isn't one already. That might allow to change the recipients thing below into a custom field that can load the recipient on demand or so... ?
...
Not sure I understand this correctly, but this isn't a problem that's specific for this, is it? I don't think we have a single non-config entity in core that does *not* require a custom storage controller ;)

Yeah, probably. That said, I can't really make sense of the current EntityNG situation — it looks like we're continuing to convert everything to Entity right now... and will have a giant Entity to EntityNG conversion later on, ultimately eliminating EntityNG?

The recipient is actually an external data source that needs to be passed in (for the user contact form only). It may be comparable to how the parent/context entity is passed into the new, decoupled comments - but I didn't have time to check and review how the new comments are doing that.

Technically, I guess it's a user reference property/field, but it only applies to the user contact form — as mentioned in the issue summary, there's room for improving the situation and exploring whether that should be a category/bundle in follow-up issues.

As long as it is not a bundle, that's also the reason for why the entity needs to disable 'fieldable'.

As mentioned, I'm happy to investigate possible ways to improve the user contact form handling. However, given the size of this patch, I'd really prefer to do that in separate issues. :)

- relying on no base table being set to indicate no storage. Dpesn't feel right although I don't have a great suggestion.

The actual reason for that exact condition is that the new Entity Query throws an exception on the exact same condition. I've clarified the comment.

The new Entity/Field Query generally seems to have much more limitations that the old EFQ did not have. One of those limitations is the aspect we're facing here: Technically, Field API should perfectly be able to query the field data tables only, without any kind of dependency on the entity storage - I do not understand why this dependency exists in HEAD. I don't know whether that was an intended design choice for whether it's an architectural bug — the new EFQ is fairly complex, so I didn't have time to study it yet.

Apparently, earlier patches in this issue worked just fine without any kind of tweak to Field API for that. Only after the new EFQ landed, those exceptions started to happen. That is why I do not see that detail to be a problem that needs to be figured out in this issue. I think this will have to be addressed in #1845372: Deleting a field from a non-node entity bundle results in an Entity Field Query Exception and/or related issues (I already commented over there).

- putting this straight into entity info means that patches like the entity reference one are going to list it as something which could be referenced, which of course it can't be. This is strongly linked to the no storage issue.

Well, I see two points there:

1) The issue summary already links to a follow-up issue that might introduce an actual storage of contact messages (which would be nice to have). If that won't happen for D8 core, then I guess that a contrib module would actually be able to plug it in. Therefore, I do not think it is not guaranteed that contact message entities will never be stored.

2) We have the same issue with config entities, and we have a very similar issue with regard to generating entity tokens and all entity types that cannot be rendered.

I think this overall issue needs to be addressed in a much larger scope. At least it doesn't make sense to me to implant a workaround for the contact message entity type only, when there is no sign of a solid idea or conclusion on how to resolve the overall problem space. That said, I'm fairly sure that the final solution will be based on entity type metadata (entity info property/ies), so I think that the contact message entity being introduced here can be brought in line very easily.

Does AccessDeniedHttpException let you pass a custom message?

It has a $message argument and so I tried, but its value is not used. I'm not sure whether it's supposed to be used for public/user-facing messages though, as it's documented with "internal message." We'll have to figure that out more generally - I'd agree that it would be most natural to just pass a message along with it — OTOH, an internal message (for logs) sounds helpful to me, too. (But the current $message does not appear anywhere.)

Changes:

1. Removed/reverted irrelevant phpDoc @param type changes.
2. Clarified base table condition in field_has_data().
3. Clarified the purpose of contact_menu_local_tasks_alter().
4. Replaced verbose todo on Message::$recipient with shorter clarification on its usage/effect.
5. Clarified Message::entityInfo() override for user contact forms.

#41: contact.entity.40.patch queued for re-testing.

I'm not able to reproduce that test failure.

Looks like we have yet another random test failure:

The test did not complete due to a fatal error.
	Completion check	EntityQueryTest.php	272	Drupal\system\Tests\Entity\EntityQueryTest->testSort()

Replaced contact_menu_title_category() with entity_page_label().

With that, I think we should be back to RTBC here.

My stance on the two major issues that have been raised:

1) The question of which entity types can be referenced by an Entity Reference field and other things can reasonably not be addressed or solved by this issue/patch. Due to taxonomy vocabularies, that's a pre-existing problem space in HEAD. All this issue/patch is doing is to add another entity type to the existing stack of entity types. In turn, I don't see that as something to be fixed here.

2) The EntityNG conversion does not seem to be documented and could also involve risks regarding performance and other topics - in general, that API still appears to be in flux and does not appear to be stable enough in order to base major features onto it. There are other major feature requests in the queue that are not based on EntityNG yet. Therefore, I think that conversion should happen in a follow-up issue.

So I hope we can move forward here. :)

Tentatively moving back to RTBC, since I think all review issues have been either fully addressed by code comments or issue comments.

1. Merged HEAD.
2. Clarified comment in system_element_info().

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

I've added all known follow-up issues to the issue summary.

The tab expansion happens for the public/user-facing /contact path, not the administration pages.

Almost identical code is contained in the following three issues:

#1067408: Themes do not have an installation status — expands tabs on admin/structure/blocks/% and admin/appearance/%
#1668292: Move simplified Profile module into core — expands tabs on admin/user/edit/%
#1825044: Turn contact form submissions into full-blown Contact Message entities, without storage — expands tabs on contact/%

The technical reasoning for this code is:

1. There is only one route, which receives a dynamic argument. It is not correct to duplicate route definitions, if the routes are factually identical and only vary by request parameter. Doing so would not only harm overall system performance, but would also make it harder to override the routes.

2. In all of these cases, the dynamic request parameter is solely used to expose a link as local task that has a valid value for the parameter filled in. All of the routes are identical, only a path parameter varies. The router/routing system translates inbound request parameters into routes. However, it is not its job to generate routes. The link expansion happens in a different layer of the system, the presentation layer. Which also means that the expansion only happens when actually needed.

3. Expanding all possible arguments into separate routes in hook_menu() requires and adds a huge chain of dependencies to the router rebuilding. Furthermore, it also adds the reverse: the route definitions depend on a certain, undefined state of the dynamic parameters. Therefore, whenever a contact form category is added, edited, or deleted, the entire router would have to be rebuilt. For no good reason at all, since all of the routes are identical. We don't want and we don't need more of these improper route definitions in core. Quite the contrary, we want and need less of them. Ideally, we eliminate all of them for D8. They caused us nothing but harm in the past, proven by a dozen of major bugs in the core queue.

4. Instead of rejecting patches like this that do things properly, I'd rather recommend that we should move forward with the tab expansion in all of the mentioned issues above. Once they are in place, we will have a better idea of how this algorithm could be abstracted and simplified for implementing modules. And even if that generalization won't happen for D8, we'd at least do things properly. However, given the new routing system in D8, I'm fairly confident that we will have to do significant changes to hook_menu() anyway.

Lastly, note that there is a follow-up issue filed already that asks for a simple configuration option for contact categories to control whether they are exposed as tabs on /contact or not, so they can be linked to and accessed via custom/individual links instead. If the routes were directly expanded in the router, then we'd unnecessarily have to create all the routes, but hide them, so they do not appear as tabs. That logic would make little sense, as it would mean to just add things in order to hide them. Instead, it makes much more sense to only generate and add what should actually be added. The router is the wrong place for that.

Um? How do you access the non-default contact category forms as a user/visitor without tabs?

I'm still not sure I understand, but if I interpret #54 correctly, then the direction is to

1) Remove the exposure of categories entirely for now, and instead,

2) Discuss and evaluate how to allow users to expose links to category-specific forms in a separate follow-up issue?

3) And in the worst case, simply accept that site admins will need to manually create links to specific category forms?

I hope I got that right — works for me.

Changes:

1. Removed contact_menu_local_tasks_alter() / exposure of categories as tabs.
2. Fixed contact message extra fields descriptions duplicate their labels.

Feedback, anyone? :)

Spin-off: #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage

#56: contact.entity.56.patch queued for re-testing.

#56: contact.entity.56.patch queued for re-testing.

#56: contact.entity.56.patch queued for re-testing.

Jesus, we have a dozen different random test failures in HEAD currently. :-/

Summary of follow-up issues

Menu system:
#1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage
#1866998: Introduce tab expansion facility for local tasks that are based on configuration

Entity/Field system:
#1845372: Deleting a field from a non-node entity bundle results in an Entity Field Query Exception

Other:
#1866978: Leverage new #input facility of #type 'item' for author name in comment form

Contact module:
#1849158: Expose contact category-specific forms and/or their URLs somewhere
#1856556: Convert user contact form into a contact category/bundle
#1856560: Store and render contact messages (and add Views support)
#1856562: Convert "Subject" and "Message" into Message base fields
#1856564: Split contact message 'mail' view mode into 'mail' and 'copy'
#599770: Clean up the contact forms listing UI: Allow to set the default category and weights on the listing page

Updated issue summary.

Also: #1867030: Contact message preview appears at random form position after sorting fields in Manage fields (which could use technical/architectural feedback on entity previews, if anyone has ideas)

Thanks. Not sure how that happened.

It does not blow up, because that particular meta info is not used anywhere. It's just simply needlessly registered information.

Contact messages are not stored, and thus they are not listed anywhere. Hence, there's no EntityListController.

We do not need to test code that is not used anywhere and which shouldn't exist in the first place.