Posted by catch on July 25, 2008 at 3:15pm
12 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | task |
| Priority: | normal |
| Assigned: | beejeebus |
| Status: | closed (fixed) |
Issue Summary
This has been available since Drupal 6, but on a quick grep, I can't seem to find an example in core. Either way we still have some large hook_form_alter() implementations which could benefit from it - i.e. http://api.drupal.org/api/function/comment_form_alter/7 and probably many others. There's was some conversion in #79551: Multiply form alter but the conversion parts in that patch never got committed after FAPI 3 added the feature.
Incidentally, there's also a '@todo, remove in Drupal 7' in the section of drupal_prepare_form() which deals with this.
One last thing, I can't seem to find any documentation about hook_form_alter_$form_id() anywhere.
Comments
#1
#2
See #331832: Document hook_form_FORM_ID_alter() for documentation.
#3
i'll take a look at this. as a consequence of reviving #332003: cut the fat out of _drupal_bootstrap_full, i noticed that drupal_prepare_form --> drupal_alter('form', $data, $form_id) --> loads lots of modules that don't care most of the time, because they are interested in 1 or 2 forms only.
another bit of drupal that has been loading too much code too much of the time, but nobody noticed because we load all modules at bootstrap anyway, so what's some more function calls between friends? its the $op code again, but in a different bucket.
#4
patch attached.
not as great an improvement as i'd hoped re. loading less code, as many form_alter hooks interrogate $form rather than look at a specific $form_id.
#5
All the changes look good, tests pass. RTBC.
#6
Justin, could you share your before/after number comparisons? You said it was "not as great as you'd hoped" but does it buy us something? If so, how much?
#7
i reviewed the code and saw $form_id = NULL
+function update_form_system_modules_alter(&$form, $form_state, $form_id = NULL) {is there a reasion why to do it, the argument is not there always, there was even a php error when using openid module
there was also some spaces left
PS: whats about left spaces in all drupal core?
#8
sometimes i don't understand it, the patch was there
#9
@dereine: thanks for updating the patch.
@webchick: i didn't do any performance comparisons, but i don't think that's necessary - IMHO just moving to smaller, simpler functions is enough of a gain, as with the $op patches. its also unlikely to show any improvement, memory or speed wise, until we make drupal load less code in via #345118: Performance: Split .module files by moving hooks and #340723: make modules only require .info files. i won't be getting back to those until i've successfully settled in to Philadelphia in a few weeks, but they are on my TODO list.
#10
oh, the testbot likes it, so RTBC.
#11
Ok, fair enough. :)
Committed to HEAD. Thanks!
#12
_openid_user_login_form_alter uses $form_id to distinc between the width of the field.
so there has to be set the $form_id, or it does not work
#13
The code that uses $form_id in _openid_user_login_form_alter is:
<?php$form['openid_identifier'] = array(
'#type' => 'textfield',
'#title' => t('Log in using OpenID'),
'#size' => ($form_id == 'user_login') ? 58 : 13,
'#maxlength' => 255,
'#weight' => -1,
'#description' => l(t('What is OpenID?'), 'http://openid.net/', array('external' => TRUE)),
);
?>
What might be a better method is:
<?php'#size' => $form['name']['#size'] - 2,
?>
Since on user_login, the size is 60, and on user_login_block, the size is 15.
#14
wow there is so much wisdom in drupal community!
is there needed to write a test for this?
#15
Yes, it would be nice to make a couple baseline tests to ensure that the OpenID login works. This is exactly the kind of thing that our testing suite should have found, and will continue not to find until we have test coverage for it.
The fix looks nice and elegant, though. :) But I'm wondering why we need to be -2? Could we make it simple and just use the same width as the user_login form?
#16
i'm just guessing, its because of the openid icon.
But both lines (normal user name and openidname) should have the same width,
is it possible to figure out who wrote this code?
#17
The 'Comment settings' is now completely underneath the 'Save content type' submit button. Weird thing is, adding #weight to the fieldset doesn't affect anything ... not sure if it's because of this patch or not though, just mentioning ...
#18
Followup with comment settings weight in #364484: Comment settings are below the Save content type button.
#19
I have successfully applied the patch from #14 and have manually tested.
Should we create a separate issue to get this particular patch commited? Or is the intent to wait until tests are also built?
#20
I ran the patch and tested it. Worked fine for me. I'd like to apply it before the testing so that the error above can be eliminated:
<div class="form-item" id="edit-openid-identifier-wrapper"><label for="edit-openid-identifier">Log in using OpenID: </label>
<input type="text" maxlength="255" name="openid_identifier" id="edit-openid-identifier" size="58" value="" class="form-text" />
<div class="description"><a href="http://openid.net/">What is OpenID?</a></div>
</div>
Mike
#21
Sorry, changed the overall title. Just wanted to bring attention to this warning:
Notice: Undefined variable: form_id in _openid_user_login_form_alter() (line 117 of /home/dm7/modules/openid/openid.module).
#22
Grumble, mutter...
Yeah, I guess I can commit this. It's bothering me a lot that I'm committing a bug fix without a test to back up that it's fixed. Just means we'll break things again in the future. :( Hope to see you guys at #251245: openid.module needs tests, as eager to help out there and get those committed. ;)
I can't really see a good reason to remove 2 from the size of the parent field, so I removed this from the patch and committed it. If someone finds an instance where this is break (I didn't in clicking through in Garland), let's fix it in CSS, not in the form definition.
#23
Automatically closed -- issue fixed for 2 weeks with no activity.
#24
some functions now take &$form_state instead of $form_state due to patches in this issue, yet I can't find anywhere why this was changed and the functions in question don't change anything in $form_state, so is there any reason to pass it as reference?
-> #503892: hook_form_alter() and hook_form_FORM_ID_alter() take $form_state as reference
also extracted the node_type form for upload to upload_form_node_type_form_alter -> http://drupal.org/node/503780#comment-1748732
#25
not sure if this also applies to profiles, but I don't rly see any reason why not
#26
forgot to remove the $form_id from the default profile
#27
default.profile is now standard.profile. Because of this, I considered setting this to "needs work", but given the age of the issue, I'm setting it to fixed as per #22. Please open new issues for specific changes, such as
standard_form_alter()tostandard_form_install_configure_form_alter(), or re-open this issue if you're including a full sweep of many places.#28
Automatically closed -- issue fixed for 2 weeks with no activity.