Problem/Motivation

From entity.api.php:

- defaults: For entity form routes, use _entity_form rather than the generic _controller or _form.

Unfortunately we do not follow this directive and have some places where we define an entity form through a controller, which then retrieves the form through the entity form builder.

Actually we don't need the controller at all, as _entity_form takes care of everything.

This is also a contributed project blocker. autosave_form gets active only on _entity_form routes. This is the case for the entity.node.edit_form route. However there is the feature request to make autosave_form active also for new entities on the add page. As the node.add route is defined through _controller instead through _entity_form this is not possible.

Proposed resolution

Switch from _controller to _entity_form and deprecate the controller. The change should be completely safe as we don't have any logic inside \Drupal\node\Controller\NodeController::add().

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3084030-13.patch1.68 KBhchonov
#13 interdiff-2-13.txt781 byteshchonov
#2 3084030-2.patch1.38 KBhchonov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented
FileSize
3084030-2.patch1.38 KB
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented
Related issues: +#2924006: Autosave not working on content creation
hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented
Issue tags: +Novice
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer and at TATA Consultancy Services, Solathat for Pfizer, Inc. commented
Issue tags: +Needs change record

+1 to RTBC. probably needs a CR?

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented

Done - https://www.drupal.org/node/3084856

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented 
+++ b/core/modules/node/src/Controller/NodeController.php
@@ -121,6 +121,10 @@ public function addPage() {
   public function add(NodeTypeInterface $node_type) {

We need to trigger the warning pointing to the change record to let people know to stop using it and we also need deprecation tests for this as well.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented 
+++ b/core/modules/node/node.routing.yml
@@ -27,7 +27,7 @@ node.add_page:
 node.add:

We can also move the rotue defination to \Drupal\node\Entity\NodeRouteProvider. Or maybe a followup.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Related issues: +#2723579: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider

Seems like a duplicate of #2723579: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented

This issue here is pretty small and blocking a contrib project. Let's please not mix it up with a much bigger one.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record +Quickfix

I guess we can reroll #2723579: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider on top of this. Setting it RTBC so that we can unblock contrib.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

Discussed briefly with @alexpott, I don't think we need a deprecation test because controllers are internal (it's a similar change to when we add bc for constructors), but this does need a @trigger_error() within the deprecated method.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff-2-13.txt781 bytes
3084030-13.patch1.68 KB

Oh, right. Setting back to RTBC as this is actually a very minor change.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

RTBC++

I agree with this being fine to go in independently of #2723579: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider. In fact that would need to deprecate the controller as well, so at least with that part this issue will actually make that smaller.

  • catch committed 78251c4 on 8.8.x 
    Issue #3084030 by hchonov, jibran: route node.add should be defined...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 78251c4 and pushed to 8.8.x. Thanks!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Published the change notice as well.

Status: Fixed » Closed (fixed)

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