Note: This might be a terrible idea! :)

However, I note in the Symfony docs that it's always _controller: http://symfony.com/doc/current/book/routing.html in Drupal there are at least 4 of these:

_form
_entity_form
_controller
_content

In trying to document these at https://drupal.org/node/2092643/edit I was left scratching my head about why we need to do this. Since all of them are just pointing over to a method, can we just use _controller consistently?

CommentFileSizeAuthor
#20 2092647-20.patch103.15 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Reiterating from irc
_content = needs to be wrapped in blocks - most page callbacks other than drupal_get_form will use this
_form = replacement for drupal_get_form, different to normal pages in so far as there are $form and $form_state parameters for the form builders just like with drupal_get_form
_entity_form = a special case of a form that needs and entity form controller, there is an entity being edited in addition so needs to setup some extra stuff
_controller = bare metal symfony - you can't return a render or form array from these, you need to return a bonafide Response object or similar eg AjaxResponse, JsonResponse and so-on

webchick’s picture

Right, I understand that currently all of those do different things. I'm asking if it's possible instead to use _controller consistently everywhere, instead of making people learn 5 alternatives for what they used to call "page callback." :)

sidharthap’s picture

use of _controller every where will be a good thought.

bappa.sarkar’s picture

+1

dawehner’s picture

Right, I understand that currently all of those do different things. I'm asking if it's possible instead to use _controller consistently everywhere, instead of making people learn 5 alternatives for what they used to call "page callback." :)

There are several reasons, let's first convert an existing _entity_form example:

Before

book.outline:
  path: '/node/{node}/outline'
  defaults:
    _entity_form: 'node.book_outline'

After

book.outline:
  path: '/node/{node}/outline'
  defaults:
    # Note this does actually not work.
    _controller: '\Drupal\Core\Entity\HtmlEntityFormController::content'
    entity_type: node 
    form_mode: book_outline

Is this really a better DX?

Additional this will basically not work with our way how we do ajax support for forms

Related issue: #2068471: Normalize Controller/View-listener behavior with a Page object

Anonymous’s picture

Issue summary: View changes

Wouldn't be _render_array better suited for the purpose instead of _content?

webchick’s picture

No..? That would tie it very specifically to the implementation, which may very well change in Drupal 9 when we no longer use render arrays but now use HtmlFragment objects everywhere (for example). We should instead tie it to the "concept" of what's being output at these paths.

tim.plunkett’s picture

Component: other » routing system
Issue tags: +WSCCI
Crell’s picture

Thanks, Tim. I wouldn't have spotted this otherwise.

Symfony always uses _controller because the callable can only return two things: A Response, or "something else that we hope a view listener knows how to handle". (Which is, in practice, generally an array of values for a Twig template, like a much much much simpler version of a theme() call in Drupal.)

In Drupal's case, we have more specific definitions of "things that can come back", as we wrap the the controller at the controller stage rather than the view stage. That was mandatory when we still had render arrays coming back. HtmlFragment has improved the situation there considerably, though.

Conceptually we COULD move them all to just view listeners, and there is, arguably, some consistency logic to be had there. (Currently we do some things with wrapping controllers, some with view listeners.) However, we still need a way to differentiate them. That means *all* _controllers/_content/callable-things need to return a classed domain object that we can determine what to do with them. HtmlFragment helps a lot, certainly, and the different mime-wrappers for dialog and modal could move to view listeners at this point. (That might even be wise, and non-API-breaking.) Forms and entities, though, are much more problematic. We need to know what's coming back and what to do with it. That means either lots more keys (as dawhener shows) or uniquely identifiable classed objects for all the things.

I'm hardly one to object to using classed objects for all the things, mind you... :-) And I know sdboyer has argued that we should be specifying a class, not method, for the _content and that it should be an object implementing HtmlFragmentInterface. There's actually a fair bit of logic to that. However, all of this is very non-trivial API changes.

So it's not that I don't think it could be done; it's that I don't think it could be done well without significantly impacting the release timeline as it would involve both internal and external API changes.

As a side note, in a sense this is already half-way done. :-) A _controller now COULD return an HtmlFragment itself and everything would be fine. Conceptually, HtmlPageController is at this point just a render array->fragment conversion layer, so if the controller is returning a non-render-array it's unnecessary.

Crell’s picture

Thinking about this further... The only way I could see this working would be the following:

ControllerResolver:

IF _controller is a callable {
  call it and return whatever it is
}
ELSE if _controller is a class name {
  instantiate that class and return it directly (ie, wrap it in a callable that would do so)
}

View listeners (all of them run/are checked until one returns a Response, controlled by priority):

IF $content is FormInterface {
  Call it and turn the result of buildForm() into an HtmlFragment. 
}
IF $content is SomeSortOfEntityClassThingToReplace _entity_view and the Accept type is text/html or dialog or modal {
  Do whatever the black magic is here to turn it into HtmlFragment
}
IF $content is an array {
  assume it's a render array and -> HtmlFragment-ify it
  # this means returning an array for ANY REASON other than "because I didn't get around to making an HtmlFragment" is WRONG.
}
IF $content is HtmlFragment && accept type is text/html {
  Convert it to an HtmlPage (as we do now)
}
IF $content is HtmlFragment && accept type is dialog {
  Convert it to a dialog response (which now happens in a wrapping controller)
}
IF $content is HtmlFragment && accept type is modal {
  Convert it to a modal response (which now happens in a wrapping controller
}
IF $content is HtmlPage {
  Convert it to a Response (this already exists)
}

And so on.

Now, that model actually has quite a bit going for it. It would be more consistent and more flexible than what we're doing now, and I could probably make an argument that it would be more RESTful if I wanted to. (Hell, REST module could return the plugin object itself to the view layer.) Now that I see it laid out that way I kind of like it. :-)

However, it's NOT a small amount of work to do. I am glossing over a number of things above (esp. SomeSortOfEntityClassThingToReplace...), and we run into the potential of an n*m explosion of view listeners. (Eg, entities in dialogs? What happens there?) We would also then be relying on the order in which those listeners run, which is specified manually. (Developing some magical before/after ordering system for listeners and building that on top of the EventDispatcher is *not* in scope for D8.) We can do that, but it requires thought to get set up correctly.

I can't see that happening without delaying beta significantly. With the relatively narrow definition of "Routing API" that I proposed in Austin we could probably do much of it without breaking the API... but we can't change the API (the YAML keys) without doing that work, and it's changing the API that is being proposed here.

So, yes we could, probably, normalize everything to _controller at this point. But not without a significant schedule impact, which I don't want on my gravestone anymore than it already is. :-)

dawehner’s picture

An alternative could be to move the special flakes like _content into the controller string, so you have something like:

_controller: content__\Drupal\foo\Baz

_controller: entity_form__\Drupal\foo\Baz
Crell’s picture

Eh, that just makes the parsing harder and even less standard. Plus we'd still have to go through and modify a crapton of routes. I'm fine with transitioning to the outline in #10 and enabling us to fold everything into _controller over time; I'm not OK with making that a release blocker. (I've got enough release blockers I'm trying to make a case for as is. :-) )

Fabianx’s picture

From IRC, we discussed having controllers implement a certain interface and determining at route compile time:

_controller: \Drupal\Foo\Bar

class Bar implements EntityFormControllerInterface {
}

Then either have a mapping of interfaces to the old keys (if they are still needed), preferably stored in the container OR directly do whatever logic is necessary.

The advantage of interface usage here is:

- Intuitive
- OOP
- Extensible
- Fast
- Contrib can somehow provide more interfaces that can be "enhanced".

and according to @dawehner we already do this for entities anyway, so full win :).

Wim Leers’s picture

Priority: Normal » Major

With #2352155: Remove HtmlFragment/HtmlPage having landed, another step forward has been made: ContentControllerSubscriber no longer does any work, it now just assigns the value of _content to _controller:

  /**
   * Sets _content (if it exists) as the _controller.
   *
   * @todo Remove when https://www.drupal.org/node/2092647 lands.
   *
   * @param \Symfony\Component\HttpKernel\Event\GetResponseEvent $event
   *   The event to process.
   */
  public function onRequestDeriveController(GetResponseEvent $event) {
    $request = $event->getRequest();

    $controller = $request->attributes->get('_controller');
    $content = $request->attributes->get('_content');
    if (empty($controller) && !empty($content)) {
      $request->attributes->set('_controller',  $content);
    }
  }

Therefore it is now already possible to replace all _content occurrences with _controller in core, then only leaving _form and _entity_form.

Bumping to major, tempted to mark critical, because of the DX implications here, as webchick described in the IS.

Fabianx’s picture

Lets do the first conversion in a subtask?

Wim Leers’s picture

Sure, I'm just putting this back on the radar :)

Wim Leers’s picture

Wim Leers’s picture

dawehner’s picture

I think we should keep the BC layer but update our documentation.

dawehner’s picture

Status: Active » Needs review
FileSize
103.15 KB

Funny fact: RouteObjectInterface::CONTENT_OBJECT already exists, even it has a different meaning ...

Here is a full conversion to see primarily whether this breaks something. I already love that people have to learn one thing less!!!!

Wim Leers’s picture

Fully green on the very first try — nice! :)

If you move that into a child issue (see #14/#15), I'll RTBC!

tim.plunkett’s picture

Before RTBCing, this issue title needs to be updated to clarify that it only combines _content and _controller, not _form or _entity_form/_entity_list

webchick’s picture

Title: Consolidate _form, _controller, _content, _entity_form, etc. into just _controller? » Consolidate _controller and _content into just _controller
Status: Needs review » Reviewed & tested by the community

Done.

I'm of course biased, so we should get another core maintainer's sign-off on this change. But to me, according to the graph at https://www.drupal.org/contribute/core/beta-changes, especially if backwards-compatibility is preserved, this impact is way bigger than the disruption here. It means you'll be able to read any random Symfony tutorial, code snippet, etc. and Drupal works exactly the same.

Crell’s picture

Even though it invalidates the trainings and presentations I've given this year on Drupal 8, I also support this move. We can keep _content as a semi-documented BC feature if we want but encourage everyone to go straight to _controller in practice.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Ok, so unlike what #15 asked (to do this initial conversion in a child issue), we've now rescoping this issue to not deal with _form and _entity_form.

If we're doing that, then we must update the code I quoted in #14, because that code says it will be deleted when this issue lands, which is now no longer true.
I'd also argue we should move that code in a new "legacy routing" subscriber or "BC routing" subscriber or something like that.

dawehner’s picture

Issue queue managment for the sake of it <3 #2376791: Move all _content routing definitions to _controller

Fabianx’s picture

Title: Consolidate _controller and _content into just _controller » Consolidate _form, _controller, _content, _entity_form, etc. into just _controller?

Moving title back and added other issue as sub-task of this one.

catch’s picture

I think this is worth doing on impact -> disruption.

Also while this updates a lot of files, in terms of patch conflicts routing YAML is meh on my list.

Wim Leers’s picture

… and such conflicts are super trivial to fix anyway.

tim.plunkett’s picture

Status: Needs work » Postponed

@dawehner in #2368275-33: EntityRouteEnhancer and ContentFormControllerSubscriber implicitly depend on too many services

One really interesting bit about this patch is that it theoretically allows you much easier to not longer have all the special cases:

_form: Drupal\foo\Form\Bar

vs.

_controller: controller.form
form: Drupal\foo\Form\Bar

----------

_entity_form: 'foo.bar'

vs.

_controller: controller.entity_form
entity_form: 'foo.bar'

--------------

_entity_view: 'foo.baz'

vs.

_controller: controller.entity_view
entity_view: foo.baz
Berdir’s picture

Status: Postponed » Active

So..

_content is gone, yay.

I think the conclusion here was that it's not a good idea to merge the remaining stuff together, like _controller/_form/_entity_form/_entity_view. I'm also pretty strongly -1 on that.

Unlike _controller/_content, all of those work differently. _controller is a method, _form a classname, _entity_form is entity_type.form_mode/operation, _entity_view is entity_type.view_mode and so on.

I don't see how the comment in #30 is really an improvement here, instead of one magic key, you have to remember a magic service name (I guess?) + a magic key? It might allow to remove some internal code, but that's not what we were after here?

My suggestion: remove _content from the issue title and mark as won't fix.

andypost’s picture

Title: Consolidate _form, _controller, _content, _entity_form, etc. into just _controller? » Consolidate _form, _controller, _entity_form, etc. into just _controller?

_content is not an option now, +1 to wont fix
Also this change is serious disruption

amateescu’s picture

Status: Active » Closed (won't fix)

I agree with #31 and #32, this change wouldn't give us any better DX and it's certainly not worth the impact at this stage in the D8 cycle.

Fabianx’s picture

I think _entity_view and _entity_form are special as those deal with class names, but _form is unnecessarily so as its just a class name and could implement just an interface (which it probably even does already) to 'upcast' to the current form - and this could still be done, but would need a BC layer.

But leaving at won't fix, as it can easily be re-openend if someone likes for work on something like #13.

dawehner’s picture

I agree with @berdir and @amateescu that getting rid of _form would be problematic, especially if you just read the routing definitions. It helps quite a bit if you know its a form.