Problem

Steps to reproduce:

Go to admin/structure/types/manage/article and check "Create new revision" under Publishing Settings
Create a new article node
Use "Quick edit" to change the node
Click the Revisions tab for that node

Expected result:

A new revision was made with the log message of "Updated the Body field through in-place editing." or similar

Actual result:

No revisions tab, no revision created

Cause

The edit.module still checks variable_get() instead of the node type config entity

Fix

Load the entity and check.

However, the edit.module does insane things to build this form that isn't even really a form, but a class with arbitrary methods that mimic EntityFormControllerInterface.
Because it's not a real form, it calls drupal_build_form() directly.

This patch restores some sanity, but because of the need to mess with $form_state directly, is still hacky. I'll open a follow-up to turn this into a real entity form.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
8.1 KB

Status: Needs review » Needs work
Issue tags: -Needs tests

The last submitted patch, edit-2040021-1.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

#1: edit-2040021-1.patch queued for re-testing.

Wim Leers’s picture

Thanks! Will review, probably early next week.

Also:

However, the edit.module does insane things to build this form that isn't even really a form, but a class with arbitrary methods that mimic EntityFormControllerInterface.

This got in before FormInterface existed.

tim.plunkett’s picture

EntityFormController predated this by a good deal. It's way more similar to that, I just went with FormInterface to keep the patch small.

jessebeach’s picture

Issue tags: +sprint, +Spark

Adding tags.

Wim Leers’s picture

Title: In place editing of nodes does not create new revisions » In-place editing of nodes does not create new revisions (when the content type is configured to create new revisions by default)
Assigned: Unassigned » Wim Leers
Issue tags: -Needs tests
FileSize
11.7 KB

First: apologies that I'm only handling this now, there were always higher priorities to deal with.


Cause

The edit.module still checks variable_get() instead of the node type config entity

Why? Because #111715: Convert node/content types into configuration introduced this change, but failed to apply it to edit.module.

However, the edit.module does insane things to build this form that isn't even really a form, but a class with arbitrary methods that mimic EntityFormControllerInterface.

EntityFormControllerInterface is for full entity forms. This is not a full entity form. As the method name (EditController::fieldForm()) already indicates, as well as the class name (EditFieldForm), this is not about an entity, but about an individual field.

EntityFormController predated this by a good deal. It's way more similar to that, I just went with FormInterface to keep the patch small.

As per the above, EntityFormControllerInterface is inherently a wrong choice.

You further complained in #2073167: edit_field_form() and EditFieldForm are a pile of hacks (now merged with this issue):

function edit_field_form(array $form, array &$form_state, EntityInterface $entity, $field_name, TempStoreFactory $temp_store_factory) {
  $form_handler = new EditFieldForm();
  return $form_handler->build($form, $form_state, $entity, $field_name, $temp_store_factory);
}

What?

This class is just a bad copy of FormInterface

I do agree that EditFieldForm should implement FormInterface. But it didn't exist yet when this code got committed, back in December 2012. effulgentsia set up this way of doing things in #1824500-54: In-place editing for Fields, where he said I got inspired to refactor it to D8-style OOP. Who is more familiar with the latest best practices than effulgentsia? He wrote the above.

FormInterface was introduced in February 2013, by you (and thanks for that! :)). Much like your first problem with edit module (retrieving node type configuration using the old API) was caused by those introducing the new API failing to apply the new API consistently wherever the old API was used, here too it was forgotten to apply FormInterface to EditFieldForm.

Because it's not a real form, it calls drupal_build_form() directly.

Well, yes, EditController::fieldForm() is a controller that always returns an AjaxResponse(), and because of that we have to do things in a somewhat special way. If you can think of a cleaner way to implement EditController::fieldForm(), feel free to provide suggestions (after this patch has landed would probably be best).


In summary, yes, Edit is doing some things in a way that does not (yet!) follow the latest best practices. Yes, some of those best practices have been around for a long time. Yes, Edit should use them. Yes, we will fix what you rightfully complained about here.

But … the fault not necessarily lies with Edit module. When changing existing APIs, or enforcing new APIs across Drupal core, it's up to those making these changes to apply them consistently. As shown here, the big complaints in this issue all boil down to those introducing new/changed APIs failing to apply them elsewhere.

Edit module was the first module in Drupal core to do many things. E.g. it was the first to use routes that used arguments. Then it got scolded for doing some things not entirely correctly. But there were zero docs when implementing it the first time around, and best practices were not even established.


Attached patch is based on #1, but with its bugs fixed, rerolled against latest HEAD, and with test coverage.

Wim Leers’s picture

Issue tags: -sprint, -Spark

Status: Needs review » Needs work
Issue tags: +sprint, +Spark

The last submitted patch, fix_and_test_revision_handling_edit-2040021-7.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
11.69 KB

Chasing HEAD.

Status: Needs review » Needs work

The last submitted patch, fix_and_test_revision_handling_edit-2040021-10.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Postponed

Hrm, this will conflict with #2074037: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests anyway, and that issue's been RTBC already, so postponing on that.

Wim Leers’s picture

Status: Postponed » Needs review
FileSize
7.23 KB
15.63 KB

This should be green. Getting this to green turned my hair gray though.

Let's get this done.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

The code now uses the new node type storage proper and has extensive added tests. Great fix! I did not find anything to complain about :) Implementing the FormInterface proper and moving to a more standard injection system is also very welcome and seems like in the scope since the node type service needed to be newly injected.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome! Very nice to have this one put away.

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

Yes! Great to have this out of the way!

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