In #2574969: Add a Views-like UI for adding fields we are revamping the fields UI to be more usable (and not crash on large sites). I tried to implement it similar to the Views UI, in that changes are first being made and remembered across page requests, but then still need to be explicitly saved once again (and can be discarded until then).

The code is almost completely there (in that issue, not yet committed), but not quite working. Since it isn't integral to that other issue, though, which is so large and invasive that it should be committed ASAP, the "unsaved changes" part will be solved in this follow-up issue.

CommentFileSizeAuthor
#9 2640982-9--fix_fields_ui_unsaved_changes.patch48.76 KBdrunken monkey
#9 2640982-9--fix_fields_ui_unsaved_changes--interdiff.txt4.91 KBdrunken monkey
#7 2640982-7--fix_fields_ui_unsaved_changes.patch53.15 KBdrunken monkey
#7 2640982-7--fix_fields_ui_unsaved_changes--interdiff.txt29.61 KBdrunken monkey
#2 2640982-2--fix_fields_ui_unsaved_changes.patch9.53 KBdrunken monkey

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Related issues: +#2638116: Clean up caching of Index class method results (especially fields)
StatusFileSize
new 2640982-2--fix_fields_ui_unsaved_changes.patch9.53 KB

While I now found the problem and was able to fix it (as well as a few others that were previously covered), I ran into difficulties caused (once again) by our permanent caching of the index's fields. Specifically, even after pressing "Cancel" on the "Fields" form, the discarded changes were afterwards still displayed (and used internally) as the current index state until you cleared the cache. And while that would easily fixed by just resetting the cache when discarding the changes, this also means that those changes will be already visible throughout the code on pages other than the Fields UI, which completely defeats the purpose and is bound to lead to complicated bugs.

So, instead of fixing all of this here, I think we should first solve #2638116: Clean up caching of Index class method results (especially fields), which will hopefully put an end to all those problems, and then here only fix the problems specific to this functionality.

Attaching a WIP patch to later pick things up at this point. (Please don't look at the routing.yml changes, it's too embarassing.) The tests still need to be adapted to expect this behavior, and to also test for it.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Issue tags: +Needs tests

In the tests, we should also be sure to check that discarding changes works fine, parallel changes to other parts of the index will be reflected correctly and that the unsaved changes don't appear anywhere else in the UI.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Postponed » Needs work
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Issue tags: +Release blocker
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Assigned: Unassigned » drunken monkey

Time to finally tackle this.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Assigned: drunken monkey » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2640982-7--fix_fields_ui_unsaved_changes--interdiff.txt29.61 KB
new 2640982-7--fix_fields_ui_unsaved_changes.patch53.15 KB

OK, this should finally fix it, and comes with tests to prove it.
(Contains #2727697-3: Changes to the index are discarded when it is serialized, since that's needed.)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I just went trough this and it looks good, the test is very readable and it passes, so that's great.
I'm happy with the patch, but I do have some very small nitpicks. But not wanting to hold up the issue for that.

  1. +++ b/src/Form/FieldConfigurationForm.php
@@ -33,6 +40,37 @@ public function getFormId() {
+   * @param \Drupal\Core\Datetime\DateFormatter $date_formatter
...
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, RendererInterface $renderer, DateFormatter $date_formatter, RequestStack $request_stack) {

    Can we typehint on the DateFormatterInterface here instead? 

  2. +++ b/src/Form/UnsavedConfigurationFormTrait.php
@@ -0,0 +1,123 @@
+   * @var \Drupal\Core\Datetime\DateFormatter

    Interface here as well.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 2640982-9--fix_fields_ui_unsaved_changes--interdiff.txt4.91 KB
new 2640982-9--fix_fields_ui_unsaved_changes.patch48.76 KB

Needed a re-roll anyways, so I also corrected the class name use.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

  • drunken monkey committed b264b99 on 8.x-1.x 
    Issue #2640982 by drunken monkey, borisson_: Fixed "unsaved changes"...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Great, thanks for reviewing!
Committed.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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