Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#39 | interdiff-28-39.txt | 5.06 KB | gambry |
#39 | add_maxlength_to-2594553-39--test-only.patch | 2.36 KB | gambry |
#39 | add_maxlength_to-2594553-39.patch | 4.03 KB | gambry |
Comments
Comment #2
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedPatch attached which allows others attributes too.
Comment #4
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedOops patch renamed.
Comment #5
cilefen CreditAttribution: cilefen commentedFeature requests must go to 8.1.x because 8.0.x is in RC.
Comment #6
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedComment #8
steamx CreditAttribution: steamx commentedEither the test fails or the '#' character causes trouble. I thought retesting would make sense as the test bot correctly discovered the file. Weird :(
Comment #9
Revathi.B CreditAttribution: Revathi.B commentedI had applied this patch.This is not working for me.Please retest the patch.
Comment #10
steamx CreditAttribution: steamx commentedAs you can see the test already had been retested and failed so why'd you try this one out in the first place?
Comment #12
steamx CreditAttribution: steamx commentedComment #15
shadcn CreditAttribution: shadcn at Chapter Three commentedAttached are two patches. The first patch has the test and is expected to fail. The second patch has the test + fix.
Comment #16
shadcn CreditAttribution: shadcn at Chapter Three commentedComment #18
gambryUse array short syntax
[]
. Also this feedback applies to the rest of the patch (multiple occurrences).Visibility missing
expected 1 blank line after function.
Comment #19
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@gambry,
Could you be a bit more specific on the second suggestion.
Comment #20
gambryYes of course!
If you open the (passing) test results you'll see this coding standard issue message:
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
Comment #21
yogeshmpawarComment #22
yogeshmpawarUpdated patch as per changes mentioned in #18 & also previous patch failed to apply on 8.4.x branch.
Comment #23
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commentedClear 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.
Comment #25
gambryComment #26
lauriiiThe 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.
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!
Comment #27
lauriiiThis also needs a change record.
Comment #28
gambryFeedbacks 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?
Comment #29
garnett2125 CreditAttribution: garnett2125 commentedNeeded 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
Comment #31
sudhanshug CreditAttribution: sudhanshug at Google Summer of Code commentedComment #32
gambryComment #33
gambryCR is created, so all feedbacks from #26 and #27 should be addressed.
Comment #34
tstoecklerAs far as I can tell, @laurii's feedback has been adressed, thanks!
Comment #36
gambryNot sure how #15 got to be tested. RE-queuing #28 for confirmation.
Comment #37
larowlannit: 'Test' not 'Text'
This is brittle to the markup being generated.
Can you use an xPath expression instead?
Or if you prefer, use the css select helper.
instead.
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.
Comment #38
larowlanWould 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
Comment #39
gambryThis 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!
Comment #41
tstoecklerAs 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!
Comment #42
larowlanUpdating issue credit to include reviewers who changed the course/shape of the patch.
Comment #44
larowlanCommitted 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!
Comment #46
xjmI'm not sure this fix quite merits a release notes mention. The CR should be sufficient to inform people of the improvement. Thanks!