Updated: Comment #N

Problem/Motivation

#2131851: Form errors must be specific to a form and not a global added a new interface for setting errors, while we still are calling procedural functions.

Proposed resolution

Switch function calls to method calls.

Remaining tasks

User interface changes

API changes

API addition:
FormBase::setFormError() proxies through to FormErrorInterface::setErrorByName()

Comments

amateescu’s picture

FormBase::setFormError() proxies through to FormErrorInterface::setErrorByName()

What's the reason for "hiding" the method name (setErrorByName())? Are we not proud of it or something? :)

dawehner’s picture

What's the reason for "hiding" the method name (setErrorByName())? Are we not proud of it or something? :)

I like the fact that this potentially makes it more decoupled.

tim.plunkett’s picture

StatusFileSize
new57.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,212 pass(es).
[ View ]

I think the difference between FormBuilder::setError() and FormBuilder::setErrorByName() is confusing, and we shouldn't expose that to FormBase. We can pick a good name, and then if we ever rename or refactor or remove the FormBuilder ones, we don't have to update the names throughout.

Rerolled.

h3rj4n’s picture

StatusFileSize
new57.88 KB
FAILED: [[SimpleTest]]: [MySQL] 59,069 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new57.93 KB
FAILED: [[SimpleTest]]: [MySQL] 59,109 pass(es), 128 fail(s), and 58 exception(s).
[ View ]
new1.53 KB
new2.08 KB

Why don't you use the dependency injection? I created a patch that does this after rerolling your patch.

Added a interdiff of the merge conflicts and a interdiff with the changes of the dependency injection.

The last submitted patch, 4: form_set_error-2145007-4.patch, failed testing.

tim.plunkett’s picture

Because that won't work. See your failures. We have to follow the pattern of the setters/getters.

The last submitted patch, 4: form_set_error_reroll-2145007-4.patch, failed testing.

tim.plunkett’s picture

This didn't need a reroll at all. Please review the patch in #3.

tim.plunkett’s picture

Issue tags:+FormInterface
StatusFileSize
new56.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,287 pass(es).
[ View ]

Now it needed a reroll. Aggregator categories were removed.

jibran’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldEditForm.php
@@ -172,7 +172,7 @@ public function validateForm(array &$form, array &$form_state) {
-      form_error($form['field']['cardinality_container']['cardinality_number'], $form_state, $this->t('Number of values is required.'));
+      $this->setFormError('field][cardinality_number', $form_state, $this->t('Number of values is required.'));

Umm is it correct?

tim.plunkett’s picture

Yes, when dealing with fieldsets the #parents are a bit different. There are many more usages of form_set_error (the style with the string) than form_error (the style with the form element), and so I chose the string-based one. Also, you must have access to the $form and $form_state to use form_error, whereas the string-based you only need $form_state.

I manually tested all of the fieldset based ones, as well.

jibran’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormBase.php
    @@ -214,4 +221,33 @@ protected function container() {
    +   * @param string $name

    It can also be NULL

  2. +++ b/core/lib/Drupal/Core/Form/FormBase.php
    @@ -214,4 +221,33 @@ protected function container() {
    +   *   The name of the form element.

    Perhaps little more description explaining that what can be used as name.

tim.plunkett’s picture

StatusFileSize
new57.94 KB
PASSED: [[SimpleTest]]: [MySQL] 59,322 pass(es).
[ View ]
new1.67 KB

1) PHP casts NULL to an empty string when used as an array indices, so let's just always use a string.
2) I think the @see is enough here.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for the fixes. It is a simple patch nothing else to complain so RTBC.

alexpott’s picture

Title:Convert form_set_error() in FormBase classes to use FormErrorInterface» Change notice: Convert form_set_error() in FormBase classes to use FormErrorInterface
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record, +API change, +Approved API change

Committed 560e5af and pushed to 8.x. Thanks!

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
jonreid’s picture

Summary

  • Classes that inherit from (extend) FormBase and its children now use the protected function setFormError instead of form_set_error()
  • The new function accepts the same arguments as were used previously: the name of the form element (string $name), an associative array containing the current state of the form (array $form_state) and an optional message to show to the user( string $message)
  • For example in FeedFormController.php:

Before

<?php
class FeedFormController extends ContentEntityFormController {
    
$result = $feed_storage_controller->getFeedDuplicates($feed);
     foreach (
$result as $item) {
       if (
strcasecmp($item->title, $feed->label()) == 0) {
       
form_set_error('title', $form_state, $this->t('A feed named %feed already exists. Enter a unique title.', array('%feed' => $feed->label())));
       }
       if (
strcasecmp($item->url, $feed->url->value) == 0) {
       
form_set_error('url', $form_state, $this->t('A feed with this URL %url already exists. Enter a unique URL.', array('%url' => $feed->url->value)));
       }
     }
    
parent::validate($form, $form_state);
?>

After

<?php
class FeedFormController extends ContentEntityFormController {
    
$result = $feed_storage_controller->getFeedDuplicates($feed);
     foreach (
$result as $item) {
       if (
strcasecmp($item->title, $feed->label()) == 0) {
       
$this->setFormError('title', $form_state, $this->t('A feed named %feed already exists. Enter a unique title.', array('%feed' => $feed->label())));
       }
       if (
strcasecmp($item->url, $feed->url->value) == 0) {

       

$this->setFormError('url', $form_state, $this->t('A feed with this URL %url already exists. Enter a unique URL.', array('%url' => $feed->url->value)));
       }
     }
    
parent::validate($form, $form_state);
?>
jonreid’s picture

Status:Active» Needs review

Created change notice comment.

Cottser’s picture

Thanks @jonreid! In the code samples I'm not sure if having the class declaration is useful, but I would definitely show that the code is in the validateForm method, and perhaps seek out a shorter example.

xjm’s picture

Issue tags:+Missing change record

Thanks @jonreid and @Cottser! Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release We can even use the new change record draft feature to iterate on it. :)

Cottser’s picture

Yay! Created my first draft change record: https://drupal.org/node/2186135

Revisions welcome, publish it when it's good to go :)

Thanks again @jonreid!

xjm’s picture

Title:Change notice: Convert form_set_error() in FormBase classes to use FormErrorInterface» Convert form_set_error() in FormBase classes to use FormErrorInterface
Priority:Major» Normal
Status:Needs review» Fixed
Issue tags:-Needs change record, -Approved API change, -Missing change record+Approved API change]

Looks great to me! Published.

xjm’s picture

Issue tags:-Approved API change]+Approved API change

Status:Fixed» Closed (fixed)

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