Convert contact_category_list, contact_category_add, and contact_category_edit to new-style Controllers using the instructions on http://drupal.org/node/1800686 . All of those methods can go on the same controller class.

Then, convert the delete form to new-style form controller based on the instructions on the same page.

Related
#1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mkadin’s picture

Assigned: Unassigned » mkadin

*Sets his crosshairs on this one*

mkadin’s picture

FileSize
4.28 KB

Trying to figure out what's wrong with the title callback on the edit page.

Crell’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, contact_routing_2.patch, failed testing.

mkadin’s picture

I've got a few issues / questions:

1) title_callback isn't being passed an upcasted entity. Looks like defining route_name in your hook_menu() means that that 'path' in the menu system has the symfony style path with entity types enclosed in {}. Perhaps we need to postpone this and starta new issue for dealing with this.

2) Also, I can't seem to figure out the local task piece. Should I be defining two separate routes for /admin/structure/contact/manage/{contact_category}/edit and /admin/structure/contact/manage/{contact_category} ? I dropped the title stuff for now and then tried to figure that out. No configuration (one, the other, or both) led to a result where all of the pages returned content and all of the local task tabs were working properly.

Crell’s picture

Default local tasks have always required double-entry in the menu system. That hasn't changed at this point.

As discussed in IRC, we probably should hold on this one until we change how the path gets bridged over, and/or let the menu system deal with {} placeholders.

mkadin’s picture

Status: Needs work » Postponed
jibran’s picture

Status: Postponed » Needs work

As per #7

mkadin’s picture

rerolled, but still not working. On the admin/structure/contact page there's a new error:

Notice: Undefined offset: 4 in _menu_translate() (line 757 of core/includes/menu.inc).

Also going to the edit page for a contact category throws a big ol error. It seems as though #1945418: New-style placeholders in menu_router table break breadcrumbs, menu tree, etc didn't clear up the issue with title arguments / title callbacks.

tim.plunkett’s picture

Heads up, #1983844: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case will add a generic list controller you can use for contact_category_list().

tim.plunkett’s picture

andypost’s picture

dawehner’s picture

Notice: Undefined offset: 4 in _menu_translate() (line 757 of core/includes/menu.inc).

My temporary fix has been to set tab_root to the actual root. The problem is that there is a single router entry with a different path.

+++ b/core/modules/contact/contact.routing.ymlundefined
@@ -4,3 +4,24 @@ contact_category_delete:
\ No newline at end of file

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.phpundefined
@@ -0,0 +1,59 @@
\ No newline at end of file

... missing newline

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.phpundefined
@@ -0,0 +1,59 @@
+use Drupal\Core\ControllerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Entity\EntityManager;
+use Drupal\contact\Plugin\Core\Entity\Category;

Let's sort them.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.phpundefined
@@ -0,0 +1,59 @@
+class ContactController implements ControllerInterface {

Needs docs.

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.phpundefined
@@ -0,0 +1,59 @@
+   * The PluginManager for the contact category entity.
+   * @var \Drupal\Core\Entity\EntityManager

There should be an empty line between them.

andypost’s picture

There's no need in special controller at all

+++ b/core/modules/contact/contact.moduleundefined
@@ -58,28 +58,21 @@ function contact_permission() {
+    'route_name' => 'contact_category_list',

+++ b/core/modules/contact/contact.routing.ymlundefined
@@ -4,3 +4,24 @@ contact_category_delete:
+contact_category_list:
+  pattern: '/admin/structure/contact'
+  defaults:
+    _content: '\Drupal\contact\Controller\ContactController::adminList'
+  requirements:
+    _permission: 'administer contact forms'

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.phpundefined
@@ -0,0 +1,59 @@
+   * Content for contact category listing page.
+   */
+  public function adminList() {
+    return $this->entityManager->getListController('contact_category')->render();

Should be

      _content: '\Drupal\Core\Entity\Controller\EntityListController::listing'
      entity_type: 'contact_category'

as introduced in #1983844: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case

+++ b/core/modules/contact/lib/Drupal/contact/Controller/ContactController.phpundefined
@@ -0,0 +1,59 @@
+  public function categoryAdd() {
+    $category = entity_create('contact_category', array());
+    return entity_get_form($category);
...
+  public function categoryEdit(Category $contact_category) {
+    return entity_get_form($contact_category);

EntityFormController already have $this->entity so enough in routing

+  defaults:
+    _entity_form: contact_category.default
+  requirements:
andypost’s picture

Status: Needs work » Needs review
FileSize
4.03 KB

New patch, somehow got Fatal error: Allowed memory size of 536870912 bytes exhausted (tried to allocate 32 bytes) in core8/core/lib/Drupal/Core/Routing/RouteProvider.php on line 122 for list and add

andypost’s picture

this fixes memory issue but makes admin/structure/contact/manage/feedback/edit 404 so only admin/structure/contact/manage/feedback works

andypost’s picture

nasty hack to make it work

Status: Needs review » Needs work
Issue tags: -SprintWeekend2013, -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, 1938386-contact-route-17.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2013, +WSCCI-conversion, +MENU_LOCAL_ACTION

#17: 1938386-contact-route-17.patch queued for re-testing.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

awesomeness

dawehner’s picture

@andypost
Can you explain why we need two routes even tim's approach on #1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK is to use only one?

andypost’s picture

@dawehner not sure I get how to use 1 route, probably better make it in follow-up

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.91 KB
5.46 KB

Like this.

We should never be adding double routes on purpose.

Also, fixing the double use of default.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Awesome idea.

+++ b/core/modules/contact/contact.routing.ymlundefined
@@ -4,3 +4,25 @@ contact_category_delete:
+contact_category_add:
+  pattern: '/admin/structure/contact/add'
+  defaults:
+    _entity_form: contact_category.add
...
+contact_category_edit:
+  pattern: '/admin/structure/contact/manage/{contact_category}'
+  defaults:
+    _entity_form: contact_category.edit

+++ b/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Category.phpundefined
@@ -23,7 +23,8 @@
  *     "form" = {
- *       "default" = "Drupal\contact\CategoryFormController"
+ *       "add" = "Drupal\contact\CategoryFormController",
+ *       "edit" = "Drupal\contact\CategoryFormController"

Would be nice to add to "policy" issue

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1d26631 and pushed to 8.x. Thanks!

andypost’s picture

+++ /dev/nullundefined
@@ -1,53 +0,0 @@
-  drupal_set_title(t('Add contact form category'));
...
-  drupal_set_title(t('Edit %label contact form category', array('%label' => $category->label())), PASS_THROUGH);

it seem that need follow-up for titles

Status: Fixed » Closed (fixed)

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

Pancho’s picture

Status: Closed (fixed) » Needs work

it seem that need follow-up for titles

Oh yeah, probably - and too bad actually.

tim.plunkett’s picture

Status: Needs work » Fixed

Please open a follow-up issue.

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

Anonymous’s picture

Issue summary: View changes

added related