If you are attaching some option attributes to a link field (using a custom module for example), those attributes are removed on node save.
One solution is to use link_attributes, but you might not want to allow users to edit those attributes from node edit.
LinkWidget::formElement() puts the options attributes into $element['attributes'], which is a value type element. Upon submitting, LinkWidget::massageFormValues() does not use this value. It is effectively discarded.
Since there's no form element for the attributes in the LinkWidget the easiest way to reproduce the issue is by using Drush.
drush eval '$node = \Drupal\node\Entity\Node::load(NID);$node->FIELD_MACHINE_NAME->options = ["attributes" => ["class" => "test-class"]];$node->save();'drush eval '$node = \Drupal\node\Entity\Node::load(NID);var_dump($node->FIELD_MACHINE_NAME->options);'Expected result:
The options should be the same as when you loaded them in step 4, which should look like this:
array(1) {
["attributes"]=>
array(2) {
["class"]=>
string(17) "test-class-edited"
}
}
Actual result:
The options array is empty:
array(0) {
}
Update LinkWidget::massageFormValues() to merge in the submitted attributes. This is tricky to do in a backward-compatible way, maybe impossible. A previous attempt to solve the issue by changing the element structure in the form broke a few modules that deal with link attributes. It may be impossible to fix this issue in a way that is backward compatible for all contrib modules.
A bug that prevented link attributes from being saved by the link_default widget has been fixed. This change may prevent extending widget classes from working correctly. If you maintain an extending widget that works with link attributes, then you should test that widget to ensure that this change does not disrupt that functionality. Site builders who use attribute-setting modules should test their content to ensure that those attributes continue to work as expected.
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes
Comments
Comment #2
aalin commentedchanging $element['attributes'] = .. to $element['options']['attributes'] is fixing this
Comment #3
aalin commentedComment #4
idebr commented@aalin Can you add a test case showcasing the current failing behavior?
Comment #5
aalin commented1. Set some custom option attributes to a field (consider that the field is not empty):
2. check that the custom options exists:
this will output: [data-ga-event] => my-event
3. go to: /node/1/edit and save
4. check again custom options on field_link:
this will output NULL
Comment #6
mashermike commentedComment #8
mashermike commentedComment #9
mashermike commentedComment #15
ranjith_kumar_k_u commentedRe-rolled #8 for 9.4.
Comment #17
yogeshmpawarHope updated patch will fix test failures.
Comment #19
yogeshmpawarAnother try to fix test failures.
Comment #21
smustgrave commentedThis looks valid. and the test cases seem to pass.
Comment #22
smustgrave commentedComment #23
catchCommitted/pushed to 10.1.x, cherry-picked to 10.0.x, 9.5.x, 9.4.x, thanks!
Comment #25
maxstarkenburgI'm finding that this change is breaking some link-related contrib. For example,
link_classandlink_targetno longer allow edits to their respective attribute values once a field has been saved the first time (#3301937: Link classes can't be edited/updated after core 9.4.5 update and #3302123: Targets can't be updated after core 9.4.5 update (ok as of 9.4.6)), andmenu_link_attributeshas a more "additive" problem now, with replacement values just being concatenated onto the previous one (#3302105: As of core 9.4.5, can't remove link attribute values once applied). Haven't (yet) testedlink_attributesand other modules I might not be aware of that might be affected.I don't know if the onus is on core to make a further change for this issue to work better with these modules, or whether it's on contrib to "get with the [new] program", so to speak, but in any case, since I have some affected sites where I can't just remove these modules, and also because I'm not sure about those modules' maintenance statuses, I've made a patch for core that basically reverts the change made here, to use in the meanwhile.
Comment #26
maxstarkenburgAlso, please let me know if best practice in a case like (especially where the change was already released) is to file a new issue instead of tacking on comments to the originating one?
Comment #27
smustgrave commentedThink it's best to open a new issue referencing this one.
Comment #28
maxstarkenburgThanks @smustgrave, I've created #3302472: Multiple link-related contrib modules broken by #3056652 to address the breakage (should probably port the patch over there too, though am already using the above one on a few of my composer.json files, heh).
Comment #29
idebr commentedRestoring the issue status to 'Fixed' as a follow-up issue is available at #3302472: Multiple link-related contrib modules broken by #3056652
Comment #30
idebr commentedComment #32
catchI've just reverted this from the 9.4.x branch, so those modules will work again with no changes in the next patch release of 9.4.x. If the contrib modules need adapting to work with both versions of the form structure, then they should end up working with the (old) 9.4.x and (new) 9.5.x versions anyway.
Comment #33
catchComment #34
catchfwiw if an issue has very recently been committed and introduced an obvious regression, it's OK to re-open it for a revert. For every other case, it's better to open a follow-up.
I thought about this some more and went-ahead and reverted from all branches.
If the contrib module just need to adapt to the new form structure, we can possibly re-commit this to 10.1.x-9.5.x with a release note and change record.
If this was a form element I probably would have thought about this before commit, but I wouldn't have expected contrib modules to be relying on a #type => value that wasn't working in core, so it'd be good to understand why the contrib modules broke due to this.
Comment #36
segx commentedPatch #25 seems to be implemented and the problem is fixed in the 9.4.6 update. Please verify ... thank you!
Comment #37
maxstarkenburg@segx I did a quick test with
menu_link_attributeson a site after updating to core 9.4.6 and can verify that the problem is now absent with regard to that module (I would assume the other modules affected are now fine again too). The original issue posted at top however is, I suppose, back to square one.Comment #40
dcam commentedUpdated the IS.
Comment #42
dcam commentedI've verified that MR 13299 is compatible with the
link_class,link_target, andlink_attributesmodules. It is NOT compatible withmenu_link_attributes.They aren't relying on the value. They are relying on the
$element['options']['attributes']render array keys. There's no problem while the Core widget sets$element['attributes']. Nothing else uses it. But the contrib modules' widgets all set up their render arrays to match the expected structure of a URL's options array. They make actual form elements at$element['options']['attributes']. When Core suddenly started doing it too everything broke, probably because Core set it as avalueelement. But then you had a module likelink_attributesturning it into adetailswith child elements for the different attributes. It's a wonder that they weren't more broken than they were. That's why I decided to leave the Core widget's structure as it has been. Instead, I'm merging the attributes into the field value inmassageFormValues().menu_link_attributesis different because it merges its attribute values in at https://git.drupalcode.org/project/menu_link_attributes/-/blob/8.x-1.x/m.... So we end up with the same "additive" problem described in #25. I don't know if something can be done about this. I'm looking for other opinions, so I'm setting this to Needs Review.I haven't written a change record pending a decision on a final approach to fix this.
Comment #43
dcam commentedTagging for framework manager review due to the issue with contrib compatibility described in #42.
Comment #44
smustgrave commentedQuestion is there anything we can do to not break menu_link_attributes and still fix the bug? And deprecate the BC way to give that module time to update? Know we shouldn't let contrib dictate a core fix but that module does have 82K downloads.
Comment #46
smustgrave commentedThis one appears to need a rebase. I was about to mark it RTBC just to get into the queue to get in front of a manager.
Comment #47
dcam commentedAmazingly, the Update Fork button worked to rebase the MR, even though it was more than 1000 commits behind.
I don't think so.
I went ahead and added the change record and release notes snippet. I didn't give before/after examples because any changes that need to be made by extending widgets or modules will be implementation-specific. There's no one single thing that anyone needs to do in order to update their code.
Comment #48
smustgrave commentedProbably still needs framework manager sign off but since it's been 6 months going to RTBC to hopefully get some attention.
Comment #49
catchCan we open an issue again menu_link_attributes? Ideal thing would be if they can implement things so it will work for both before and after this commit in a new release.
Comment #50
smustgrave commentedOpened #3587518: Potential breaking change from core
Comment #51
quietone commentedI updated the branch in the MR and reworked it so the change is the first item. I also make a heading for the action maintainers and site builders should take so it is easier to find. Therefore, the change record needs review.
Adding release note tag so we can inform maintainers/site builders.
Comment #52
catchCouple of minor comments on the MR.
Comment #53
dcam commentedFeedback was addressed.
Comment #54
mohit_aghera commentedAll 3 feedback items are addressed.
Moving backed to RTBC.
Comment #55
larowlanLeft a comment on the MR regarding a bug this introduces.
We need to expand test coverage for that case as well.
The good news is I ran the link_attributes test suite against this branch and it passed.
link_attributes has a widget that extends this one AND adds support for attributes via a UI.
My gut feeling is to close won't fix this and ask people to use that module - that's what it's for!