#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
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 ?
Comment | File | Size | Author |
---|---|---|---|
#16 | 2076321-LinkWidget_title_validate-16.patch | 2.68 KB | pcambra |
#9 | 2076321-LinkWidget_title_validate-9.patch | 3.22 KB | yched |
#6 | link_widget_validate-2076321-6.patch | 1.96 KB | Mac_Weber |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch:
- 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 :-)
Comment #2
yched CreditAttribution: yched commentedAlso, 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 ?
Comment #3
yched CreditAttribution: yched commentedSo, broader title.
Comment #4
andypostSeems this needs test
Is this trick documented?
Comment #4.0
andypostnon-form validation ?
Comment #5
jhedstromI think most of this has already been fixed. The only part that seems left is making
validateTitle()
a static method.Comment #6
Mac_Weber CreditAttribution: Mac_Weber commentedThis file has been moved and it also has changed a lot.
Sending a new patch with the proposed changes.
Comment #7
Mac_Weber CreditAttribution: Mac_Weber commentedsending to testbot
Comment #9
yched CreditAttribution: yched commentedMeanwhile, #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
Comment #10
yched CreditAttribution: yched commentedComment #11
yched CreditAttribution: yched commentedComment #12
dawehnerSo 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?
Comment #13
yched CreditAttribution: yched commented@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 :-)
Comment #14
dawehnermeh
Comment #16
pcambraPlain reroll
Comment #17
YesCT CreditAttribution: YesCT commentedthat 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.
Comment #18
webchickCould we get a beta evaluation here, please?
Comment #19
yched CreditAttribution: yched commentedClarified the IS and added the beta evaluation
Comment #20
yched CreditAttribution: yched commentedComment #21
alexpottIs it just me that finds having $this->t() and t() within the same class super ugly?
Comment #22
alexpottre #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!