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.

Files: 
CommentFileSizeAuthor
#13 fix_and_test_revision_handling_edit-2040021-13.patch15.63 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,859 pass(es).
[ View ]
#13 interdiff.txt7.23 KBWim Leers
#10 fix_and_test_revision_handling_edit-2040021-10.patch11.69 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,575 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#7 fix_and_test_revision_handling_edit-2040021-7.patch11.7 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_and_test_revision_handling_edit-2040021-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 edit-2040021-1.patch8.1 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,123 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new8.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,123 pass(es).
[ View ]

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

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

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

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

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.

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

Issue tags:+sprint, +Spark

Adding tags.

Title:In place editing of nodes does not create new revisionsIn-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
StatusFileSize
new11.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_and_test_revision_handling_edit-2040021-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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):

<?php
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.

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.

Status:Needs work» Needs review
StatusFileSize
new11.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,575 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Chasing HEAD.

Status:Needs review» Needs work

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

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.

Status:Postponed» Needs review
StatusFileSize
new7.23 KB
new15.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,859 pass(es).
[ View ]

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

Let's get this done.

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.

Status:Reviewed & tested by the community» Fixed

Awesome! Very nice to have this one put away.

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

Yes! Great to have this out of the way!

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