Set #maxlength for TelephoneDefaultWidget to the one specified in TelephoneItem plugin

Hardcoding #maxlength in form element seems like a better option.

Can someone help me out with the failed test case on the above patch of comment #15 ?

Hardcoding #maxlength in form element seems like a better option. Drupal 9.0.x

Tested #18 on 9.1, max length has been hard coded
RTBC + 1

Applied patch #18 on 9.2.x and it works fine. The max length in form element is set to 256 after patch.Adding screenshots.

Before patch:

After patch:

I'm moving it to RTBC

Let's add a constant \Drupal\telephone\Plugin\Field\FieldType\TelephoneItem::MAX_LENGTH so this is maintained in a single place.

IGNORE

As per #23

Applied #18 patch applied successfully and looks good for me
Thanks for the patch
for ref sharing screenshot....

+++ b/core/modules/telephone/src/Plugin/Field/FieldWidget/TelephoneDefaultWidget.php
@@ -66,6 +67,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      '#maxlength' => TelephoneItem::getMax(),

Just a minor suggestion here, IMHO I think we should use TelephoneItem::MAX_LENGTH directly, I mean, the constant is already public and this way we avoid calling the function and then the variable. What do you think?

That's it from my side, thanks for the patch!

I agree with #34 so this should be rerolled with that suggestion.

Also needs tests

Kindly review patch for version 9.5x,,

Can you please provide an interdiff so we can see the changes

#38 is literally the same patch as #32
So

• It's an unnecessary patch adding noise to the issue
• The feedback that got it switched "Needs Work" has not been addressed, so that work still needs to be done. Therefore, the status should still be "needs work"

Also remove "needs reroll" tag, I ran patch #32 on 9.5.x. It applies and tests pass, which confirms no reroll is necessary. A reroll is only needed if the patch doesn't apply to the current dev branch. Patches created during earlier dev branches are often able to apply to later ones. A reroll is typically only necessary if they specifically target lines that have changed since the patch was created.

Moving to NW for the test cases

Starting at #32

Addressed #33 and add a simple test line

Verified and tested patch #42. Patch applied successfully on 9.5.x-dev.

Test Steps:
1. Patch #42 includes #33 (to use TelephoneItem::MAX_LENGTH)
2. Apply patch
3. Check #maxlength for TelephoneDefaultWidget is now set to 256.

Test Result:
#maxlength for TelephoneDefaultWidget is now set to 256.

Moving to RTBC

Before patch:

After patch:

A test was added in #42 and there has been no review of that. Setting to NW for a code review.

I see that screenshots were added for that on 23 December 2021 at 17:54 in #31 for a 1 year old patch. That patch, #17, was failing tests on 17 Dec 2020. Therefore credit has been removed.

@quietone updated issue summary with most recent screenshots in #44.

Moving to NR for the code review portion.

Patch #42 Tested and Applied successfully on Drupal 9.5.x.

Steps to Reproduce:
1. Enable telephone module in the Drupal.
2. Create new field with the Telephone number field type.
3. Try to create the content inspect and check the max-length attribute of the Telephone number field.
4. Apply the patch.

Test Results:
After applying the patch. Field max-length is updated into the 256.

Triaging for bugsmash so tagging.

@Manibharathi E R Please read comments #45 and #46. The manual testing is complete and the test code just needs code review so there was no need to retest. Thanks.

I have reviewed the test code in #42 and it appears correct and the test-only patch failed as expected.

Marking RTBC based on the following. Thanks, everyone!

1. Title and issue summary are clear and accurate
2. Has steps to reproduce
3. Has embedded "before" and "after" screenshots from #44
4. Metadata seems fine
5. Patch applies cleanly
6. No coding standard issues in test bot
7. Code addresses issue and does not change unrelated things
8. Tests have been added and pass
9. Test-only patch fails
10. Issue was reproducible
11. Manual testing was successful per #44

+++ b/core/modules/telephone/src/Plugin/Field/FieldType/TelephoneItem.php
@@ -21,6 +21,8 @@
+  const MAX_LENGTH = 256;
+

The const should have documentation and we should also use this in \Drupal\telephone\Plugin\Field\FieldType\TelephoneItem::schema() and \Drupal\telephone\Plugin\Field\FieldType\TelephoneItem::getConstraints() - here we can remove the $max_length variable completely.

Addressed points in #51

#52 applied successfully on Drupal 10.1.x-dev, Thanks for the patch

1. +++ b/core/modules/telephone/src/Plugin/Field/FieldType/TelephoneItem.php
   @@ -22,6 +22,11 @@
   +   * Set the max length of the telephone widget. Without the default is 128.
   

   Nit: I think we can remove the "With the default" comment. I don't think that's relevant to the const.

2. +++ b/core/modules/telephone/tests/src/Functional/TelephoneFieldTest.php
   @@ -89,6 +89,7 @@ protected function setUp(): void {
   +    $this->assertSession()->elementAttributeContains('css', 'input[name=\'field_telephone[0][value]\']', 'maxlength', '256');
   

   I think this should use the const as well.

   Does it fail when an int is passed? If is does we should just cast it to a string

Addressed the comment #54 point 1.
point 2 need to be addressed
Could not apply patch #52 in Drupal 10.1.x
Attached patch against Drupal 10.1.x

Patch #55 is working fine, please check screenshot attached for reference.

Check the screenshot for after patch #55 applied

#54 still needs addressing. Recommend to use the patch from #52

Addressed the feedback in #54, this patch is based on #52. Since the feedback was mine I'm leaving it in needs review rather than RTBC.

Tested on Drupal 10.1.x and PHP 8.1

- Title and issue summary are clear
- Was able to reproduce the issue using the steps provided
- Patch applies cleanly and fixes the issue
- No coding standard issues detected
- Did a code review and no issues were identified
- Tests pass

Marking as RTBC, not adding screenshots since they have already provided earlier and not much has changed since

1. +++ b/core/modules/telephone/src/Plugin/Field/FieldType/TelephoneItem.php
   @@ -22,6 +22,11 @@
   +   * Set the max length of the telephone widget.
   

   While the purpose of adding this const was to make this value available to the telephone widget, this description is not accurate. It is used for more than that.

   If it described it is the max length for storage, that should be fine since it's true within this class. How it's used by other classes invoking it is up to them.

2. +++ b/core/modules/telephone/tests/src/Functional/TelephoneFieldTest.php
   @@ -89,6 +90,7 @@ protected function setUp(): void {
   +    $this->assertSession()->elementAttributeContains('css', 'input[name=\'field_telephone[0][value]\']', 'maxlength', TelephoneItem::MAX_LENGTH);
   

   No need to escape the single quotes for the input[name] selector just use double.

Tried to address points in #62

I think it ready, update IS

Changed on commit, I didn't feel it was ok to knock this back for the third time on a comment for a constant.

diff --git a/core/modules/telephone/src/Plugin/Field/FieldType/TelephoneItem.php b/core/modules/telephone/src/Plugin/Field/FieldType/TelephoneItem.php
index 7e2ff790044..3cec358056e 100644
--- a/core/modules/telephone/src/Plugin/Field/FieldType/TelephoneItem.php
+++ b/core/modules/telephone/src/Plugin/Field/FieldType/TelephoneItem.php
@@ -23,7 +23,7 @@
 class TelephoneItem extends FieldItemBase {
 
   /**
-   * Default max length for telephone storage.
+   * The maximum length for a telephone value.
    */
   const MAX_LENGTH = 256;
 

Removing issue credit for screenshots that were added when we already had screenshots
Adding issue credit for those who reviewed the code that had an outcome on the final result

Committed to 10.1.x, backported to 10.0.x and 9.5.x as the risk of disruption is low.