Problem/Motivation
$form['checkbox'] = [
'#type' => 'checkbox',
'#title' => t('Show link'),
];
$form['link'] = [
'#type' => 'link',
'#title' => t('Link to front page'),
'#url' => Url::fromRoute('<front>'),
'#states' => [
'visible' => [
':input[data-drupal-selector=edit-checkbox]' => ['checked' => TRUE],
],
],
];
Expected output:
<a href="/" data-drupal-selector="edit-link" data-drupal-states="{"visible":{":input[data-drupal-selector=edit-checkbox]":{"checked":true}}}" id="edit-link">Link to front page</a>
Actual output:
<a href="/" data-drupal-selector="edit-link" id="edit-link">Link to front page</a>
This appears to be because \Drupal\Core\Render\Element\Link::preRenderLink
moves the attributes into #options['attributes']
, which would be fine... if drupal_process_states()
(which adds the states to the element's #attributes
) was invoked before pre_render callbacks were invoked in \Drupal\Core\Render\Renderer::doRender
.
Proposed resolution
Move the invocation of drupal_process_states()
before pre_render callbacks.
Update Render/Element/Link.php element adding support to js states.
Remaining tasks
none
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|
Issue fork drupal-2820586
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
markhalliwellComment #3
markhalliwellComment #4
markhalliwellComment #5
dawehnerJust an idea. Maybe add
drupal_process_states
to\Drupal\Core\Render\Element\Link::getInfo
and drop#states
at the end?This technique allows render elements to provide the processing at the exact right position.
Comment #6
markhalliwellIf it were a
\Drupal\Core\Render\Element\RenderElement::preRenderStates
method, maybe. However,drupal_process_states
is currently a procedural function; one that doesn't return the element which would be needed to be a#pre_render
callback.I'm not really sure what you mean here. This patch moves the invocation before the
#pre_render
callbacks are invoked because it needs to add the necessarydata-drupal-states
attribute before\Drupal\Core\Render\Element\Link::preRenderLink
is invoked, which effectively ostracizes the#attributes
array afterwards.Comment #7
markhalliwellIt's also unfortunate and perhaps partially confusing that the name of the procedural function (due it's name in 7.x) is
drupal_process_states
.It is really just a
#pre_render
type of modification. If it's going to get added to the element info, then it would need to be a#pre_render
callback since it's currently executed in\Drupal\Core\Render\Renderer::doRender
(always invoked) rather than a#process
callback which would only ever be executed in\Drupal\Core\Form\FormBuilder::doBuildForm
.However, keep in mind that moving it to a
#pre_render
callback would require manually adding it to the getInfo array of an element as well. This would technically be a BC break since the current code will always check if#states
exists (on any type based element or standalone render array).Comment #8
markhalliwellComment #18
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRe-rolled #4 for 9.3
Comment #19
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedre-roll patch #18, Attached interdiff for same.
Comment #23
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This will need a test case to show the issue
Did not test patch yet.
Comment #24
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedAdding unit test for field links using states
Patch #19 didn't work with me, it was necessary to do a small fix in state.js, see changes interdiff file
Comment #25
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedComment #26
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedComment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedTested on Drupal 10.1.x with a standard install.
Picked a random module with a form, in this case admin_toolbar, and added
Verified the data-drupal-selector wasn't there and the link was appearing.
Applied the fix and the attribute was there and the link was hidden until I checked the box.
Good job with the tests!
Comment #28
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedUpdating attribution
Comment #29
longwaveThis feels a bit too hardcoded, and the comment doesn't explain *why* we are doing this. Maybe we should add
.js-form-item
to the link itself when#states
is used, then I think we could leave the JavaScript alone?In #994360: #states cannot check/uncheck checkboxes elements we added a third round of tests in this method to ensure the state was correctly reset after unchecking the checkbox again; we should add the same check for this change.
Comment #30
myha CreditAttribution: myha commentedI made that work for 9.5 by add 'container' element and put states here. Link added inside 'container'.
Comment #31
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedWorking on issues
Comment #32
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedHello @longwave,
I added the third round validating the checkbox. Also, I updated the commend on state.js explaining better why we update the element.
I agree with you that's is too hard-coded. But at the same time, in this case, should be hard coded, because we need to check if the element is a link tag. What do you think if we keep it as it is but explain why we are doing this?
About your suggestions I think is a good idea, maybe we should have a new js class to state.js handle these cases instead of checking the element tag, in the future, we could have more elements using this feature by just adding this class. My only concern about this idea is the link state won't work out of the box, we should add a new class to the link form element. I guess that we can't use the already existing class(js-form-item, .js-form-submit, .js-form-wrapper) because the links are not wrapped by them.
Regards.
Comment #38
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedComment #39
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems the points brought up in #29 have been addressed.
Comment #41
longwaveThe comment for the A tag case needs rewording, but I'm not sure what to suggest.
Comment #42
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedComment #43
smustgrave CreditAttribution: smustgrave at Mobomo commentedChange makes sense.
If the committer get to it first need to remove duplicate // from the states.js, but didn't think it was enough to hold up.
@joaopauloc.dev if you see this before the committer mind removing that please.
Thanks
Comment #44
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedRemoved.
Thanks @smustgrave.
Comment #45
lauriiiAdded comment to the MR
Comment #46
smustgrave CreditAttribution: smustgrave at Mobomo commentedLeft a comment back.
Comment #47
quietone CreditAttribution: quietone at PreviousNext commentedI'm triaging RTBC issues.
After reading the issue summary I wonderws if drupal_process_states, mentioned in the proposed resolution, was still in use. So, I searched and found and found that it was deprecated in 2019, #3000068: Deprecate drupal_process_states(). I am tagging for an issue summary update.
Reading the comments, I see that @lauriii has asked for a less risky solution. I think that warrants setting this back for work.
I am setting back to NW.
Comment #49
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedComment #51
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedComment #52
smustgrave CreditAttribution: smustgrave at Mobomo commentedRemoving summary update tag as that was done in #49
Comment #53
lauriiiNice! The current approach looks less scary. I posted few comments on the MR but I think we're getting there
Comment #54
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedComment #55
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeemed to have some test failures so rerunning
Comment #56
smustgrave CreditAttribution: smustgrave at Mobomo commentedHave reran the tests a few times but seems something is breaking.
Comment #57
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedFixed the unit test that was failing.
Comment #58
smustgrave CreditAttribution: smustgrave at Mobomo commentedDidn't see a test-only patch and the test-changes feature of gitlab is blocking me.
So ran locally without the changes to core/misc/states.js core/lib/Drupal/Core/Render/Element/Link.php
Great!
So reviewing the change itself, shame we need specialty code for links in the states.js but confirmed it does work using the example in the IS.
All threads appear to be resolved.
Great work!
Comment #59
longwaveAdded some comments about the comments.
Comment #60
joaopauloc.dev CreditAttribution: joaopauloc.dev at ImageX commentedComments fixed again.
Comment #61
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems threads have been resolved.
Tested again following the IS.
Took that code snippet and added to the Performance Form.
States did not work, as both elements rendered.
Applied the MR and states work perfectly.
Comment #63
lauriiiCommitted 9b3d27c and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!