Issue #2708101: Default value for link text is not saved
Merge request reports
Activity
181 183 */ 182 184 public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) { 183 185 /** @var \Drupal\link\LinkItemInterface $item */ 186 $default_values = []; 184 187 $item = $items[$delta]; 188 $default_values = !empty($item->getFieldDefinition()->getDefaultValueLiteral()) ? $item->getFieldDefinition()->getDefaultValueLiteral() : []; Can you use the Elvis operator here so that
$item->getFieldDefinition()->getDefaultValueLiteral()
isn't called twice? What does getDefaultValueLiteral()? What are you guarding against here? getFieldDefinition() returning nothing or getDefaultValueLiteral() returning nothing?$default_values = $item->getFieldDefinition()->getDefaultValueLiteral() ?: [];
changed this line in version 2 of the diff
433 437 parent::flagErrors($items, $violations, $form, $form_state); 434 438 } 435 439 440 /** 441 * {@inheritdoc} 442 * 443 * We duplicate extractFormValues() 444 * so that it does not call filterEmptyItems() 445 * when in the default value form. 446 * This way, we can save a default value with only a text and not URI. There are some unnecessary line breaks here, keep the lines going until they exceed 80 chars.
We duplicate extractFormValues()
isn't entirely accurate. It's more accurate to call it an override that largely replicates the functionality of the parent method.The final sentence could be rephrased to something like:
This makes it possible to save a default value for "Link text" without requiring a default value for "Url"
Edited by Ben Mullinschanged this line in version 2 of the diff
454 $key_exists = NULL; 455 $values = NestedArray::getValue($form_state->getValues(), $path, $key_exists); 456 457 if ($key_exists) { 458 // Account for drag-and-drop reordering if needed. 459 if (!$this->handlesMultipleValues()) { 460 // Remove the 'value' of the 'add more' button. 461 unset($values['add_more']); 462 463 // The original delta, before drag-and-drop reordering, is needed to 464 // route errors to the correct form element. 465 foreach ($values as $delta => &$value) { 466 $value['_original_delta'] = $delta; 467 if ($value['uri'] == NULL && $value['title'] == NULL) { 468 unset($values[$delta]); 469 } 41 42 $uri_is_valid = FALSE; 42 43 } 43 44 } 44 45 if (!$uri_is_valid) { 45 if (isset($url) && $url !== NULL && !$uri_is_valid) { changed this line in version 2 of the diff
41 42 $uri_is_valid = FALSE; 42 43 } 43 44 } 44 45 if (!$uri_is_valid) { changed this line in version 6 of the diff
863 863 return (string) $output; 864 864 } 865 865 866 /** Just want to point out there's an existing test:
\Drupal\Tests\link\Functional\LinkFieldTest::testLinkTitle
that confirms the changes in this MR do not cause any regressions in validating the link field widget when used in forms. This looks like a good additional test for targeting the specific changes happening here.
181 183 */ 182 184 public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) { 183 185 /** @var \Drupal\link\LinkItemInterface $item */ 186 $default_values = []; changed this line in version 2 of the diff
233 237 '#type' => 'textfield', 234 238 '#title' => $this->t('Link text'), 235 239 '#placeholder' => $this->getSetting('placeholder_title'), 236 '#default_value' => isset($items[$delta]->title) ? $items[$delta]->title : NULL, 240 '#default_value' => isset($default_values[$delta]['title']) ? $default_values[$delta]['title'] : NULL, This caused the tests to fail, due to "Undefined array key 0" (https://www.drupal.org/pift-ci-job/2158278).
Reverting that.
changed this line in version 3 of the diff
477 // Let the widget massage the submitted values. 478 $values = $this->massageFormValues($values, $form, $form_state); 479 // Assign the values and remove the empty ones. 480 $items->setValue($values); 481 482 // Put delta mapping in $form_state, so that flagErrors() can use it. 483 $field_state = static::getWidgetState($form['#parents'], $field_name, $form_state); 484 foreach ($items as $delta => $item) { 485 $field_state['original_deltas'][$delta] = isset($item->_original_delta) ? $item->_original_delta : $delta; 486 unset($item->_original_delta, $item->_weight); 487 } 488 static::setWidgetState($form['#parents'], $field_name, $form_state, $field_state); 489 } 490 } 491 else { 492 parent::extractFormValues($items, $form, $form_state); changed this line in version 2 of the diff
added 185 commits
-
59f0ffd7...da0ae6a9 - 181 commits from branch
project:9.3.x
- 4f90d421 - Merge branch '9.3.x' into 2708101-default-value-for
- aa642007 - Applying patch 87
- fc915df4 - Reverting wrong change
- 5c4a3c35 - Placing comment in the right place
Toggle commit list-
59f0ffd7...da0ae6a9 - 181 commits from branch
233 236 '#type' => 'textfield', 234 237 '#title' => $this->t('Link text'), 235 238 '#placeholder' => $this->getSetting('placeholder_title'), 236 '#default_value' => isset($items[$delta]->title) ? $items[$delta]->title : NULL, 239 '#default_value' => isset($default_values[$delta]['title']) ? $default_values[$delta]['title'] : NULL, changed this line in version 3 of the diff
added 1 commit
- 5106866d - Fix default value overriding the node value in the form
433 437 parent::flagErrors($items, $violations, $form, $form_state); 434 438 } 435 439 440 /** 441 * {@inheritdoc} 442 * 443 * Override extractFormValues() so that it does not call filterEmptyItems() 444 * when in the default value form. This makes it possible to save a default 445 * value for "Link text" without requiring a default value for "URL". 446 */ 447 public function extractFormValues(FieldItemListInterface $items, array $form, FormStateInterface $form_state) { 448 if (!$this->isDefaultValueWidget($form_state)) { 449 parent::extractFormValues($items, $form, $form_state); 450 } 451 else { In reviewing this again, I don't like that we're duplicating so much code here ... but I haven't dug in enough to find an alternative solution.
I don't know what core maintainers would suggest. If this was a contrib module, I'd simply add a better comment here to indicate that this is being copied from somewhere else, and highlight the changes. I'd also put a return in line 449 so that we don't need the else.
But ideally I think we need to find a better place to hook or a refactor to extractFormValues() that allows these changes without having to duplicate this code.
I think we all agree that duplication is not ideal. I couldn't find an easy refactor, I think refactoring this would imply to touch a lot of code, probably starting with the base extractFormValues() function which would impact on so many parts...
The comment is quite explicit though, in my opinion it's clear that we're overriding extractFormValues() to just remove the specific piece we are mentioning.
added 348 commits
Toggle commit listadded 4591 commits
- 72db37bc...eeaf9c82 - 4581 earlier commits
- 1b5c7db4 - Issue #3454960 by quietone, nod_, larowlan, rkoller, smustgrave: Update to jquery UI 1.14.0 beta 2
- 9e83195c - Issue #3393400 by mglaman, finnsky, m4olivei, bnjmnm, plopesc, ckrina,...
- d49ffa40 - Issue #3361728 by quietone, longwave, xjm: Make 10.x EOL warning better than the 9.5.x one
- b97be27a - Issue #2990588 by Utkarsh_33, andrewmacpherson, oriol_e9g, smustgrave, shaal:...
- 77b8e596 - Issue #3450773 by ahsannazir, Kanchan Bhogade, Gauravvvv, kostyashupenko,...
- fa1b1ef0 - Issue #3455557 by mstrelan: Remove ThemeInitialization::resolveStyleSheetPlaceholders
- 74249dc9 - applied patch 78
- d4fdb51f - Applying patch 87
- 4910a640 - Placing comment in the right place
- 8b03e698 - Fix default value overriding the node value in the form
Toggle commit listadded 437 commits
- 8b03e698...f9d14651 - 427 earlier commits
- 81f055eb - Issue #3477329 by phenaproxima, thejimbirch, b_sharpe: Recipe validation...
- ce398e86 - Issue #3473203 by mstrelan: Fix use of ConstraintViolation instead of...
- 4e9a6028 - Issue #3083507 by oknate, jungle, quietone, neelam_wadhwani, longwave, xjm,...
- 9c0bd377 - Issue #3478281 by pere orga, nayana_mvr: CKEditor 5 has its own border color...
- 0ba8488d - Issue #3262156 by finnsky, kostyashupenko, ranjith_kumar_k_u, gauravvvv,...
- f7bb4b6f - applied patch 78
- 8a1ed78d - Applying patch 87
- e1ff703d - Placing comment in the right place
- 38b46b65 - Fix default value overriding the node value in the form
- 44f30b77 - #issue-2708101: Fixed pipeline failure.
Toggle commit list