Updated: Comment #40
Problem/Motivation
With form.inc as is, we are limited in our ability to write unit tests, and to safely refactor.
We can move the major functions into methods in a service, and can finally remove the faux-private functions (see API changes, below)
Proposed resolution
Introduce a new FormBuilderInterface, and move a majority of form.inc to be methods.
This patch takes us from not being unit testable at all to 70% test coverage of the form builder code.
Remaining tasks
- reviewed by @dawehner and @ParisLiakos
- rtbc by @Crell in #35
- only change after that was a docs re line wrapping comments to 80 chars in #37
- done done
User interface changes
N/A
API changes
API additions:
\Drupal::formBuilder() now contains the drupal_*_form() methods, which are deprecated
API changes:
The following functions are now protected methods, and are not accessible:
- _drupal_form_id
- _form_validate
- _drupal_form_send_response
- form_state_keys_no_cache
The first 3 were already prefixed with _, which was our own special way of saying "NO!"
The fourth, form_state_keys_no_cache(), was very internal.
Related Issues
#2112711: Provide an easier mechanism for using drupal_get_form() directly
Followups
- #2120863: Add docs to core/lib/Drupal/Core/Form/FormBuilder.php
- (done, these are opened) followups to be opened see #32 and #33
- #2120745: Use Symfony Request for Core/Form (from 3.)
- 5. did not need to be opened
- #2120847: Add test coverage for FormBuilder::sendResponse() (from 8.)
- #2120841: Convert form_options_flatten() to a method on FormBuilder (from 18.)
Comment | File | Size | Author |
---|---|---|---|
#37 | form-2112807-37.patch | 250.16 KB | tim.plunkett |
#37 | interdiff.txt | 42.43 KB | tim.plunkett |
#33 | interdiff.txt | 37.38 KB | tim.plunkett |
#33 | form-2112807-33.patch | 250.13 KB | tim.plunkett |
#31 | form-inc.txt | 91.15 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettHere's a start.
Comment #3
tim.plunkettOh right. The installer needs a form :)
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedhell yeah! now one cant use that, unless sub-classing:)
how about adding an interface in the mix, making possible to do all the kinds of stuff in contrib?
Comment #6
Crell CreditAttribution: Crell commentedMake an off handed comment in IRC, Tim writes a 126 KB patch. Life is good. :-)
This should probably be called form_renderer, or something like that. "form" implies it is "THE form for the site", which it is not.
I agree with Paris that this needs an interface. Nearly all services need an interface. :-)
We should probably add a note in the docblock that this is horrible but FAPI can't handle returning the response object at this time. Acknowledge the flaws so you don't get blamed for them later. :-)
This is a service. We can just have this as normal dependencies. No need for \Drupal.
Also, a 1500 line class means something is wrong. I suspect there's a lot of refactoring that could be done to break up the class into more discrete parts. Whether that's something worth doing here, or even worth doing at all if we may be replacing it in Drupal 9, is a debatable point, I grant. If we decide not to, we should document/@todo why it wasn't done.
Comment #7
tim.plunkettThose services are not available on install, and I didn't feel like looking up the optional services notation, I will do that on the next pass.
Renamed Form to FormBuilder (service name of 'form_builder')
Added an interface
Added some unit tests (50% coverage so far)
Moved more code in
Fixed bug from #3
The 8 function_exists hacks at the bottom of FormBuilderTest are sad/hilarious.
Comment #8
tim.plunkettAnd yes, this will need refactoring, but the "1500 line class" is actually 131 blank lines, 695 comment lines, and 683 code lines :)
Comment #9
Crell CreditAttribution: Crell commentedHa! That's competing with the database then on lines of comment vs. lines of code. :-)
Comment #10
jibranTagging.
Comment #12
tim.plunkettThis ditches the getters/settings, except for currentUser().
Comment #13
tim.plunkett@ParisLiakos pointed me to #2048223-61: Add $account argument to AccessCheckInterface::access() method and use the current_user service for how to approach fixing currentUser()
Comment #14
tim.plunkettI'd like to hold off on any crazy changes with currentUser() in this issue, since it is a consistent problem all over right now, and not in scope.
Therefore, I'd like to think of this as being in a committable state, and it is ready for serious reviews.
Keep in mind I see no reason to change or improve docs, since they were 100% copy/pasted.
Comment #15
tim.plunkettI had to restore a couple of the original form.inc functions, as some contrib modules used them.
Comment #16
dawehnerImpressive work!
It would be good to have the request added via setter injection so synching the request later would actually work.
Don't we always add since which version the particular function is considered as deprecated?
I know this is out of scope of the patch though I would like to add the "need" or usage of the different dependencies. For example:
The CSRF token generator to validate the form token.
I know this is c&p though can't we just use the new request object now?
Is there a reason why we don't use the full use statement?
url() to url generator?
Is there a reason why this has to be public?
I really like if we also typehint the actual interface, so we have autocompletion for both
We could also just split up the test into two test functions: testRedirectWithResult testRedirectWithoutResult.
We don't necessarily have to put this into the global namespace
Comment #17
tim.plunkett1) Done
2) Done
3) I did this for csrf_token, http_kernel. I chose to not do it for event dispatcher, url_generator, or module handler, since their usage will likely change over time, and the only thing worse than no docs are wrong docs.
4) I'd rather not mess with that here
5) There are 4 different classes within Drupal\Component\Utility here, and I prefer this to 4 use statements. This is an established pattern in core.
6) Done
7) I originally removed the wrapper and made it protected, but it is used by many tests and a couple contrib modules, so I decided to leave it.
8) Done
9) Done
10) It seems we do have to leave it here, while calling it from the test directly would work, the FAPI code cannot find it. Plus, we're trying to maintain coverage for the legacy functionality here, non-namespaced functions.
0c1ad23 Provide interface typehints for mocked objects.
8b28e2f Split redirect test into two.
0dd986d Expand verbosity of @deprecated
26b66d5 Switch to setter injection for the request.
1ef6e2e Expand explanation of why certain services are needed.
3b2ff97 Remove last usage of url()
Comment #18
tim.plunkettRealized I missed all of the form_set_error/form_set_value stuff too.
Hopefully this doesn't break anything, but that should be the last of the obvious stuff in form.inc.
Also, this needs to stop before the whole thing becomes unreviewable.
Comment #19
dawehnerIt seems to be the wrong approach to move the static variables in all the different form error functions to the service as the service should be stateless.
Is it possible to keep the form errors out for now and think about a better way later?
Comment #20
tim.plunkettYes, we can just go ahead with #17, though I'm interested in seeing the results of #19.
Comment #22
msonnabaum CreditAttribution: msonnabaum commentedThis looks like a nice step forward. The FormBuilder class is doing quite a bit, but that can be factored out later. The cache and validation seem like good candidates for extraction in a followup.
I wouldnt worry too much about the statelessness of this service at the moment. A bigger problem is all the implicit dependencies it has. I'd be nice if more of them were wrapped in local methods like getElementInfo to make them more explicit, keep things testable, and make it easier to refactor later.
Comment #23
tim.plunkettAfter discussing with @msonnabaum more, I decided to go ahead with #18, because its no worse than drupal_static() calls. This is still a 1:1 port, we can clean up if we so choose later.
This gives the getElementInfo() treatment to the other procedural dependencies.
Comment #25
tim.plunkettUgh. No idea how these weird references work, function vs method. So I'm just skipping it and going back to the old way.
Also, injecting t()
Interesting note about PHPUnit.
I was running my tests like this:
But then other test files were loaded, and their conditional functions bled into my test run. Also note the memory used.
@msonnabaum pointed out a better way to run one test:
Same number of assertions, 75% less memory, because only what is needed is loaded. And this one doesn't have the function hacks bleeding through.
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedthere is a getStringTranslationStub method in your base class which does exactly this
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedwe should return it, shouldnt we? i mean the old code used to
same here
Comment #28
tim.plunkett#26, sure but I'm fixing that one then
#27 good call! Those deprecated wrappers weren't actually used so it didn't break.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedSorry, but i didnt do the review with dreditor, it is very laggy and CPU is killing me
1)
Case insensitivity made this hide. lets make the I lowercase
2)
Those test methods are exactly the same?
3)
Utility\Crypt is weird :S why not the full namespace or, even better no namespace and use statement?
It also happens for other Utitlity classes eg Url and NestedArrray in validateForm()
for Unicode in doValidateForm()
4)
Can we use the module handler here?
5)
Should be $this->redirectForm
6)
lets retrieve $path from $this->request
7) Finally if you have set php to show strict errors phpunit testsuite fails now because of the barch_get madness:/
Comment #30
tim.plunkett1) Fixed
2) Yep, that was a bad copy/paste. Removed
3) Ugh, you're the third person to complain about this. We already use this pattern in core, but just to stop having people complain I'm switching to full use statements.
4) Yes, fixed
5) Yes, fixed
6) Fixed
7 is a problem in HEAD, not introduced here.
I finished the test coverage for the changes to _drupal_form_id (now getFormId), and for getCache() since that's where the module_load_include() call was.
Comment #31
tim.plunkettThis patch is pretty damn big, but as I said before, it is mostly copy/paste.
I realize now that I moved the functions into FormBuilder in the order I ported them, and they were in an even different order in FormBuilderInterface.
I've rearranged them both to be in the same order as they were in form.inc. This also let me do a direct diff of the two files, which is really very useful. Keep in mind the docs of the functions that became public were moved to FormBuilderInterface.
Comment #31.0
tim.plunkettupdated.
Comment #32
jibranWohooo great work @tim.plunkett. A lot of doc issues. If you think these things can move to follow up issues please create one or add suggestion I'll create those.
Can we add @see
hook_forms
and some more explanation?Why not
$this->getContainer()
or simply$this->container
?Can we use
$request
here? Instead of$_GET
or$_POST
.80 chars.
I think we should declare
$form
and useempty
/is_array
for check here.80 chars
80 chars
A simple doc line would be great here.
Is this correct? I don't see any call to
menu_get_item()
. Yeah$this->menuGetItem()
is here so should we$this->menuGetItem()
add here.80 chars.
80 chars
80 chars
expand it to 80 chars.
80 chars.
Oh this is the place where magic happens :D
80 chars.
80 chars
Hello OO?
80 chars.
Url::isExternal($path);
80 chars.
@see to the function or @todo to remove it.
80 chars.
Comment #33
tim.plunkett1) Added two @see, there is docs about this in retrieveForm
2) Because FormBuilder is not ContainerAware. Also this is copy/paste
3) Follow-up, please. Copy/paste.
4) Fixed
5) Follow-up, please. Copy/paste.
6) Fixed
7) Fixed
8) Follow-up, please. Copy/paste.
9) menuGetItem() is available, but the actual function underlying that is not. This is still correct.
10) Fixed
11) Fixed
12) Fixed
13) Fixed
14) Fixed
16) Fixed
17) Fixed
18) Follow-up, that isn't needed yet.
19) Fixed
20) Fixed
21) Fixed
22) We don't need to @see it when in this format. And we don't need explicit @todos, because there is a good chance not all of these will go away.
23) Fixed
Comment #34
jibranThanks for the awesome work. For fixing all the doc issues. I don't think there are anymore doc issues. The patch is green with a lot of unit test coverage and I think it is good to go. I haven't reviewed unit test part of the patch. RTBC for me but let others decide it. I think @tim.plunkett should be author of this commit. :). I will add the followups in the morning.
80 chars. If you have to ever reroll the patch.
Comment #35
Crell CreditAttribution: Crell commentedI've reviewed the manual interdiff from #31 and the incidental parts of #33. Since the rest should be just copy/paste I didn't look into it too deeply. This looks like a big step forward, as we can now inject the form builder cleanly, unit test more things, save some code weight on non-form-using requests... All good stuff.
timplunkett++
Comment #36
XanoI couldn't find anything functionally wrong with the patch. Good work, Tim, and thanks! Below are some minor comment issues and some code style nitpicking.
This reads a little awkwardly, plus the assumption makes me doubt it.
80 characters.
80 characters.
80 characters.
80 characters.
80 characters, and many more for FormBuilderInterface::buildForm().
80 characters, and many more for FormBuilderInterface::rebuildForm().
80 characters.
80 characters, and many more for FormBuilderInterface::validateForm().
80 characters, and many more for FormBuilderInterface::redirectForm().
80 characters, and many more for FormBuilderInterface::setErrorByName().
form_set_error() should be \Drupal::FormBuilder()->setErrorByName().
80 characters.
80 characters, and many more for FormBuilderInterface::doBuildForm().
80 characters, and many more for FormBuilderInterface::setValue().
Not very readable with all these associative arrays on one line.
Comment #37
tim.plunkettDocs that were well-wrapped to 80 characters are a huge pain when indented two spaces :)
Comment #38
tim.plunkett#37: form-2112807-37.patch queued for re-testing.
Comment #39
dawehnerStorm should really have a feature to rewrap the comments as we like them to be.
Comment #40
alexpott@catch and I have discussed this and decided that it makes sense to do this refactoring as it is consistent with the direction of Drupal core and the procedural wrappers have not been removed.
Comment #40.0
alexpottUpdated issue summary.
Comment #40.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #41
alexpottCommitted 82eb292 and pushed to 8.x. Thanks!
If you have php configured for strict errors you need the following fix - applied on commit.
Comment #41.0
alexpottUpdated issue summary.
Comment #41.1
tim.plunkettUpdated issue summary.
Comment #42
Gaelan CreditAttribution: Gaelan commentedCreated #2120863: Add docs to core/lib/Drupal/Core/Form/FormBuilder.php. Didn't add #5 in #32 because it would change code flow.
Comment #43
dawehnerAdded some basic one: https://drupal.org/node/2121003
Comment #44
tstoecklerMy Dreditor died on the huge patch, so not providing the actual code lines here, but inside FormBuilder $this->csrfToken and $this->httpKernel get called unconditionally even though they are optional in the constructor. Can someone explain that, it seems wrong? Thanks!
Comment #44.0
tstoecklerUpdated issue summary.
Comment #45
jibranChange notice makes sense to me.
Comment #45.0
jibranadded issue
Comment #46
YesCT CreditAttribution: YesCT commentedI checked, the follow-ups are open. so removing the needs follow up tag. I updated the issue summary.
and https://drupal.org/contributor-tasks/draft-change-notification says after removing the needs change notification, to also change status to fixed. doing that.
If this needs to stay open for something else, please list the next steps.
Comment #48
tim.plunkett#44:
They are only optional because of the installer. AFAICS all of the calls are behind $user->isAuthenticated() checks, so no problems here.
Once someone rewrites the installer, we won't have this problem :)
Comment #49
tstoecklerAhh, thanks for #48, that makes sense. A code comment would've been nice, but yeah...
Comment #49.0
tstoecklermatching followups and noting they are all opened.