Problem/Motivation

\Drupal\Core\Form\FormValidatorInterface and \Drupal\Core\Form\FormValidator are missing some type hints, both in function signatures and docblocks. Also, several variables and methods have somewhat misleading names.

Proposed resolution

Add type hinting and documentation, and improve method and variable names.

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#1 drupal_2469799_1.patch19.92 KBXano
#11 drupal_2469799_11.patch609 bytesGirish-jerk
#14 interdiff-11-14.txt482 bytesAda Hernandez
#14 drupal_2469799_14.patch618 bytesAda Hernandez
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Active » Needs review
FileSize
drupal_2469799_1.patch19.92 KB

Status: Needs review » Needs work

The last submitted patch, 1: drupal_2469799_1.patch, failed testing.

Status: Needs work » Needs review

Xano queued 1: drupal_2469799_1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: drupal_2469799_1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -142,7 +142,7 @@ public function __construct(FormValidatorInterface $form_validator, FormSubmitte
-  public function getFormId($form_arg, FormStateInterface &$form_state) {
+  public function getFormId($form_arg, FormStateInterface $form_state) {

These are wrong. The & is still needed, even though they are objects, because that is the only way the object can be *replaced*. See http://3v4l.org/HmA7s

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Girish-jerk’s picture

Girish-jerk CreditAttribution: Girish-jerk at Valuebound commented
Status: Needs work » Needs review
FileSize
drupal_2469799_11.patch609 bytes

Added type hints for documentation.
please review.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks solid.

Maybe we should fix the first line of documentation for ::$safeCoreValueCallables in this issue as well, but that might be solved in #2572709: Fix 'Drupal.Files.LineLength' coding standard or any of the other phpcs issues. I can't find one for the specific first line of documentation phpcs rule. That's why I think that this can go in as-is.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -84,11 +84,15 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
+   * The form submit.

This should be The form submitter..

Ada Hernandez’s picture

Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commented
Status: Needs work » Needs review
FileSize
drupal_2469799_14.patch618 bytes
interdiff-11-14.txt482 bytes

#13 fixed.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

#14 fixes #13.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed d3bcb70752 to 8.6.x and 78cd81a842 to 8.5.x. Thanks!

Backported to 8.5.x as this is documentation.

  • alexpott committed d3bcb70 on 8.6.x 
    Issue #2469799 by Adita, Xano, Girish-jerk: improve documentation and...

  • alexpott committed 78cd81a on 8.5.x 
    Issue #2469799 by Adita, Xano, Girish-jerk: improve documentation and...

Status: Fixed » Closed (fixed)

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