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.
Comment | File | Size | Author |
---|---|---|---|
#13 | fix_and_test_revision_handling_edit-2040021-13.patch | 15.63 KB | Wim Leers |
#13 | interdiff.txt | 7.23 KB | Wim Leers |
#10 | fix_and_test_revision_handling_edit-2040021-10.patch | 11.69 KB | Wim Leers |
#7 | fix_and_test_revision_handling_edit-2040021-7.patch | 11.7 KB | Wim Leers |
#1 | edit-2040021-1.patch | 8.1 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettComment #3
tim.plunkett#1: edit-2040021-1.patch queued for re-testing.
Comment #4
Wim LeersThanks! Will review, probably early next week.
Also:
This got in before
FormInterface
existed.Comment #5
tim.plunkettEntityFormController predated this by a good deal. It's way more similar to that, I just went with FormInterface to keep the patch small.
Comment #6
jessebeach CreditAttribution: jessebeach commentedAdding tags.
Comment #7
Wim LeersFirst: apologies that I'm only handling this now, there were always higher priorities to deal with.
Why? Because #111715: Convert node/content types into configuration introduced this change, but failed to apply it to edit.module.
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.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):
I do agree that
EditFieldForm
should implementFormInterface
. 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 . 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 applyFormInterface
toEditFieldForm
.Well, yes,
EditController::fieldForm()
is a controller that always returns anAjaxResponse()
, and because of that we have to do things in a somewhat special way. If you can think of a cleaner way to implementEditController::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.
Comment #8
Wim Leers#7: fix_and_test_revision_handling_edit-2040021-7.patch queued for re-testing.
Comment #10
Wim LeersChasing HEAD.
Comment #12
Wim LeersHrm, 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.
Comment #13
Wim LeersThis should be green. Getting this to green turned my hair gray though.
Let's get this done.
Comment #14
Gábor HojtsyThe 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.
Comment #15
webchickAwesome! Very nice to have this one put away.
Committed and pushed to 8.x. Thanks!
Comment #16
Wim LeersYes! Great to have this out of the way!