Use validation functions from the libphonenumber library

I've added the additional constraint. Please test and review.

added a new patch. Please review

reroll the patch. Please review

Reroll the patch. kindly review.

What’s the difference between the patches and the MR?

Also it would be nice to provide an interdigital or explanation why you post a changed patch.

It is counter productive now and takes time to analyse what is going on. What patch does need review or is it the MR? If patches become obsolete, you can hide them.

So please go through the patches and MR and provide some feedback what is going on.

Patches is against the alpha version while Merge request is against the dev branch. Previously i was created patch from dev branch and codebase is on alpha version so it was not getting applied thats why i have created patch from alpha branch and now it is working.
@tim-diels

suryabhi → closed merge request !9

I tried different approach in the recent. Kindly review

The last submitted patch, 5: 3376601-3.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Patch applied. Done

Am I missing something here? Why is this set as fixed? And still very chaotic communication. Please try to not open 2 MR’s and have so many patches without providing info what is changed.

@malcomio can you help with what is correct here?

suryabhi → closed merge request !8

No need to review the patches - they don't relate to the dev branch.

The two merge requests are alternative approaches:

MR 8 - uses $this->context->addViolation
MR 10 - uses $form_state->setError

In our implementation we encountered issues with the addViolation approach, where the errors were displaying twice (although that may have been because of some code in a custom module).

To me, it seems cleaner to use addViolation, as this is consistent with the rest of the module.

suryabhi → reopened merge request !8

Sorry for the more work, but needs rebasing.

Looking at our existing validation it certainly seems like adding a call out to \libphonenumber\PhoneNumberUtil::isValidNumber() would duplicate existing, earlier checks that validate the entered number against the chosen country. I must be missing something. Could you provide more detail on how to reproduce the issue that this addition resolves?

One example where we found that this patch was needed was when a user selected India, then entered "999" as the phone number, they did not see any error message.

The function in the library is documented at https://github.com/giggsey/libphonenumber-for-php/blob/master/src/PhoneN... - there aren't many examples in the tests there, but one is +33 6 76 83 51 85 for Germany

Wow. That is a problem. Thank you for catching this. I am re-categorizing this issue as a bug report.

I lean toward adding this new check in Drupal\phone_number\PhoneNumberUtil::testPhoneNumber() because that is a centralized checker used by both Drupal\phone_number\Plugin\Validation\Constraint\PhoneNumberValidator::validate() (line 69, $phone_number = $item->getPhoneNumber(TRUE);) and Drupal\phone_number\Element\PhoneNumber::phoneNumberValidate() (line 244).

That would ensure this new check is called in (I think) all situations (render element usage, regular field usage, programmatic entity field usage, etc.) and probably avoid any duplicated error messages.

And to ensure this bug stays fixed, I think we should add a test confirming the fix. It could be as simple as submitting India + 999 into a phone number field, confirming that the submission is not accepted, and confirming the expected form validation error is displayed.