#2417793: Allow entity: URIs to be entered in link fields added validateUriElement() as a static '#element_validate' for the 'uri' element.
The class already had a validateTitleElement() method, which is not static.

- we should be consistent
- the current practice throughout all core widgets is to have FAPI helpers as static methods unless there is an explicit reason not to (i.e the method needs to rely on the state of the object). See #13 for details.

--> the pre-existing validateTitleElement() helper should be moved to a static, just like the recently added validateUriElement().

Patch additionally moves the two methods closer together, for better internal organization.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task : internal code consistency (better understandability), avoiding needless form serialization overhead
Issue priority Normal
Unfrozen changes Unfrozen because it only changes ...
Prioritized changes This is not a prioritized change for the beta phase.
Disruption none

Original report by yched

In LinkWidget::formElement():

$is_field_edit_form = ($element['#entity'] === NULL);

This doesn't work, there always here at least a dummy entity.

Also,
- the validateTitle() helper #element_validate has no reason not be be static.
- validateTitle() does $element['title']['#required'] = TRUE;, but a #element_validate callbeck seems like a weird place for doing this ?
- isn't it weird that the "requiredness" of the title is only checked by the widget, but not by field validation ? What about REST / programmatic entity saves ?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
1.6 KB

Patch:
- uses the right check for "are we on Field UI's 'default value' widget"
- moves validateTitle() to a static

Does nothing about $element['title']['#required'] = TRUE; in the #element_validate callback, I'll leave it to maintainers to decide :-)

yched’s picture

Also, isn't it weird that the requiredness of the title is only checked by the widget, but not by field validation ?
What about REST / programmatic entity saves ?

yched’s picture

Title: LinkWidget wrongly checks "is this the 'default value' widget" » Link "title" validation is wonky

So, broader title.

andypost’s picture

Issue tags: +Needs tests

Seems this needs test

+++ b/core/modules/link/lib/Drupal/link/Plugin/field/widget/LinkWidget.php
@@ -52,9 +52,9 @@ public function formElement(FieldInterface $items, $delta, array $element, $lang
+    $is_field_edit_form = (!empty($element['#entity']->field_ui_default_value));

Is this trick documented?

andypost’s picture

Issue summary: View changes

non-form validation ?

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Needs work

I think most of this has already been fixed. The only part that seems left is making validateTitle() a static method.

Mac_Weber’s picture

FileSize
1.96 KB

This file has been moved and it also has changed a lot.
Sending a new patch with the proposed changes.

Mac_Weber’s picture

Status: Needs work » Needs review

sending to testbot

Status: Needs review » Needs work

The last submitted patch, 6: link_widget_validate-2076321-6.patch, failed testing.

yched’s picture

Title: Link "title" validation is wonky » Link "title" validation should use a static method
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.22 KB

Meanwhile, #2417793: Allow entity: URIs to be entered in link fields added validateUriElement() as a static '#element_validate' for the 'uri' element, we should be consistent.

Patch:
- renames validateTitle() to validateTitleElement() for consistency,
- moves the two methods closer together,
- unifies their phpdocs
- cleans up a couple stale "use" statements while we're in there

yched’s picture

Issue summary: View changes
yched’s picture

Issue summary: View changes
dawehner’s picture

So what exactly is the advantage of making it static again? We have some call to t() for example with that patch. There is no reason to not have it static,
okay, but what is the reason to change it then?

yched’s picture

@dawehner: having FAPI #callbacks as static means we don't find oursevles serializing the Widget object when the $form gets serialized.

That's the pattern we tried to uniformly apply throughout all core widgets (cause, multiply that by all widgets present in the form, and the size of the serialized data explodes)

I wish we hadn't allowed FAPI callbacks as non-static methods in D8 to begin with, and stuck to the "if it needs context, place it in #properties in the $element" motto as in D7, because of the blissfully-chain-serialize-everything-and-its-mom situation we have now, but well, that battle is lost. At least we can try to not make things worse when we don't have to :-)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

meh

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2076321-LinkWidget_title_validate-9.patch, failed testing.

pcambra’s picture

Status: Needs work » Needs review
FileSize
2.68 KB

Plain reroll

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

that was just taking out changes that were taking out unused use statements.

though I'm not sure how 9 passed removing use for ConstraintViolationListInterface
since #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme put that in on jan 30th.

re-rtbc'ing

but I wonder why we didn't add a test to show the problem for programatic saves. #9 took off the needs tests tag. is that because at that time this changed from a bug to a task? maybe this needs a beta evaluation.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs beta evaluation

Could we get a beta evaluation here, please?

yched’s picture

Issue summary: View changes

Clarified the IS and added the beta evaluation

yched’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Is it just me that finds having $this->t() and t() within the same class super ugly?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

re #21 I guess I am. I think we should have a \Drupal::t() so at least this can be autoloaded.

Committed 643f136 and pushed to 8.0.x. Thanks!

  • alexpott committed 643f136 on 8.0.x
    Issue #2076321 by yched, Mac_Weber, pcambra: Link "title" validation...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.