First, I apologize for not being involved in this earlier on. I've been immersed in other projects for the past few months and became vaguely aware of the forms API just recently (only after I posted the Formproc module). Below are my comments. I should mention that Formproc is the third form-handling library I've worked on so I'm very familiar with how these things work and the common problems and I feel it would be irresponsible not to address a few issues directly. I'm worried that some serious mistakes are being made. I should also mention that I have not yet had time to study the new API as thoroughly as I would like to before this posting, but I think time is of the essence right (potential RC in a few days?) so forgive me if I've overlooked anything. I've done my best not to.
1) MAJOR CONCERNS
a) The function-call-by-naming-convention approach to validation and execution is not good. It duplicates a problem (with flexibility and security) that exists in FormKit, a Python-based form-processing project I was hired to work on.
- Firstly, a user can forge a form with arbitrary ID and call any function of the form x_execute() or x_validate().
- Secondly, where Drupal has a policy of clean API over backwards-compatibility the distinction of public vs. private functions is crucial for establishing some stability, and where the only method currently available for specifying public vs private is through function name ('_' prefix), this convention would seem to undermine the ability to choose whether execution and validation functions are public or private.
- It ties functionality to names which reduces flexibility and code readability.
- Lastly, it clutters the function namespace in a more serious way than the existing hook functions which at least don't suddenly come into being with each form that's created.
I fail to see the advantage of this approach. It yields nothing but headaches in FormKit (where the first problem was solved by having to manually register callable functions...ugh). Formproc requires no such naming conventions as all behavior is modular and specifiable in the form-defining array.
b) Not making filters and other features modular and applicable to an individual field promotes bad (fragmented, not modular) code and inhibits customization/contributions. Check out what Formproc offers in terms of filtering, add-on features and compound fields. I fear the Forms API approach will lead to many custom #types in cases where something minor is all that's needed.
c) If one already has to manually specify a function name in the #valid property, why then append '_valid' to that name? This forces module developers to learn another convention and buys nothing but complication. What it does do is eliminate the ability to use an already-existing function (including a PHP built-in) for validation, and inhibit code re-use and good function naming.
d) Not allowing arguments to be passed to validation functions (am I wrong on this?) severly limits their usefulness. Do we really need to write two different validators if we want a 30 character limit on one field and a 50 character limit on another? Do we have to write different validators if we want different error messages? I hope I'm missing something.
2) OTHER CONCERNS
a) We need increased ability to control error message display. Formproc easily allows either:
- all messages at top of page or
- one general "There are errors"-type message at the top and field-specific messages in red by each field with a problem.
Again, please correct me if this functionality is included and I'm missing it.
b) The date field should be far more flexible. Check out Formproc's datefield (last two examples at the bottom of this page), parameterized like PHP's date() function.
Generally, I think too much emphasis has been placed on writing totally custom aspects of a form when, in my experience, most forms require only a small modifications to the basic elements. Of course, there are ALWAYS exceptions (many which are major) so customization must be easy and powerful, but I believe the Forms API will promote too much un-reusable custom code for common behavior. Basically, it lacks modularity, which is especially problematic for an open source project.
I can't make any real suggestions as I'm not yet totally familiar with the new API, but it seems like adding #filter and #special properties, changing the behavior of the #valid property, and allowing arguments for all of these callbacks (as Formproc does) would alleviate some of the problems, as I see them.
In closing, I don't want this post to be taken the wrong way. Obviously much thought and hard work has been put into the new API and it's definitely an improvement over 4.6. I'm just a little frustrated because, even eager as I am to abandon Formproc in favor of something more integrated, I'd far prefer, as it stands now, to continue using it than upgrade to 4.7. Hopefully I've missed something or someone can talk me into it?
Comments
Some answers
I will deal with this valuable post more in the coming days but some quick remarks:
Regarding namespace, the public vs private is a valid problem, though you can make that clear by prepending a _ before form id / callback. Please observe that currently all core module uses the caller function name as callback / beginning of form id which also helps separating things.
1c) that's a good idea, I'll get rid of '_valid'.
d) #validation_arguments.
2a) Drupal always used error messages on top. If you think this needs a change... this can be discussed. I doubt this can happen for 4.7 though.
2b) Care to submit a patch?
Could you please elaborate on #filter and #special?
We always welcome helpful criticism, no worries. It's sad that you were out of the loop.
--
Read my developer blog on Drupal4hu.
--
Drupal development: making the world better, one patch at a time. | A bedroom without a teddy is like a face without a smile.