Problem/Motivation

As of (at least) HTML 5 the element is allowed to use the attribute 'maxlength'. The Textfield class as well as the Passwordfield do support this attribute and I don't see any good reason why the Textarea should be a leftover.

Proposed resolution

Update Textarea class and Form API Reference page.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steamx created an issue. See original summary.

Sagar Ramgade’s picture

Patch attached which allows others attributes too.

Status: Needs review » Needs work

The last submitted patch, 2: drupal-Add_#maxlength_to_textarea_in_Form_API-2594553-2.patch, failed testing.

Sagar Ramgade’s picture

cilefen’s picture

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

Feature requests must go to 8.1.x because 8.0.x is in RC.

Sagar Ramgade’s picture

Status: Needs work » Needs review

The last submitted patch, 2: drupal-Add_#maxlength_to_textarea_in_Form_API-2594553-2.patch, failed testing.

steamx’s picture

Either the test fails or the '#' character causes trouble. I thought retesting would make sense as the test bot correctly discovered the file. Weird :(

Revathi.B’s picture

I had applied this patch.This is not working for me.Please retest the patch.

steamx’s picture

As you can see the test already had been retested and failed so why'd you try this one out in the first place?

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

steamx’s picture

Issue tags: +API change

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

shadcn’s picture

Attached are two patches. The first patch has the test and is expected to fail. The second patch has the test + fix.

shadcn’s picture

The last submitted patch, 15: add_maxlength_to-2594553-15-fail.patch, failed testing.

gambry’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Render/Element/Textarea.php
    @@ -62,4 +64,20 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
    +    Element::setAttributes($element, array('id', 'name', 'value', 'cols', 'rows', 'resizable', 'maxlength', 'placeholder'));
    

    Use array short syntax []. Also this feedback applies to the rest of the patch (multiple occurrences).

  2. +++ b/core/modules/system/src/Tests/Form/ElementTest.php
    @@ -183,4 +183,12 @@ public function testFormElementErrors() {
    +  function testMaxLength() {
    

    Visibility missing

  3. +++ b/core/modules/system/src/Tests/Form/ElementTest.php
    @@ -183,4 +183,12 @@ public function testFormElementErrors() {
    +  }
    

    expected 1 blank line after function.

chiranjeeb2410’s picture

Assigned: Unassigned » chiranjeeb2410

@gambry,

Could you be a bit more specific on the second suggestion.

gambry’s picture

Yes of course!
If you open the (passing) test results you'll see this coding standard issue message:

core/modules/system/src/Tests/Form/ElementTest.php
line 194 The closing brace for the class must have an empty line before it

There must be an empty line between each function/class method and also before the closing brace for a class.

Installing and integrating Coder with your editor will help you identify these before creating the patch: https://www.drupal.org/project/coder

yogeshmpawar’s picture

yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
4.17 KB

Updated patch as per changes mentioned in #18 & also previous patch failed to apply on 8.4.x branch.

chiranjeeb2410’s picture

Status: Needs review » Reviewed & tested by the community

Clear with all the changes. All the fixes suggested on #18 are met cleanly.
Also, previous patch fails to apply as mentioned by @Yogesh Pawar. However, current
patch applies well. Updating to RTBC.

The last submitted patch, 4: drupal-add_maxlength_to_textarea_in_form_api-2594553-2.patch, failed testing.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change

The patch seems to be fixing what it says and has test coverage. However, we should polish the implementation to make it consistent with the pre-existing implementation.

+++ b/core/lib/Drupal/Core/Render/Element/Textarea.php
@@ -62,4 +64,29 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
+  public static function preRenderTextarea($element) {

Instead of adding a new pre-render function, we should extend the pre-existing implementation which is in template_preprocess_textarea. We should also make sure that the API addition is documented in both, the preprocess function and the render element so that it shows up nicely in the Drupal.org documentation.

I also removed the API change tag since this is not an actual API change, but just a pure API addition.

Thanks everyone who has been working on this issue!

lauriii’s picture

Issue tags: +Needs change record

This also needs a change record.

gambry’s picture

Feedbacks from #26 should be addressed. I'm not sure where else the maxlength property/attribute should be documented, but I'm assuming the update on the Form API Reference for 8.x should be done manually, correct?

garnett2125’s picture

Needed to change patch to apply it on Drupal 8.2.7

UPDATE Sorry wrong file and maybe not the right place to post this as its not for 8.4.x version

Status: Needs review » Needs work

The last submitted patch, 29: add_maxlength_to-2594553-29.patch, failed testing.

sudhanshug’s picture

Status: Needs work » Needs review
gambry’s picture

gambry’s picture

Issue tags: -Needs change record

CR is created, so all feedbacks from #26 and #27 should be addressed.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

As far as I can tell, @laurii's feedback has been adressed, thanks!

The last submitted patch, 15: add_maxlength_to-2594553-15.patch, failed testing. View results

gambry’s picture

Not sure how #15 got to be tested. RE-queuing #28 for confirmation.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/src/Tests/Form/ElementTest.php
    @@ -183,4 +183,13 @@ public function testFormElementErrors() {
    +   * Text the maxlength attribute.
    

    nit: 'Test' not 'Text'

  2. +++ b/core/modules/system/src/Tests/Form/ElementTest.php
    @@ -183,4 +183,13 @@ public function testFormElementErrors() {
    +    $this->assertPattern('/<input .* id="edit-title" .* maxlength="255" .* \/>/', 'Text field has correct maxlength in form.');
    +    $this->assertPattern('/<textarea .* id="edit-description" .* maxlength="255" .*>/', 'Textarea field has correct maxlength in form.');
    

    This is brittle to the markup being generated.

    Can you use an xPath expression instead?

    Or if you prefer, use the css select helper.

    $elements = $this->cssSelect('textarea[name=description][maxlength=255]');
    $this->assertNotEmpty($elements);
    

    instead.

  3. +++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml
    @@ -489,3 +489,10 @@ form_test.get_form:
    +form_test.max_length:
    
    +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMaxLength.php
    @@ -0,0 +1,50 @@
    +namespace Drupal\form_test\Form;
    

    We're trying to avoid adding new test forms and routes for testing render elements, especially for tests based on WebTestBase. Can you use a kernel test that doubles as a form instead?

    See \Drupal\KernelTests\Core\Element\PathElementFormTest for an example.

larowlan’s picture

Would be great if we could open up a postponed follow-up that added this ability to the 'Text Long' fields, we already collect a max length in their field settings, so should be a matter of passing it through in the widget.

Looking great

gambry’s picture

This patch covers feedbacks on #37. I've never thought about using Kernel tests to test forms. I've never thought in general to use Kernel test for checking rendered HTML.
Thanks a lot @larowlan, that was a really useful review!

Status: Needs review » Needs work

The last submitted patch, 39: add_maxlength_to-2594553-39--test-only.patch, failed testing. View results

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community

As far as I can tell all of @larowlan's feedback has been adressed. Opened a follow-up per #38. Should be good to go, from my perspective. Thanks @larowlan and @gambry!

larowlan’s picture

Issue tags: +Needs change record

Updating issue credit to include reviewers who changed the course/shape of the patch.

  • larowlan committed 2e9f821 on 8.4.x
    Issue #2594553 by gambry, arshadcn, Sagar Ramgade, Yogesh Pawar,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record +8.4.0 release notes

Committed as 2e9f821 and pushed to 8.4.x.

Published the change record.

Unpostponed the follow-up.

Added the 8.4.0 release notes tag, I for one can already think of a client project where this will be useful.

Thanks for adding a great feature!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -8.4.0 release notes

I'm not sure this fix quite merits a release notes mention. The CR should be sufficient to inform people of the improvement. Thanks!