Updated: Comment 0
Problem/Motivation
Proposed resolution
Convert exposed forms to use FormInterface
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | form-2147669-10.patch | 27.05 KB | tim.plunkett |
Updated: Comment 0
Convert exposed forms to use FormInterface
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | form-2147669-10.patch | 27.05 KB | tim.plunkett |
Comments
Comment #1
dawehnerLet's see what the testbot tells us.
Comment #2
dawehnerA lot of really great points of tim later.
Comment #4
tim.plunkettmergeDeep *always* confuses me. I do this wrong every time.
When you add a check for form errors to the end of testGetCache you see that the whole method was flawed, and was not actually testing the proper behavior. This was exposed by these changes.
Comment #6
dawehnerperfect!
Comment #7
tim.plunkettThe installer is drunk. I manually tested and it failed, and pointed me to what I fixed in the interdiff. I don't know that you could ever have those args be by reference, but it doesn't seem to hurt to remove it now.
Comment #8
tim.plunkettI take it back, not the installer's fault. The mergeDeep is stripping references. Here's an alternate patch.
Comment #9
tim.plunkettI don't want to trigger another retest until those two come back, so just leaving this here for now:
Comment #10
tim.plunkettFinally remembered this issue, whoops.
Comment #11
dawehnerLet's ask damian for a review.
Comment #12
tim.plunkett10: form-2147669-10.patch queued for re-testing.
Comment #13
dawehnerI am fine with that now.
Comment #14
webchickCommitted and pushed to 8.x. Thanks!
Comment #16
cilefen commented