The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
I need to get a component id from a webform. I do not know the parent id (pid) and in this case it is irrelevant. There will only be one instance of a form_key per webform.
As pid is not optional we need to give a distinct pid which may not be known.
We should create a new webform_get_cids that returns all cids regardless of pid.
Proposed resolution
Add new fn:
/**
* Given a form_key, determine all cids regardless of parent component.
*
* @param $node
* A fully loaded node object.
* @param $form_key
* The form key for which we're finding cids.
* @return array
*/
function webform_get_cids(&$node, $form_key) {
$cids = array();
foreach ($node->webform['components'] as $cid => $component) {
if ($component['form_key'] == $form_key) {
$cids[] = $cid;
}
}
return $cids;
}
Remaining tasks
Review & commit patches.
User interface changes
None
API changes
None, just extra function.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | webform-get-cid-2825711-31.patch | 5.37 KB | mccrodp |
| #25 | webform-tests-green.png | 41.68 KB | mccrodp |
| #22 | webform-get-cid-2825711-22.patch | 5.34 KB | mccrodp |
| #19 | webform-get-cid-2825711-19.patch | 4.92 KB | mccrodp |
| #17 | webform-get-cid-2825711-17.patch | 5.11 KB | mccrodp |
Comments
Comment #2
liam morlandThe form_key is not unique. For example, you can have elements with the same form_key in different fieldsets. It might be useful for a function like this to return an array of all elements with a given form_key.
It could default to 0, but I think it helps people to understand that the $pid to unique identify a component by form_key. If it is 0, they might not realize that form_key is not necessarily unique.
* @param null $pidThis should be:
* @param int|null $pidComment #3
mccrodp commentedThanks a lot Liam. I didn't realise that the
form_keywas not unique, I just always make it unique myself.I think it's definitely useful to get this functionality in. Do you think it should exist in a separate function then? If so I can submit a patch, what do you think, something like a
webform_get_cids? It could return an array indexed by pid or have the form:array('pid'=>$pid, 'cid' => $cid).At the moment to do the above I have to copy and paste this function and modify it, make an extra call prior to it to find the components pid, etc. Would definitely reduce the need for custom code like this.
I can also make a patch (or PR if we have a GitHub repo?) to make that code comment change you suggested plus I think the long description should detail that form_key is not unique and that 0 is used to find the top level components.
Let me know what you think. Thanks.
Comment #4
liam morlandA simple array of $pid is probably what it should return. With that one can easily get the $pid if one has the $node, which they need to run the function. It should be a separate function. It needs to be submitted as a patch.
Comment #5
mccrodp commentedA simple array of
$cid, ok, I get you, if they have the cid they can do a$node->component[$cid]->pidor whatever it is should they need the$pidvalue. Right, I'll submit a patch for this tomorrow and deal with improving fixing & adding more detail to thewebform_get_cidfunction tomorrow.I will proceed with a new function
webform_get_cids, let me know if you would prefer alternative naming for it. Thanks.Comment #6
mccrodp commentedComment #7
mccrodp commentedComment #8
mccrodp commentedComment #9
mccrodp commentedAdded child issue #2826555: Clarify and correct phpdoc comments for webform_get_cid fn
Comment #10
liam morlandThanks for the patch.
There needs to be a blank line before @return. It should be
@return int[]to indicate that it is an array of integers. There needs to be an explanation of the return value.Put types for both @params. There should be no "&" before $node. It should be possible to put a type hint in the function declaration.
At the top:
Comment #11
liam morlandPerhaps this function should be done as part of webform_get_cid(). If $pid is an integer, return the single integer cid. Otherwise, return an array of cid.
In any case, there should be a test added to test the new functionality. There are not many tests in Webform, but any new functionality should get a test.
Comment #12
mccrodp commentedThanks for the suggestion. I have left as a separate fn just in case this disrupts any contrib modules when making the pid arg optional.
I cannot get Simpletest running on my client project (has some error, the joys of Simpletest) but I've included an example of my thinking so far. Please review and give any feedback before a re-roll another more precise patch. I'll get Simpletest going next time in a Vanilla D7 install.
Comment #14
liam morlandI would like it to be one function to avoid having a function that isn't used by Webform. Previously, it would raise a missing argument error if the $pid was left out, so I don't think there is a problem with changing the function signature to allow it to be NULL and setting that as the default to get the new behavior.
You might find the site_test module will help you run the tests, though contrib module development should be done on a plain Drupal install.
Comment #15
mccrodp commentedYep, I was trying to cut corners as was short for time. Now that it's the same function I will certainly do this in a Vanilla install. Thanks for the tip re
site_test.Yes, I was just thinking about an edge case where some contrib was passing in a NULL in some cases for pid, either by error or poor design. After the commit of this patch they previously would have received a null return value, now they will have an int array. If that's fine, no problem, next patch will have it as part of the
webform_get_cid.Thank you.
Comment #16
mccrodp commentedHere's the latest iteration. Perhaps a default test with all args could be added, but I have converted what I had to the main function:
webform_get_cidwith optional argument and now my previous test is passing.(I don't get how Site Test could have been useful if you have to rename the tests, but perhaps I misunderstood from the explanation. However, it did give me an error after uninstalling it, even after clearing cache, so doesn't seem too stable, I had to reenable it to get my site & tests running again.)
Thanks for review.
Comment #17
mccrodp commentedAdded test for standard use of
webform_get_cidby passing pid and matching against expected return cid value.Comment #18
liam morlandThanks for the patch.
No need for
isset($pid); it has to exist and we already know that is is not NULL."else if" should be "elseif".
The first line of the comments needs to be a one-line explanation. Details can go after a blank line.
$pid should be int|null.
I wonder if it would be better to have the
$pid === NULLcheck outside for foreach and have two foreach.Comment #19
mccrodp commentedThis was in case a
FALSEwas passed as a param. I was checking exactNULLvalue using===, but I've simplified it as requested.Done
done, simplified to 1 line, no need for description as there is extra detail in the params section.
done
done, it will return a blank array if nothing is found in either case. I thought about adding a
FALSE, but wanted to keep the fn small and logic to a minimum.Comment #21
mccrodp commentedThese errors don't seem to be related to the patch, but it helped me realise I created the patch wrongly, based off patch in #16, rather than #17. Going to re-roll now.
Comment #22
mccrodp commentedComment #24
mccrodp commentedAgain, this is passing for me locally and the tests look to be failing on the
WebformConditionalsTestCasetest cases.Comment #25
mccrodp commentedAll tests green, setting this back to "Needs Review"
Comment #27
mccrodp commentedComment #28
liam morlandWhen I run the tests locally, I see errors like the ones shown above. Do you have additional modules installed which change how it works?
Comment #29
liam morlandComment #30
mccrodp commentedI reinstalled Drupal 7 dev and removed Site Test module completely, now I'm seeing these errors. Will take a look, thanks.
Comment #31
mccrodp commentedI changed the condition in the
webform_get_cidfunction toif ($pid === NULL)as==was being entered undesireably when encountering a0, and returning an array, although it should be returning the cid only. I had done this to allow for the case aFALSEwas passed for pid, but perhaps this was unnecessary, so now the operator is===.Comment #32
mccrodp commentedHey Liam, do you think we can get this patch included soon, or any ideas of a timeframe for inclusion? Many thanks.
Comment #33
liam morlandI will try to look at it Friday.
Comment #35
liam morlandThanks.
Comment #36
mccrodp commentedThanks a lot Liam.