Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
#1913856: Convert Views UI forms to use FormInterface only tackles the non-AJAX forms, because they are much more straightforward.
This attempts to untangle the various AJAX forms that use the Views faux-modal.
The main complication here is the multi-step stacking nature of them. For example, you click to add a new field, you pick two of them, and you go right into configuring the first and the second.
Initially this patch will contain the work done in #1913856: Convert Views UI forms to use FormInterface.
Comment | File | Size | Author |
---|---|---|---|
#33 | 1919142-33-followup.patch | 1.28 KB | lokapujya |
Comments
Comment #1
tim.plunkettIncludes the changes from the other issue.
The diffstat in admin.inc is the important part here :)
Comment #3
dawehnerCan we get for example #1913718: Aggregation settings link generates AJAX error. in first in order to be safe that we don't break stuff again?
Comment #4
tim.plunkettYes, agreed. I'm going to continue working on this until its green, but that should definitely go in first.
Comment #5
dawehnerYeah it's definitive hard to not bike-shed on moves, so I try to give some general comments.
Let's make it public. In general this field should certainly not be part of the Field class ... maybe better the Handlerbase, but it's adding code on runtime, which is actually just needed in the Admin UI. Do you think it's worth to add a small helper class for this?
We should fix this comments-80-chars later.
opened an unrelated follow-up #1919700: [Change notice[ Replace views_ui_cache_set with a method on the ViewUI object
public
See comment below.
... public again.
Stuff like that really look nice!
The docs doesn't seem to match anymore :)
Forms as plugins seems to be 100% over-enginering, but yeah I agree that using pre_replace here is wrong.
Do we really have to rewrite all of that here? This potentially removes the flexibility of the function, without any gain, Oh i see you have type and ID now on the form base class.
It feels just wrong to have both of them, but well.
Comment #7
tim.plunkettI fixed the public on the methods, and the outdated docs on ajaxForm(), nice catch.
We talked about the changes to addFormToStack(), so I'm leaving that in.
The mapping of form_keys to classes is the main problem left to solve here.
The Drupal\views_ui\Form\ViewsUIDelete change I'm leaving up to #1915774: Decide whether core route controllers should generally/always be DIC services or not, but I didn't see the reason to add a service when it didn't need anything injected.
Fixed some of the tests failures.
Also, removed the $third paramater from getStandardButtons(), since it was only used in once place and was really an unneeded abstraction. Also, it only worked for functions.
Comment #9
tim.plunkettMy last bugfix exposed two test inconsistencies, fixed here.
Comment #10
tim.plunkettRerolled on top of the latest patch in the blocking issue.
Comment #12
dawehner#10: vdc-1919142-10.patch queued for re-testing.
Comment #14
tim.plunkettWhoops, missed the validate->validateForm switch.
Comment #16
tim.plunkettComment #17
tim.plunkettOkay, finally removed that preg_replace_callback. Leaving assigned in case I have to fix tests, but otherwise I'd like to be done with this (and the Views UI!).
Comment #18
tim.plunkettTagging.
Comment #19
tim.plunkettThis included the same fix as in #1913718: Aggregation settings link generates AJAX error., so it needed a reroll.
Comment #20
damiankloip CreditAttribution: damiankloip commentedMostly some nitpicks, overall looks great!
Having this logic seems weird?
Same here, to nitpick though, capital Flip. Or are these slightly out of scope for this issue?
Adds
Namespaces
Need to fill this in.
.
I guess we still need this in here?
Nice!
Comment #21
tim.plunkettThere's some new standard that says you don't need the full namespace, even in docs, when its the same namespace. I don't agree, but that's what it is.
I'm not fixing the docs/code that was 100% moved, at least not right away.
I did resolve all of the @todos.
Comment #22
damiankloip CreditAttribution: damiankloip commentedYeah, that seems inconsistent to me. Where is the issue for that? It seems as crazy as removing ALL uses of md5, just for the sake of it ;) Oh well.
Anyway, those changes look good. This is ready. Nice patch.
Comment #23
dawehnerI see the point that this makes it indeed harder for non-IDE folks to follow what's going on. As reading is more important then writing code, I personally on your site.
Comment #24
tim.plunkettI spoke to klausi, a proponent of that standard, and it turns out it was removed because it was an inconsistency. So yay for that.
Just changed those parts of the docblock, leaving RTBC.
Comment #25
tim.plunkettEr, interdiff to prove it
Comment #26
damiankloip CreditAttribution: damiankloip commentedSuper. IMO that was a bad idea anyway :)
Comment #27
webchickOk, this is the last of the major refactorings to bring Views UI into line with D8, so hooray for that. I do have a bunch of things I found when going through, which are detailed below. I think these can be handled in follow-ups though, since Tim was pretty adamant that the scope of this issue be just purely porting what's there, not cleaning it up as well.
My main confusion was around getKey() vs. getFormID() (and there's also $view->id()). I get (now) that one is in for building a URL to the form (the PHPDoc needs to be amended slightly for that) and the other is used for the form itself, but from a module developer perspective it's still really weird that I need to invent two unique strongs that both refer to the same form. It'd be much better if at least one of these could be auto-derived from the class name in order to eliminate boilerplate stuff that's thrust onto humans to figure out. Tim said that was tried with getKey(), but resulted in some pretty gnarly code (see http://drupal.org/files/interdiff_3349.txt).
I also said that it may make sense to move this to the "upstream" form interfaces, because depending on how #1886616: Multistep Form Wizard comes along, it'd be nice if this stacking system could use the same mechanisms rather than having two multistep form creation things in core. However, that's not ready, and this is, so makes sense to move forward on it.
Most of the rest of this is pretty minor, other than please, please, PLEASE open a follow-up to expand on the comments that say things like "// flip." to explain why they are inverting the value of various variables in a most confusing manner. :P~
Committed and pushed to 8.x. Thanks. Other feedback is below, marking needs work for that.
It's odd that most of these end in Form but not rescanThemes. We should make this consistent, no?
(Minor) is it "Exposed" or "Expose"? The name/comment should match.
I realize this is just copy/paste code, but this kind of code/comment combo is not ok. :P Let's get a follow-up to clean this up.
(minor) Let's knock this down to 80 chars (+another para if necessary).
That seems worth a follow-up as well.
It's confusing to me that we have two functions that basically say "give me a unique name of this thing." can we not just re-use getFormID()?
Yet another ID we use in the URL? :)
While the @return does a pretty good job of explaining this, the actual PHPDoc that will show up in IDEs is pretty terrible. Could we expand it to at least have the word "URL" in it somewhere? An example might be good too, to explain that this should be all lower-case with hyphens separating the words.
This cleaned up a lot of boiler-plate code, which is nice. However, it seems like we could clean up even more if we just auto-generated the form key from the class name. Each of these is exactly the same in that regard.
Comment #28
dawehnerOpened a followup: #1933290: Saving anything on handlers isn't stored properly
Comment #29
dawehnerSmall followup: #1933364: Remove the ajax errors in the UI
Comment #30
tim.plunkettComment #31
andypostThis issue blocks #1791352: When changes are made for all displays, mark all displays as changed which is a long standing bug in D7
Comment #32
dawehnerI don't understand why this is blocking it. As webchick wrote in her comment:
Comment #33
lokapujyaThis issue was already committed. Addressing some minor followups here and opening a new issue for a more complicated one.
Comment #35
andypostMethod
\Drupal\views_ui\Form\Ajax\ViewsFormBase::getForm()
contains following linesBut
$view::$forms
- variable$forms
is unknownComment #36
lokapujya@andypost Maybe open a followup for that?
Can #33 be committed or should I move to a followup?
Also, I opened: https://www.drupal.org/node/2676618
Comment #37
andypostChanges make sense!
PS no idea how to name the follow-up
Comment #38
alexpottSo in answer to
In the last 3 years our followup process has become quite different. Please create a new issue with the patch in #33 - the issue title and summary bear no relation to what the issue is doing.
Comment #39
alexpott