Skip to content
Snippets Groups Projects

Issue #2708101: Default value for link text is not saved

Closed Issue #2708101: Default value for link text is not saved
11 unresolved threads
Closed Ben Mullins requested to merge issue/drupal-2708101:2708101-default-value-for into 9.3.x
11 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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() : [];
  • 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.
  • Ben Mullins
  • 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 }
    • Author Maintainer

      Is this check happening to compensate for not using filterEmptyItems()? Could a comment be added to explain that (or whatever the reasoning is to differ from the parent?)

    • is this right, that we don't call parent unless isDefaultValueWidget() is TRUE and the key does not exist? Should this call the parent when isDefaultValueWidget() return FALSE?

    • Please register or sign in to reply
  • 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) {
  • 41 42 $uri_is_valid = FALSE;
    42 43 }
    43 44 }
    44
    45 if (!$uri_is_valid) {
  • Ben Mullins
  • 863 863 return (string) $output;
    864 864 }
    865 865
    866 /**
    • Author Maintainer

      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.

    • Please register or sign in to reply
  • 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 = [];
  • 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,
  • 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);
  • added 185 commits

    Compare with previous version

  • 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,
  • Juan Olalla added 1 commit

    added 1 commit

    • 5106866d - Fix default value overriding the node value in the form

    Compare with previous version

  • 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.

    • Please register or sign in to reply
  • Juan Olalla added 348 commits

    added 348 commits

    Compare with previous version

  • Doug Green added 4591 commits

    added 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

    Compare with previous version

  • shalini jha added 437 commits

    added 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.

    Compare with previous version

  • shalini jha added 1 commit

    added 1 commit

    Compare with previous version

  • Please register or sign in to reply
    Loading